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

Unified origin #2149

Merged
merged 19 commits into from
Dec 13, 2022
Merged

Unified origin #2149

merged 19 commits into from
Dec 13, 2022

Conversation

xarvic
Copy link
Collaborator

@xarvic xarvic commented Mar 8, 2022

This PR removes the viewport_offset property of WidgetPod and changes the ClipBox implementation to use WidgetPod::set_origin instead. This will fix incorrect hot state when scrolling.

TODOs:

  • find a way to merge WidgetPod::set_origin and WidgetPod::set_origin_dyn
  • decide when hot state should change. I think the current implementation, which is doing it during layout is wrong, since we don't know the new origin in layout.
  • change TextBox implementation to use scroll_to_view and LifeCycle::ViewContextChanged instead of manually managing its Scroll widget

@xarvic xarvic force-pushed the unified_origin branch 2 times, most recently from 72903f2 to 61d80f8 Compare July 30, 2022 07:33
@xarvic xarvic marked this pull request as ready for review July 30, 2022 12:11
@xarvic xarvic linked an issue Jul 30, 2022 that may be closed by this pull request
@jpochyla
Copy link
Contributor

Hi @xarvic, do you think this is ready to be merged?

@xarvic
Copy link
Collaborator Author

xarvic commented Aug 18, 2022

I am still waiting for awnser, how to make WidgetPod::set_origin callable from multiple contexts. TextBox for an example updates its scroll position when it recieves LifeCycle::FocusChanged. Which would otherwise be imposible.
set_viewportoffset solved this by requiering no context at all but this leads to a buggy hot state.
If we could merge #1611, doing this should be straitforward.

@xarvic
Copy link
Collaborator Author

xarvic commented Aug 18, 2022

Apart from that the PR is ready for review.

@jpochyla
Copy link
Contributor

I think #1611 was missing a use-case inside Druid, and this is a compelling one!

@xarvic xarvic force-pushed the unified_origin branch 2 times, most recently from 8cc0bd2 to 96be87f Compare August 25, 2022 08:55
@xarvic
Copy link
Collaborator Author

xarvic commented Aug 25, 2022

@jpochyla the PR is now ready for review.

@liias
Copy link
Contributor

liias commented Sep 20, 2022

FWIW: I tested this on macOS and this fixes the incorrect hot state while scrolling.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on and sorry for letting it sit so long!

Given that this changes quite a bit of code it's going to take several rounds of review. I'm going to start off with a simple surface level review of small stuff that stuck out as I went over the changes.

Next I will dig deeper to understand what is even going on here. 😄

CHANGELOG.md Outdated Show resolved Hide resolved
druid/src/contexts.rs Outdated Show resolved Hide resolved
druid/src/contexts.rs Outdated Show resolved Hide resolved
druid/src/contexts.rs Outdated Show resolved Hide resolved
druid/src/contexts.rs Outdated Show resolved Hide resolved
druid/src/event.rs Outdated Show resolved Hide resolved
druid/src/event.rs Outdated Show resolved Hide resolved
druid/src/widget/clip_box.rs Outdated Show resolved Hide resolved
druid/src/widget/clip_box.rs Outdated Show resolved Hide resolved
druid/src/widget/scroll.rs Outdated Show resolved Hide resolved
@xStrom xStrom added architecture changes the architecture, usually breaking S-waiting-on-author waits for changes from the submitter labels Dec 3, 2022
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Thanks for investing your time into making the adjustments. I have reviewed the latest commit and have a few more minor notes.

I've also been playing around with the code a bit, but a more thorough review of the substance here isn't ready yet.

druid/src/contexts.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
druid/src/core.rs Show resolved Hide resolved
druid/src/event.rs Outdated Show resolved Hide resolved
druid/src/event.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Okay, here's my first pass on the code. One interesting thing is that it's looking like there's increasingly fewer reasons to have the context trait changes. We'll see about that, for now can just look over the stuff I commented in this batch.

druid/src/event.rs Outdated Show resolved Hide resolved
druid/src/window.rs Outdated Show resolved Hide resolved
druid/src/contexts.rs Outdated Show resolved Hide resolved
druid/src/contexts.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
druid/src/core.rs Outdated Show resolved Hide resolved
- removed data and ctx from set_origin
druid/src/window.rs Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
druid/src/window.rs Outdated Show resolved Hide resolved
@xStrom xStrom added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Dec 12, 2022
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Thanks for your continued effort on this! I think this is ready for merge.

@xStrom xStrom removed the S-needs-review waits for review label Dec 12, 2022
@xarvic
Copy link
Collaborator Author

xarvic commented Dec 13, 2022

And thank you for the thorough review :)

@xarvic xarvic merged commit 0214404 into linebender:master Dec 13, 2022
@jpochyla
Copy link
Contributor

Thanks a lot Kaur!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture changes the architecture, usually breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When scrolling a list, button hot state sometimes gets stuck
4 participants