Tabs widget and the Rotated widget that it uses. #1160

rjwittams · 2020-08-26T21:59:00Z

The tabs widget and dependencies extracted from binding-scroll.

This does static and dynamic tabs, and also includes a Rotated widget that it uses to rotate tab labels.

Things that maybe should change:

The way Rotated messes with the invalidation region is a bit rough and ready.
I didn't want to come up with an elaborate scheme as the best way to do it needs discussion
Axis probably should be promoted out of Flex
CrossAxisAlignment for tabs might be an over-re-use. But not sure.
Possibly the event methods in tabs (when events should be passed on to children) should move onto the Event enum.
Possibly the event transform in Rotated should move there too.

I left both where they were as its probably easier to review

rhzk · 2020-08-27T23:04:48Z

Some things I noticed from the example:

If I click on a new tab I see the content of the previous tab I was on. I.e if I click on "Advanced" I see the content of "Basic" and If I then click on "Page 3" I see the content of "Advanced". But then the button does not work, I guess because I am on "Page 3".

I am not sure that the child widget size should be set to the entire tab space, now I cant have fixed width on a label, and if I add a background color to the label it fills the tab.

I am not a fan of downward text but if we where to have it I dont like to have each letter rotated, I think it would be better in that case for it to be like this:

P
a
g
e

3

otherwise it feels like I should tilt my head to the side to read it

rjwittams · 2020-08-27T23:15:43Z

On rotation: this is fairly standard. E.g IntelliJ does it. I have literally never seen your recommendation outside of motel signs. If you 'don't like' the rotation, don't use it.
I have not seen the behaviour you describe at all wrt tabs. Possibly this is a platform issue? I'll test it again but it sounds very odd.
I don't get why you wouldn't add your own spacing to the child, ie put it in a Flex or whatever. Having dead space invented by the tab widget seems strange to me.

rjwittams · 2020-08-27T23:25:02Z

I can't recreate the behaviour you describe. I hope its fairly obvious I would have noticed that... is this on Windows? I wonder if its a platform event handling difference or something? Really doesn't make a lot of sense. Does the same happen with the dynamic tabs?

rhzk · 2020-08-28T00:29:21Z

The issue is also with the dynamic tabs, I am on windows.

Sure you can put widgets inside a flex but I tought it was odd to overwrite the widgets behavior, ie a textbox filling the entire width of the tab instead of the width you'd expect it to have.

For the suggestion to have downwards tabs without the text being rotated, if you dont want to have that thats fine, but saying "if you dont like it dont use it" is not the wording I would use if somebody asked for a feature.

rjwittams · 2020-08-28T00:37:40Z

You are asking to remove a feature. You literally don't have to use it - leave the tabs not rotated by setting the rotation to None.

I have no idea what you mean by overriding the widgets behaviour. It does exactly the same thing as e.g ViewSwitcher.

If this is a Windows only issue it must be fairly fundamental.

rhzk · 2020-08-28T00:50:24Z

What I ment was this, top one being a textbox in viewswitcher and bottom one in a tab

rjwittams · 2020-08-28T00:50:35Z

On the tab changing, I managed to get something like it to happen on GTK in a linux VM. It might be some difference in the way the animations work - in fact they don't seem to be happening at all... I think it means that it gets stuck at the start of the animation that should be happening - ie with the previous tab.
I'll investigate tomorrow.

rjwittams · 2020-08-28T01:06:14Z

I think its because I do .expand() on the tabs body (which does work like ViewSwitcher internally), this was I think also to make the animations look sensible (using the whole width of the tab). I'll see if there is a better way to do it...

Add in a call to request_paint to make the tabs transition animation work on Linux (and hopefully Windows)

…self, and just draw its child in the origin. Seems to work ok with the animations.

rjwittams · 2020-08-28T12:17:05Z

@rhzk I think both of those issues might be fixed now - no longer use expand, the tab body expands itself (which leaves children alone).
The animation one was a bit odd, I had to add in another call to request_paint. I think it might be that the mac shell ends up painting anyway when animation frames happen and I was accidentally relying on that. So possibly a bug in the mac shell.
I guess the lesson is to always try at least two platforms!
I haven't tested Windows as I don't have it.
I will try out the web again at some point

I also noticed on Linux that the circle with an X on it symbol wasn't in the default font. This was pretty much a placeholder anyway. I may paint it directly or use images - but not sure we have a good answer on shipping around images in the druid library? Probably painting is better. Can do some kind of hover then too

rhzk · 2020-08-29T01:02:48Z

Looks like its all working on Widows now!

rjwittams · 2020-08-29T08:58:09Z

http://chil.rice.edu/research/pdf/byrne2002hfes.pdf <- On rotated text. There is always a creaky old paper to be wheeled out

jneem · 2020-08-30T17:12:16Z

druid/src/widget/rotated.rs

