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

Scrolling element using middle mouse button #423

Conversation

igorsegallafa
Copy link
Contributor

Related to: #422

@mikke89
Copy link
Owner

mikke89 commented Mar 2, 2023

Hey, welcome, and thanks for submitting the PR. I like the idea, and would be happy to accommodate this feature.

It's possible to add mousemove to the default actions. This is mainly an optimization, we only add the events here that we need to add. Event propagation can be quite slow, so ideally we want to avoid this, particularly for rapidly fired events like mouse movement.

Presumably, there is only one element that will be scrolled at a time, so I think it makes more sense to move the state variables to the Context, instead of keeping it locally in the Element. This also avoids the overhead of adding the state variables to every Element, even those that cannot or will not be scrolled. Then, you can dispatch mousescroll events from the Context, the same way Context::ProcessMouseWheel currently does this. How does this sound to you?

@mikke89 mikke89 added the enhancement New feature or request label Mar 2, 2023
@igorsegallafa
Copy link
Contributor Author

Hey, welcome, and thanks for submitting the PR. I like the idea, and would be happy to accommodate this feature.

It's possible to add mousemove to the default actions. This is mainly an optimization, we only add the events here that we need to add. Event propagation can be quite slow, so ideally we want to avoid this, particularly for rapidly fired events like mouse movement.

Presumably, there is only one element that will be scrolled at a time, so I think it makes more sense to move the state variables to the Context, instead of keeping it locally in the Element. This also avoids the overhead of adding the state variables to every Element, even those that cannot or will not be scrolled. Then, you can dispatch mousescroll events from the Context, the same way Context::ProcessMouseWheel currently does this. How does this sound to you?

Cool, good thoughts. I updated the PR to deal with logic suggested by you, but I still have some doubts:

  • Should I call the element->Scroll() function for every Element on hover chain? (like I did)
  • Do you have any idea to deal with the cursor icon? I think we doens't have a good icon for it as we have on browser. We could use another existent icon just for feature visualization purpose.. but ye, it will not look pretty. 😢

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

This looks better, nice. I have some more comments here.

The cursor, right. What the cursor looks like is really up to the client when implementing their system interface. But we should probably submit some built-in cursor names, and then users display their icon based on that. My suggestion: rmlui-scroll-idle, rmlui-scroll-up, rmlui-scroll-down.

I see that most platforms don't really provide any good default cursor icons for this. As a placeholder, we could perhaps use the move cursor in the built-in backends?

Finally, we should document in ProcessMouse...() functions that button 2 now means middle mouse.

Source/Core/Context.cpp Outdated Show resolved Hide resolved
Source/Core/Context.cpp Outdated Show resolved Hide resolved
Source/Core/Context.cpp Outdated Show resolved Hide resolved
Source/Core/Context.cpp Outdated Show resolved Hide resolved
@igorsegallafa
Copy link
Contributor Author

Suggestions already committed and now code it is simpler, thank you for review.

Copy link
Owner

@mikke89 mikke89 left a comment

Choose a reason for hiding this comment

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

The code is looking much nicer now, good job.

I tested the behavior of this locally, and I think there are some quality-of-life improvements we can make. This is mostly about making it feel good, and there aren't necessarily any right or wrong answers, but here are some suggestions:

  • Maybe cancel the scroll state if there is nothing to scroll. You can test this by checking if the event is still propagating.
  • It would be nice to support middle-mouse-hold-and-scroll, then release. I use this mode quite a lot myself.
  • The minimum scroll speed is 1px/frame. This can actually be quite fast, depending on DPI and FPS. We could accumulate subpixel movement, and submit them once they are greater than one.
  • It feels very sensitive near the reference, while opposite far away from the reference. Maybe it would feel better with a nonlinear speed vs distance? Try to square it? And perhaps add a deadzone near zero. This is mostly about tuning and feeling, we can tune this more later on as well.

Backends/RmlUi_Platform_SDL.cpp Outdated Show resolved Hide resolved
Backends/RmlUi_Platform_GLFW.cpp Show resolved Hide resolved
Source/Core/Context.cpp Outdated Show resolved Hide resolved
@igorsegallafa
Copy link
Contributor Author

  • Maybe cancel the scroll state if there is nothing to scroll. You can test this by checking if the event is still propagating.
  • It would be nice to support middle-mouse-hold-and-scroll, then release. I use this mode quite a lot myself.
  • The minimum scroll speed is 1px/frame. This can actually be quite fast, depending on DPI and FPS. We could accumulate subpixel movement, and submit them once they are greater than one.
  • It feels very sensitive near the reference, while opposite far away from the reference. Maybe it would feel better with a nonlinear speed vs distance? Try to square it? And perhaps add a deadzone near zero. This is mostly about tuning and feeling, we can tune this more later on as well.

Hmm I don't have many experience with these two remaining items that you mentioned. I ll take a look and try something.
Do you think this PR it is ready for review? Then I can take it from Draft status.

@igorsegallafa igorsegallafa marked this pull request as ready for review March 4, 2023 21:07
@igorsegallafa igorsegallafa changed the title Draft scrolling element using scroll mouse button Scrolling element using middle mouse button Mar 5, 2023
@mikke89
Copy link
Owner

mikke89 commented Mar 6, 2023

Nice, this is ready for merging in my view. We can keep tweaking it later on as well, with the mentioned requests.

Let me know if you are happy with it like this, or when you are finished with any further changes.

@igorsegallafa
Copy link
Contributor Author

I agree, we can merge it. I will quote these items that you mentioned on the issue that I opened, then we can close the issue when we finally deal with them.

@mikke89 mikke89 merged commit d129fb8 into mikke89:master Mar 7, 2023
@mikke89
Copy link
Owner

mikke89 commented Mar 7, 2023

Sounds good, thanks for the nice PR! :)

igorsegallafa added a commit to igorsegallafa/RmlUi that referenced this pull request Mar 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants