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

Make scroll view touch friendly #229

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

WTRipper
Copy link
Contributor

@WTRipper WTRipper commented Jun 27, 2020

Adds the capability to scroll in a ScrollViewer by drag touch gesture. In my first tests the scrolling is working.
However the interaction is still interpreted by the children widgets (e.g. I can scroll the ScrollViewer and drag a Slider inside the ScrollViewer at the same time). Can someone point me where I can fix this?

I tried to keep the overall architecture of ScrollViewer as it is. If everything works out and you are fine with the changes @rds1983 I think this should solve #87 .

@WTRipper
Copy link
Contributor Author

WTRipper commented Jul 4, 2020

In my last commit I added event arguments to the TouchMoved, -Entered and -Left events where the ScrollViewer can hook up an callback that is only called if the touch event isn't handled.

However there is still a lot to do. Probably not each case which handles the touch event sets the flag that the event was handled at the moment. Also a lot of the widgets process the TouchDown event instead of the TouchUp or combination of both.. Furthermore things like KeyBoardFocus are interferring at the moment.

But: A ScrollViewer with a Slider in it already works as expected - TouchMove gestures in the ScrollViewer are processed as scrolling but TouchMove gestures on the Slider are processed by the Slider only.

@WTRipper
Copy link
Contributor Author

WTRipper commented Jul 4, 2020

Made some larger refactorings but now it should be working. But I still need to test everything. What's not working at the moment is TextBox. I tried to remove most widget's logic from the OnTouchDown method to be able to scroll even if the touch gesture starts on a widget. For TextBox I think I need to undo those changes, differentiate between touch and mouse use or changing the behaviour of TextBoxes have to be focused first before one can interact with them.

@rds1983
Copy link
Owner

rds1983 commented Jul 5, 2020

Hello,
Great job!
However there are two things I would ask you to do:

  1. Rename IsToggled/ToggledChanged back to IsPressed/PressedChanged. I like your naming better, but the rename makes it way harder to review the code as a lot of files not related to this scope of this PR had been changed.
    This rename should be done as separate PR. Also old API must be preserved as wrapper around new one and marked with Obsolete attribute(i.e. see Widget.PaddingLeft property).

  2. Frankly I dont like changes to the touch events API that adds HookableEventArgs class. It seems unintuitive and hacky. Also it'll generate garbage every time TouchMoved is fire. Moreover it is used only in the ScrollViewer. Maybe there are ways to archieve same goals without the new API?

@WTRipper
Copy link
Contributor Author

WTRipper commented Jul 5, 2020

Hmm the thing is those changes where needed to provide the new functionality.

  1. IsPressed was used for two things:
    • The state of a button while button is actually pressed down. So the short time between TouchDown and TouchUp.
    • The toggled state of a toggable button.

The problem is to make a scrollviewer scrollable by touch gesture one needs to move most actions away from TouchDown (and also from from TouchUp) to a combination (that thing that I called Click). Therefore I splitted up the IsPressed state into actual IsPressed (the first usecase above) that is used for example for draggable things and therefore cannot be moved away from the TouchDown and TouchUp. And the second Property IsToggled that now acts on Click (TouchDown and TouchUp on same widget without moving outside the widget). I know that's a breaking change. If you want the IsPressed property as an obsolet property in your API we would need a second name for what is called IsPressed in this PR.. maybe IsPushed or IsPushedDown. But removing those changes from this PR to another makes the PR less useful since those changes were needed to make the scrolling work as I mentioned above.

  1. Yes, I see the downsides. Do you have another idea? That was the only thing I came up with. To handle the scroll gesture right we need to know if there is something else that would likes to interpret the touch move gesture. This can be done either:
  • By analyzing the stack of widgets first and see if there is something that likes to interpret the gesture and is flagged to block others from doing it. Downside would be every widget needs another property. That flag would also be very unflexible or we need a method that checks the touch input on itself (a lot to do for the dev now and the machine at runtime). So I think this approach would be more harmful than my approach.
  • By handling every widget first and if no widget interpreted the gesture, handling each scrollview afterwards. This would be basically a very similar approach than mine as you still need to have some kind of return or object that is passed between the calls. While this is a little bit more explicit it is the same amount of garbage, not reusable and make the code a lot more complicated.

As you can see I am not able to come up with another approach at the moment that I think is better. If you have an idea I am eager to know. On the other side I think creating one object per touch move is not as bad. EventArgs are quite common and only a small object - if really needed one could reuse the objects (but I would never suggest it as long as you are not creating hundreds of them). My HookableEventArgs is inspired by HandledEventArgs which is also made to get some feedback from the handlers to the system regarding the event was handled completely. However I liked it a bit more capsulated if one would like to change its functionality in future. Adding the possibility to add one callback makes it possible to handle ScrollViewers without going through the hierarchy two times and as long as the hierarchy is parsed from top to bottom it will also handle multiple ScrollViewers that are stacked into each other correctly.

@WTRipper
Copy link
Contributor Author

WTRipper commented Jul 7, 2020

Just as a reminder for my todo list:
I also have to look into the dropdown as it seems I broke the ability to change it's value.

@WTRipper
Copy link
Contributor Author

WTRipper commented Jul 7, 2020

@rds1983 I think I can split IsPressed into IsPressed and IsPushedDown in this PR here - and move the renaming of the other IsPressed properties and the marking as obsolete to another PR. This way there is not more than needed in this PR here but still it will be completely usable on its own.
But still we would need to decide what to do with the second point you brought up.

@rds1983
Copy link
Owner

rds1983 commented Aug 17, 2020

Sorry for the late response.
Considering 1.
Can you at least, rename IsToggled to IsPressed, and new property IsPressed to IsToggled. I know, it's probably wrong naming. However it would drastically reduce amount of changes needed to review. And the naming could be fixed as the separate PR.

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.

2 participants