+impl<T, W: Widget<T>> Widget<T> for Rotated<W> {
+    fn event(&mut self, ctx: &mut EventCtx, event: &Event, data: &mut T, env: &Env) {
+        if let Some((transform, inverse)) = self.transforms {
+            ctx.widget_state.invalid.transform_by(inverse);


Every context with a request_paint method can modify the invalid region, so this also needs to be in update and lifecycle. But I think it would actually be nicer (and maybe less fragile) to handle this in WidgetState. That is, replace WidgetState::viewport_offset by, say, WidgetState::viewport_transform,(which would be an Affine), and then handle the transformation of invalid regions during WidgetState::merge_up.

Yes - as I mentioned in the description, I didn't want to do too much with this region stuff before the general idea was thought to make sense.

jneem

I just looked at Rotated so far...

druid/src/widget/rotated.rs

jneem

Very nice! My main complaint, as usual, is that it's missing API docs. I gave up on pointing them out one-by-one, but anyway I think that the CI is now configured to complain about them :)

druid/examples/widget_gallery.rs

druid/src/lib.rs

druid/src/widget/flex.rs

druid/src/widget/tabs.rs

cmyr · 2020-09-08T18:38:29Z

@rjwittams and I discussed this earlier today, and we agreed to split out the rotation work into a subsequent PR.

# Conflicts: # CHANGELOG.md

cmyr

Great, happy to see the ambition here. I have a bunch of questions and comments inline, but overall I think this is close to ready to get in?

Some additional unordered thoughts:

I'd like to see some clarification around the various types whose names begin with Tab; this means some docs, but also some careful consideration around what needs to be public and what does not.
I'm also seeing a bunch of warnings when I play around with the example, which should get looked at; either we're missing some event or the warnings are too aggressive.
I haven't done a careful review of the layout and drawing code; if there are issues in there they'll shake out over time.
I'm concerned about String in places; ideally we would be using LabelText for any text that is shown the user.
we're still missing docs on public items; if these aren't showing up in CI it probably means the items aren't actually being exported, and shouldn't be pub.
I think this is an interesting exploration of some new patterns, which I'm happy about. It's also nice that it is quite compartmentalized, and so we can always revisit in the future without much disruption.

@rjwittams if you'd like to go through the various comments and either reply or mark them as resolved that would be great, and then please ping me and switch the label back to 'ready-for-review' when you think you've addressed everything, and I'll take another pass. thanks!

CHANGELOG.md

druid-shell/src/region.rs

druid/examples/tabs.rs

druid/src/widget/tabs.rs

cmyr · 2020-09-25T16:47:07Z

druid/src/widget/tabs.rs

+///
+pub struct Tabs<TP: TabsPolicy> {
+    axis: Axis,
+    cross: CrossAxisAlignment, // Not sure if this should have another enum. Middle means nothing here


let's talk about this: what is this type used for, exactly?

Which end of the cross axis does your tab bar live on. Should be fairly easy to get from running the example.

ah, gotcha. I might call it something like "tabs_location"? And I agree, CrossAxisAlignment doesn't feel like exactly the right enum for this, I might just shrug and add something like,

enum TabsEdge { Leading, Trailing, }

🤷

Yeah I think thats better

# Conflicts: # CHANGELOG.md

Use that in examples.

cmyr

Okay, I think this is pretty much there. a few more comments are inline, but I don't think anything is blocking, except maybe for using something other than CrossAxisAlignment.

Feel free to address other stuff as you see fit; I'll take one more look when you're done and then I think we're good to go. Thanks!

druid/examples/tabs.rs

cmyr · 2020-09-28T19:55:45Z

druid/examples/tabs.rs

+fn build_root_widget() -> impl Widget<AppState> {
+    fn decor<T: Data>(label: Label<T>) -> SizedBox<T> {
+        label
+            .padding(5.)


The problem is ensuring widgets look correct when used beside one another; this is especially an issue when using a RadioGroup. If you use the default_spacer methods, you will have the same space between widgets as is used in RadioGroup (and other built-in components) wheras if you pick your own padding values you'll end up with items that don't line up correctly.

This isn't currently a practical issue in this example, but given that examples in general tend to be copied by new users, I want to discourage people from using arbitrary padding values, and encourage them to use these other mechanisms that will create more consistent layouts.

I agree about the increased code noise, and don't have a good answer there. Perhaps in the future we will apply auto-padding to items in collection by default or something? But this is where we are for the time being. 😒

druid/src/lib.rs

druid/src/widget/flex.rs

druid/src/widget/tabs.rs

cmyr · 2020-09-28T20:08:30Z

druid/src/widget/tabs.rs

+
+    /// Put the tab bar at the corresponding end of the cross axis.
+    /// Defaults to Start. Note that Middle has the same effect as Start.
+    pub fn with_cross_axis_alignment(mut self, cross: CrossAxisAlignment) -> Self {


echo my earlier comment I would prefer to call this something like with_tab_bar_location or whatever.

cmyr · 2020-09-28T20:10:55Z

druid/src/widget/tabs.rs

+            let mut temp = TabsContent::Swapping;
+            std::mem::swap(&mut self.content, &mut temp);


nit, but I think the idiomatic way to do this would be to use std::mem::replace, something like,

let content = std::mem::replace(&mut self.content, TabsContent::Swapping);

Yep, I think its another case where I didn't know about replace when I wrote this!

rjwittams · 2020-09-28T21:16:04Z

I think I have changed the essential bits.

cmyr

Okay this looks good to me, happy to get this in as-is and we can shake out any issues as they arise. Thank you for your patience!

stale

rjwittams added 3 commits August 26, 2020 22:48

Tab widget and the Rotated widget that it uses.

fe395db

Clippy and CHANGELOG.md fixes for tabs

bfccbfa

Extra level of Clippy fixes...

7967e05

rjwittams added 3 commits August 28, 2020 13:03

Add option to control the tabs transition

86ddd32

Add in a call to request_paint to make the tabs transition animation work on Linux (and hopefully Windows)

Don't expand the tab body, instead make the tab body always expand it…

6ccedde

…self, and just draw its child in the origin. Seems to work ok with the animations.

Rustfmt and Clippy...

ab52e7b

jneem reviewed Aug 30, 2020

View reviewed changes

druid/src/widget/rotated.rs Outdated Show resolved Hide resolved

jneem previously requested changes Sep 2, 2020

View reviewed changes

cmyr added the S-waiting-on-author waits for changes from the submitter label Sep 8, 2020

rjwittams added 3 commits September 9, 2020 22:19

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

819aa27

# Conflicts: # CHANGELOG.md

Fixup AnimationFrame move

5aad9cf

Remove rotation and add docs.

028f1ab

cmyr reviewed Sep 25, 2020

View reviewed changes

jneem mentioned this pull request Sep 27, 2020

Guarantee that WidgetAdded is the first thing a widget sees. #1259

Merged

rjwittams added 3 commits September 28, 2020 15:47

Remove some other extraneous changes.

068c715

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

8246e06

# Conflicts: # CHANGELOG.md

Fix event warnings

a352942

Make the tab labels LabelText inc dynamic.

9826b58

Use that in examples.

rjwittams added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Sep 28, 2020

rjwittams requested a review from cmyr September 28, 2020 17:29

cmyr reviewed Sep 28, 2020

View reviewed changes

cmyr added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels Sep 28, 2020

Add TabsEdge enum. Change layout code in tabs example.

cc29a72

rjwittams added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels Sep 28, 2020

rjwittams requested a review from jneem September 28, 2020 21:15

rjwittams requested a review from cmyr September 28, 2020 21:16

cmyr approved these changes Sep 28, 2020

View reviewed changes

rjwittams merged commit abf28d3 into linebender:master Sep 28, 2020

rjwittams deleted the tabs branch September 28, 2020 21:41

jneem mentioned this pull request May 26, 2021

Transform box #1799

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tabs widget and the Rotated widget that it uses. #1160

Tabs widget and the Rotated widget that it uses. #1160

rjwittams commented Aug 26, 2020

rhzk commented Aug 27, 2020

rjwittams commented Aug 27, 2020

rjwittams commented Aug 27, 2020

rhzk commented Aug 28, 2020

rjwittams commented Aug 28, 2020

rhzk commented Aug 28, 2020

rjwittams commented Aug 28, 2020 •

edited

Loading

rjwittams commented Aug 28, 2020

rjwittams commented Aug 28, 2020

rhzk commented Aug 29, 2020

rjwittams commented Aug 29, 2020

jneem Aug 30, 2020

rjwittams Sep 2, 2020

jneem left a comment

jneem left a comment

cmyr commented Sep 8, 2020

cmyr left a comment

cmyr Sep 25, 2020

rjwittams Sep 26, 2020

cmyr Sep 28, 2020

rjwittams Sep 28, 2020

cmyr left a comment

cmyr Sep 28, 2020

cmyr Sep 28, 2020

rjwittams Sep 28, 2020

cmyr Sep 28, 2020

rjwittams Sep 28, 2020

rjwittams commented Sep 28, 2020

cmyr left a comment

		let mut temp = TabsContent::Swapping;
		std::mem::swap(&mut self.content, &mut temp);

Tabs widget and the Rotated widget that it uses. #1160

Tabs widget and the Rotated widget that it uses. #1160

Conversation

rjwittams commented Aug 26, 2020

rhzk commented Aug 27, 2020

rjwittams commented Aug 27, 2020

rjwittams commented Aug 27, 2020

rhzk commented Aug 28, 2020

rjwittams commented Aug 28, 2020

rhzk commented Aug 28, 2020

rjwittams commented Aug 28, 2020 • edited Loading

rjwittams commented Aug 28, 2020

rjwittams commented Aug 28, 2020

rhzk commented Aug 29, 2020

rjwittams commented Aug 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneem left a comment

Choose a reason for hiding this comment

jneem left a comment

Choose a reason for hiding this comment

cmyr commented Sep 8, 2020

cmyr 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

cmyr 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

rjwittams commented Sep 28, 2020

cmyr left a comment

Choose a reason for hiding this comment

rjwittams commented Aug 28, 2020 •

edited

Loading