Skip to content

Tuple optional#1540

Merged
richard-uk1 merged 6 commits intolinebender:masterfrom
richard-uk1:tuple_optional
Feb 9, 2021
Merged

Tuple optional#1540
richard-uk1 merged 6 commits intolinebender:masterfrom
richard-uk1:tuple_optional

Conversation

@richard-uk1
Copy link
Collaborator

@richard-uk1 richard-uk1 commented Jan 17, 2021

This patch adds 2 features. Special care is needed with the Optional widget. While I'm sure it is very useful, I'm not sure I've implemented it correctly.

maan2003
maan2003 reviewed Jan 18, 2021
View reviewed changes
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.

Tuple lens looks good! For Optional, I recommend moving Runebender's Maybe to druid

@cmyr
Copy link
Member

cmyr commented Jan 18, 2021

I have similar doubts about the correctness of my Maybe widget, but I've worked out at least some of the bugs, and it is certainly useful.

@richard-uk1
Copy link
Collaborator Author

richard-uk1 commented Jan 18, 2021 •
edited
Loading

@cmyr could you comment on the difference between this Optional and runebender's Maybe? I didn't include a None option here, since you can get that functionality using Either, but maybe it makes more sense to include it. Also, how to handle events where the event should go to the widget (should_go_to_hidden, or something like that, and lifecycle events), but we don't have any data to send to the widget? I handled it by just sending the default value of the T of Option<T> to the some widget when the data was None, but this seems suboptimal.

EDIT wrote before I saw @cmyr 's comment 😛. I'd still be interested in your opinions on the questions.

@richard-uk1
Copy link
Collaborator Author

richard-uk1 commented Jan 18, 2021

And another question, if we have a widget for the None case, could we include a Null widget that lays itself out as bc.min() and does nothing else? It would go nicely with Maybe/Optional.

@cmyr
Copy link
Member

cmyr commented Jan 18, 2021

in no order:

  • there sort of is a Null widget, in the form of SizedBox::empty.
  • I like letting there be a None option, in case you want a custom placeholder.
  • I don't have any good answer about should_go_to_hidden. I think the solution I would advocate to not have the widget be hidden, but instead to create and destroy it as needed.

@richard-uk1
Copy link
Collaborator Author

richard-uk1 commented Jan 21, 2021

About the SizedBox::empty - it looks to be equivalent to a theoretical Null widget. My question is: how do we make this use of SizedBox discoverable?

@cmyr
Copy link
Member

cmyr commented Jan 21, 2021

I'm open to suggestions!

@richard-uk1
Copy link
Collaborator Author

richard-uk1 commented Jan 21, 2021

@cmyr I've swapped out the Optional for runebender's Maybe, and added some intra-crate doc links.

@richard-uk1
Copy link
Collaborator Author

richard-uk1 commented Jan 22, 2021

Ready for review :)

maan2003
maan2003 reviewed Jan 23, 2021
View reviewed changes
rjwittams
rjwittams reviewed Jan 24, 2021
View reviewed changes
@rjwittams rjwittams mentioned this pull request Jan 24, 2021
Merged
@cmyr
Copy link
Member

cmyr commented Jan 25, 2021

Do we need both Maybe and Optional? In my use, Maybe covers the optional category well enough.

@richard-uk1
Copy link
Collaborator Author

richard-uk1 commented Jan 27, 2021

Do we need both Maybe and Optional? In my use, Maybe covers the optional category well enough.

Oh I thought I had removed Optional. Will do.

@richard-uk1
Add 2 features
1efb4e8 
 - A `Lens` that combines 2 lenses into a tuple
 - A widget that displays an `Option`, showing nothing if there isn't
   anything to show.
@richard-uk1
Make sure lifecycle methods are always forwarded.
f218e0a
@richard-uk1
Use Maybe from runebender rather than Optional.
a90d9ef
@richard-uk1
Fix doc link
ab0fc51
@richard-uk1
Change wording slightly.
102e975
@richard-uk1
Address feedback
4d674f7 
 - Change `Tuple` to `Tuple2`.
 - Put a doc alias of "null" on `SizedBox::empty`.
 - Remove `Optional` which had already been removed from widget `mod`.
@richard-uk1
Copy link
Collaborator Author

richard-uk1 commented Jan 27, 2021

Ready for review again.

cmyr
cmyr approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

I won't make strong guarantees about covering all the corner cases in Maybe, but I haven't seen a crash in runebender in a while? 😬

@richard-uk1 richard-uk1 merged commit 64689fd into linebender:master Feb 9, 2021
@richard-uk1 richard-uk1 deleted the tuple_optional branch February 9, 2021 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rjwittams rjwittams rjwittams left review comments

@maan2003 maan2003 maan2003 left review comments

@cmyr cmyr cmyr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@richard-uk1 @cmyr @rjwittams @maan2003