Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cursor styles API #168

Merged
merged 13 commits into from
Dec 31, 2018
Merged

Add cursor styles API #168

merged 13 commits into from
Dec 31, 2018

Conversation

OhadRau
Copy link
Collaborator

@OhadRau OhadRau commented Dec 27, 2018

This addresses #144, implementing cursors as a style property (similar to how colors work now). I've also updated Clickable to use the pointer style cursor. Example of the API:

module Cursors = Revery_UI.MouseCursors;
<view style=Style.make(cursor: Cursors.text, ())>
  <text> "This looks like a textbox when you hover over it!" </text>
</view>

This also implements a really hacky precursor to event bubbling, where we select the top-most element below the mouse based on the depth in the DOM and choose the cursor from that node. While this works for basic cases, #164 would provide a much more flexible way of doing this (so let me know if you think I should block this until that's merged in). I also added a Window.t parameter to Mouse.dispatch to allow the cursor to get set in there (since glfwSetCursor requires that and we always have the window in scope when calling Mouse.dispatch).

Finally, here's a screenshot if you're interested in what this looks like:
image

@OhadRau
Copy link
Collaborator Author

OhadRau commented Dec 28, 2018

Just realized that this test is failing. Guess it's time to figure out what to do with the test, since it won't have a glfwWindow. I'm going to make it optional for now, but I'm open to refactoring this further in the future

(Will definitely want to clean this up, so if anyone has some input on how you think I should do that lmk)

@bryphe
Copy link
Member

bryphe commented Dec 29, 2018

This addresses #144, implementing cursors as a style property (similar to how colors work now). I've also updated Clickable to use the pointer style cursor.

Awesome! 🎉

This also implements a really hacky precursor to event bubbling, where we select the top-most element below the mouse based on the depth in the DOM and choose the cursor from that node.

This seems like a reasonable 'next step' to me. We need this logic to select the top-most element prior to having bubbling anyway - so this seems like it brings us closer to the logic we need in #164 .

Just realized that this test is failing. Guess it's time to figure out what to do with the test, since it won't have a glfwWindow. I'm going to make it optional for now, but I'm open to refactoring this further in the future

IMO, it'd be ideal if we could keep the implementation of Window/glfwWindow out of the events. Thinking about the discussion here: revery-ui/reason-reactify#35 - it might be that we want to extract a revery-core with some canonical set of components/events/layout, and so introducing some of our GLFW-specific implementation details would make that harder. In addition, having GLFW-specific details makes it harder to test this behavior in isolation - it will make our life easier architecturally if we can keep the GLFW/GL layers from becoming a cross-cutting concern.

One option I could see for removing this coupling would be to expose an event from the Mouse module - like onCursorChanged. Then, our logic here could look like:

switch (deepestNode^) {
         let glfwWin = window.glfwWindow;
         let cursor = node#getCursorStyle();
         Event.dispatch(onCursorChanged, cursor);
    };

We'd also need a place to listen to that event - the place that would make sense at the moment is the place where we bind all our other events between the 'native' world and the reconciler world: https://github.com/revery-ui/revery/blob/master/src/UI/Revery_UI.re

We could add a handler here, like:

 let _ =
    Revery_Core.Event.subscribe(
    Mouse.onCursorChanged, 
      cur => Glfw.glfwSetCursor(window, cur);
    );

It means that the Mouse itself doesn't have to care about the GLFW world, at all - it just can communicate the intent and let whatever downstream handler take care of the details.

The nice thing about this 'decoupling' is that it makes it easier to test, too - it gets us closer to being able to validate the logic w/o any rendering dependencies:

  test("onCursorChangedEvent gets dispatched with proper cursor", () => {
      let cursor = Mouse.Cursor.make();

      let count = ref(0);
     let cursor = ref(Cursors.arrow);
      let f = cur=> { count := count^ + 1; cursor := cur };
      let node =
        createNodeWithStyle(Style.make(~width=100, ~cursor=Cursors.text, ~height=100, ()));
      node#setEvents(NodeEvents.make(~onMouseDown=f, ()));

      let _ = Revery.Core.Event.subscribe(Mouse.onCursorChanged, f);

      Mouse.dispatch(
        cursor,
        InternalMouseMove({mouseX: 50., mouseY: 50.}),
        node,
      );
   
      expect(count^).toBe(1);
      expect(cursor^).toBe(Cursors.text);
    });

(I think there would still be issues as the cursors themselves depend on GLFW dependencies, but perhaps the lazy helps us here).

I find that writing tests helps not only to build up our safety net but also to rationalize the dependencies and how they relate. Just some thoughts - let me know what you think!

@OhadRau
Copy link
Collaborator Author

OhadRau commented Dec 29, 2018

Thanks for all this feedback. I'll try to fix this ASAP.

(I think there would still be issues as the cursors themselves depend on GLFW dependencies, but perhaps the lazy helps us here).

Hm, so testing out something similar in utop:

utop # let x = lazy (print_endline "Hi");;
val x : unit lazy_t = <lazy>
utop # let y = lazy (print_endline "Bye");;
val y : unit lazy_t = <lazy>
utop # x = y;;
Exception: Invalid_argument "compare: functional value".

One workaround might be a 2-layer approach here, where layer one sets the cursor through some variant (i.e. type cursorShape = Arrow | Text | Pointer) and then layer two picks that out of some kind of lazy cache (this could be a record, hashmap, or just a function returning the current values we have).

@OhadRau
Copy link
Collaborator Author

OhadRau commented Dec 31, 2018

Any way to force Azure CI to trigger? Doesn't seem like it picked up on any of the last few commits

@bryphe
Copy link
Member

bryphe commented Dec 31, 2018

Sorry about that, @OhadRau ! Just kicked off CI.

Thanks for adding the test case and abstracting out from the GLFW window implementation - looks great! 👍

Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the update - and builds are green after kicking them off again 👍

@OhadRau
Copy link
Collaborator Author

OhadRau commented Dec 31, 2018

Awesome, gonna go ahead and merge this in then!

@OhadRau OhadRau merged commit 4661b78 into revery-ui:master Dec 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants