Skip to content
This repository has been archived by the owner on Jun 6, 2020. It is now read-only.

Accomodate new pan event changes #56

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

szbergeron
Copy link

Adds fixes to accommodate changes from revery-ui/esy-sdl2#11

@Et7f3
Copy link
Member

Et7f3 commented Apr 4, 2020

It seem it miss some header

@szbergeron
Copy link
Author

It seem it miss some header

Yep, this requires revery-ui/esy-sdl2#11 to build

@szbergeron
Copy link
Author

This should also hopefully be GTG once the esy changes are merged @glennsl @Et7f3

@@ -471,7 +492,7 @@ module Event = {
| WindowClosed(windowEvent)
| WindowTakeFocus(windowEvent)
| WindowHitTest(windowEvent)
| MousePan(mousePan)
| Pan(panEvent)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is mousePan still used ?

@@ -717,17 +717,37 @@ CAMLprim value Val_SDL_Event(SDL_Event *event) {
case SDL_PANEVENT:
v = caml_alloc(1, 24);

vInner = caml_alloc(9, 0);
vInner = caml_alloc(5 * 8, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 * 8 ???? why.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

me being dumb, was adjusting number of fields and forgot to change to constant

thanks for the catch


Store_field(vInner, 2, Val_int(event->pan.source));
int axis;
if( event->pan.axis == SDL_PAN_AXIS_VERTICAL ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest: switch in case we add diagonal pan. And we could get exhaustivity checking with -Wswitch-enum flag.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thankfully I think a diagonal pan is always sum of vertical and horizontal, though agree a switch is more semantically clear here


Store_field(vInner, 4, panElement);
} else {
if( event->pan.pantype == SDL_PANEVENTTYPE_INTERRUPT ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here again a switch is better: reduce nesting. Don't omit break;

@@ -717,17 +717,39 @@ CAMLprim value Val_SDL_Event(SDL_Event *event) {
case SDL_PANEVENT:
v = caml_alloc(1, 24);

vInner = caml_alloc(9, 0);
vInner = caml_alloc(40, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 is enough ? panEvent has 5 field: so 5 values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 yep, forgot it isn't malloc lol

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants