Scrolling changes and small other fixes to support druid_table #1108

rjwittams · 2020-07-30T23:01:38Z

Druid_table is at https://github.com/rjwittams/druid_table/ for now and has been discussed on Zulip.

I had to make some small changes to druid to get things working (synchronising the sibling widgets).

Disabling scroll bars on a Scroll
Adding a callback to Scroll to emit scroll offsets
Using a command to set scroll position (including from within layout so it always happens from within event - the callback passes along the evt context to allow sending commands)
Making some things in environments (Key, KeyOrValue) Clone as I need to copy them around between sibling widgets.
Added an example I was using to demonstrate synchronising scroll positions

…o for now). Mainly around synchronising the scrolling of sibling components and being able to issue commands from a few more places.

So configuration parameters can be propagated through the widget hierarchy

…n repo for now). This is to allow disabling scrollbar visibility in the Scroll widget

ForLoveOfCats · 2020-07-31T01:06:17Z

I like this apart from the fact that of course I'm going to want #1107 merged in and this rebased on that :) The ability to disable scrollbars is definitely needed and a command to ask a scroll to scroll to a position is very helpful for the 95% of time where composition suits the situation.

Adding clone to Key and friends looks harmless to me but I'm not an expert in that area. I do know that there was the worry recently about having to many fringe examples but I'll defer to someone else on that.

The CI failing is identical to what is on the aforementioned PR so it doesn't seem to be Github Actions bugging out like we've had in the past so we should probably figure out how we are depending on harfbuzz_rs as cargo tree shows nothing on my Linux system which suggests that perhaps someone on macOS should take a look as that is the platform it is definitely failing on which surprises me as the macOS piet backend uses CoreGraphics to handle text.

rjwittams · 2020-07-31T12:32:09Z

I still think 1107 needs splitting up into multiple stages (list changes, behaviour split inside the scroll file, and then gratuitous moves/renames), and this shouldn’t be blocked by it.
I’m definitely not convinced the current scroll control is ideal.

or it doesn't get a WidgetAdded.

jneem

I left a few comments. If you're willing to split out the easy parts (i.e. env.rs, contexts.rs, view_switcher.rs) then I can get them merged ASAP. For the scroll parts, I need to spend some more time with this and #1107 (also, secretly hoping that someone with more experience/authority/opinions than me will help in sorting out the conflicts...)

jneem · 2020-08-03T17:37:19Z

druid/src/contexts.rs

+
+    pub fn submit_command(&mut self, cmd: impl Into<Command>, target: impl Into<Option<Target>>) {
+        self.state.submit_command(cmd.into(), target.into())
+    }


We've started using macros to implement methods for multiple contexts. We don't seem to have one yet for the intersection of Event/Update/LifeCycle/Layout, so you'll need a new impl_context_method! block.

I noticed that too late! Can look into it

jneem · 2020-08-03T17:44:37Z

druid/src/contexts.rs

@@ -633,7 +641,7 @@ impl<'a> ContextState<'a> {
        }
    }

-    fn submit_command(&mut self, command: Command, target: Option<Target>) {
+    pub(crate) fn submit_command(&mut self, command: Command, target: Option<Target>) {


Why this change? Don't we only call it from within the module?

I think I meant to revert this - can't remember why I changed it!

jneem · 2020-08-03T17:46:04Z

druid/src/widget/scroll.rs

-        let mut offset = self.scroll_offset + delta;
-        offset.x = offset.x.min(self.child_size.width - size.width).max(0.0);
-        offset.y = offset.y.min(self.child_size.height - size.height).max(0.0);
+    pub fn scroll(&mut self, ctx: &mut EventCtx, delta: Vec2, size: Size) -> bool {


Given that we have scroll_to now, maybe scroll_by would be a better name? Might not be worth the breaking change, though.

I think you are right, I just didn't want to break anyone

jneem · 2020-08-03T17:51:10Z

druid/src/contexts.rs

@@ -615,6 +619,10 @@ impl PaintCtx<'_, '_, '_> {
            transform: current_transform,
        })
    }
+
+    pub fn viewport_offset(&self) -> Vec2 {


Can do. I will work out if I actually needed this in the end

I didn't need it so its gone

druid/src/widget/scroll.rs

jneem · 2020-08-03T18:02:47Z

druid/src/widget/scroll.rs

+            )),
+        );
+
+        ctx.submit_command(SCROLL_TO.with(scroll_to), ctx.widget_state.id);


When you submit a command in layout or lifecycle, it won't be processed until (at least) the next frame. I think scrolling immediately is better.

This was because the scroll callback takes an EventContext (so they can submit commands). I think I may have experimented with giving them the ContextState (and thats why I changed the visibility on that method)? Or something wrapped around it. Maybe need their own context that is minimal and just lets them do what we expect, that can be derived from any other context...

druid/src/widget/view_switcher.rs

druid/src/widget/scroll.rs

jneem · 2020-08-05T23:56:58Z

druid/src/widget/scroll.rs

-        let mut offset = self.scroll_offset + delta;
-        offset.x = offset.x.min(self.child_size.width - size.width).max(0.0);
-        offset.y = offset.y.min(self.child_size.height - size.height).max(0.0);
+    pub fn scroll_by(&mut self, submit: &mut SubmitCommand, delta: Vec2, size: Size) -> bool {


I see you've decided to go for the breaking change. Which is fine, but it (along with the other publicly-visible changes in this PR) will need a CHANGELOG entry...

Ok. My thoughts were that the signature had changed so much, may as well break the name.
It already was slightly odd, due to the size, which was obtained from a context from the scrolls own widget pod - calling it from outside meant doing tricks to obtain that. So it was never great as public api - what would be needed for that is “scroll here when you can”, ie a pending scroll - but this is recreating the command system afaict. I’m going to make an alternate PR, based on the binding/scope stuff I mentioned on Zulip. It’s much closer to the composition you are talking about above, and there aren’t any callbacks. Then people can pick between approaches.

rjwittams · 2020-08-23T20:35:26Z

Not much point in this if I'm going to drip feed in bits from binding scroll.

rjwittams added 4 commits July 29, 2020 09:44

Changes to druid to support the table widget (which is in its own rep…

80e8ec2

…o for now). Mainly around synchronising the scrolling of sibling components and being able to issue commands from a few more places.

Make Key and KeyOrValue implement clone

506299d

So configuration parameters can be propagated through the widget hierarchy

Clippy fixes for scroll handling

753446c

More changes to druid to support the table widget (which is in its ow…

6a781d1

…n repo for now). This is to allow disabling scrollbar visibility in the Scroll widget

Zarenor mentioned this pull request Jul 31, 2020

Scroll behavior component #1107

Merged

Make view switcher not update its just created child,

5caefb0

or it doesn't get a WidgetAdded.

jneem requested changes Aug 3, 2020

View reviewed changes

rjwittams mentioned this pull request Aug 5, 2020

Skip update in ViewSwitcher after switching. #1113

Merged

rjwittams added 3 commits August 5, 2020 21:55

Address review comments

7fb2ce5

Merge remote-tracking branch 'remotes/upstream/master'

efe06ce

Rustfmt

3105ae8

jneem requested changes Aug 5, 2020

View reviewed changes

rjwittams closed this Aug 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scrolling changes and small other fixes to support druid_table #1108

Scrolling changes and small other fixes to support druid_table #1108

rjwittams commented Jul 30, 2020 •

edited

Loading

ForLoveOfCats commented Jul 31, 2020

rjwittams commented Jul 31, 2020

jneem left a comment

jneem Aug 3, 2020

rjwittams Aug 3, 2020

jneem Aug 3, 2020

rjwittams Aug 3, 2020

jneem Aug 3, 2020

rjwittams Aug 3, 2020

jneem Aug 3, 2020

rjwittams Aug 3, 2020

rjwittams Aug 5, 2020

jneem Aug 3, 2020

rjwittams Aug 3, 2020

jneem Aug 5, 2020

rjwittams Aug 7, 2020

rjwittams commented Aug 23, 2020

Scrolling changes and small other fixes to support druid_table #1108

Scrolling changes and small other fixes to support druid_table #1108

Conversation

rjwittams commented Jul 30, 2020 • edited Loading

ForLoveOfCats commented Jul 31, 2020

rjwittams commented Jul 31, 2020

jneem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjwittams commented Aug 23, 2020

rjwittams commented Jul 30, 2020 •

edited

Loading