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

Scroll to view #1976

Merged
merged 21 commits into from
Oct 2, 2021
Merged

Scroll to view #1976

merged 21 commits into from
Oct 2, 2021

Conversation

xarvic
Copy link
Collaborator

@xarvic xarvic commented Sep 18, 2021

This PR adds the scroll_to_view and scroll_area_to_view methods to UpdateCtx, LifecycleCtx and EventCtx, which can be used to bring specific parts of a widget into view.

There are still some things to discuss:

  • Currently the methods send commands if they are called from UpdateCtx or LifecycleCtx, but i am not sure this is the right way to do it.
  • Calling the methods produces warnings, since the Notifications are passed up to the root. Should we hard code that this is intended behavior for this Selector or search for a better way to do it, like a supress_warnings flag in Notification?

Christoph added 4 commits September 18, 2021 12:05
…ion in Scroll, improved documentation

Signed-off-by: Christoph <xarvix@web.de>
Signed-off-by: Christoph <xarvix@web.de>
Signed-off-by: Christoph <xarvix@web.de>
refactored scroll.rs

Signed-off-by: Christoph <xarvix@web.de>
@xarvic xarvic linked an issue Sep 18, 2021 that may be closed by this pull request
Christoph added 4 commits September 18, 2021 16:05
Signed-off-by: Christoph <xarvix@web.de>
Signed-off-by: Christoph <xarvix@web.de>
… to make it more efficient

Signed-off-by: Christoph <xarvix@web.de>
Signed-off-by: Christoph <xarvix@web.de>
@xarvic xarvic marked this pull request as ready for review September 18, 2021 17:33
xarvic and others added 3 commits September 18, 2021 17:37
Signed-off-by: Christoph <xarvix@web.de>
Signed-off-by: Christoph <xarvix@web.de>
Copy link
Collaborator

@maan2003 maan2003 left a comment

Choose a reason for hiding this comment

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

Some minor nits for now. haven't carefully reviewed default_scroll_to_view_handling logic yet. hope to get that done tomorrow

druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/scroll_component.rs Outdated Show resolved Hide resolved
druid/src/scroll_component.rs Outdated Show resolved Hide resolved
druid/src/scroll_component.rs Outdated Show resolved Hide resolved
druid/src/widget/mod.rs Outdated Show resolved Hide resolved
druid/src/widget/scroll.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
@xarvic
Copy link
Collaborator Author

xarvic commented Sep 21, 2021

On Desktop this should be fine, but it probably will cause problems on mobile. For an example focusing a textbox, will first update the scroll positions and then request the virtual-keyboard, which results in a layout change and therefore the text box is might be hidden again.
Do you have an idea how to solve this or is this a problem we should not care about for now?

@maan2003
Copy link
Collaborator

I think the fundamental problem is "the focused should stay in view" or "the focused widget should come in view once". maybe Scroll widget keeps track of a "focused" rect that will stay in view unless manually scroll away

@maan2003
Copy link
Collaborator

for this PR, I would just add scroll_to_view and not do it automatically on focus change yet

@xarvic
Copy link
Collaborator Author

xarvic commented Sep 21, 2021

Ok

@maan2003 maan2003 added the S-waiting-on-author waits for changes from the submitter label Sep 22, 2021
- show scrollbar when clip is moved

Signed-off-by: Christoph <xarvix@web.de>
@xarvic
Copy link
Collaborator Author

xarvic commented Sep 24, 2021

I think the fundamental problem is "the focused should stay in view" or "the focused widget should come in view once". maybe Scroll widget keeps track of a "focused" rect that will stay in view unless manually scroll away

A focused region that stays in in view until scrolled away sounds good, but i think keeping the rect wont be enough. If we have a two Scrolls in a Flex which again is wrapped in a Scroll, it could be possible that both of the inner Scrolls believe they contain the focused region and send Notifications to the outer Scroll.

Christoph and others added 3 commits September 24, 2021 17:16
Signed-off-by: Christoph <xarvix@web.de>
Signed-off-by: Christoph <xarvix@web.de>
@xarvic xarvic added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Sep 24, 2021
Signed-off-by: Christoph <xarvix@web.de>
Copy link
Collaborator

@maan2003 maan2003 left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good

druid/src/contexts.rs Outdated Show resolved Hide resolved
@maan2003 maan2003 added S-ready PR is ready to merge and removed S-needs-review waits for review labels Sep 28, 2021
xarvic and others added 3 commits October 2, 2021 07:06
Co-authored-by: Manmeet Maan <49202620+Maan2003@users.noreply.github.com>
Signed-off-by: Christoph <xarvix@web.de>
@xarvic xarvic merged commit db680a7 into linebender:master Oct 2, 2021
@xarvic xarvic deleted the scroll_to branch October 2, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ready PR is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scroll should try to show the focused widget on focus change
2 participants