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

Widget adaptor for string parsing #346

Merged
merged 1 commit into from
Dec 8, 2019
Merged

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Nov 23, 2019

Useful for e.g. text boxes intended to contain integers.

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 24, 2019

It's tempting to try to derive a Lens-flavored solution to this problem rather than a widget adapter, but the path forward there is less clear to me. A similar impl Lens<T, String> could be defined easily enough, but dropping writes violates the traditional lens laws. Maybe we could start establishing prism-inspired patterns, but I'm not sure what that would look like.

@futurepaul
Copy link
Collaborator

I love this API, seems especially slick for Label, but I'm not sure if it's quite functional enough to be good for numeric textboxes. The feedback loop of parsed data modifying the state of the textbox that's trying to modify the state just doesn't quite work in practice. Perhaps additional functionality here, or additional functionality in textbox, could correct for that?

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 25, 2019

I actually built this for, and am using it with, text boxes, but on closer examination I see that you get a panic if the text box becomes empty. Maybe adapt Option<T> instead of T? Or even Result<T, <T as FromStr>::Error>?

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 25, 2019

One problem with this approach is that it's impossible to input values for which there isn't some edit path from the current value to the desired value where every step is syntactically valid. Instantly rejecting invalid inputs is cute for things like integers, but e.g. inputting 1e10 for a float would be impossible. I think I'll reprise this to preserve invalid values internally.

@cmyr
Copy link
Member

cmyr commented Nov 25, 2019

yea, you beat me to it there. I don't think FromStr is an appropriate tool for this, because of intermediate states. IIRC there's a very good cocoa API for this stuff, let me try and dig it up

@cmyr
Copy link
Member

cmyr commented Nov 25, 2019

So I'm thinking of NSFormatter and NSNumberFormatter. The best overview is probably here. We don't need to do all of this, of course, but it hopefully provides a good overview of the problem.

So I think we want some sort of 'validator' type. There should be two validation steps: during editing, we should check for the validity of a partial string, and then when editing ends we should check for the validity of the final string.

there's lots of subtlety, here. is 1 000 a valid number? is 1,000? is 001? Should we take the current locale into consideration during parsing? etc etc. I think that we can keep a first implementation is simple as possible, but these are things to keep in mind.

@Ralith
Copy link
Collaborator Author

Ralith commented Nov 26, 2019

Wasn't able to reproduce the panic. For posterity:

thread 'main' panicked at 'byte index 2 is out of bounds of `1`', src/libcore/str/mod.rs:2051:9
<snip>
  18: core::str::traits::<impl core::ops::index::Index<I> for str>::index
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libcore/str/mod.rs:1657
  19: unicode_segmentation::grapheme::GraphemeCursor::prev_boundary
             at /home/ralith/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-segmentation-1.6.0/src/grapheme.rs:639
  20: druid::widget::textbox::prev_grapheme
             at /home/ralith/src/druid/druid/src/widget/textbox.rs:489
  21: druid::widget::textbox::TextBox::backspace
             at /home/ralith/src/druid/druid/src/widget/textbox.rs:204

I went ahead and reworked this with the Option concept; the user's input is now only modified on update, never during event, and parse errors yield None.

This is obviously not touching at all on the questions you're raising regarding rich formatting, internationalization, and dynamic user-visible validation. I think those are important things to have, but maybe this very simple approach is still useful in the meantime/for simple cases?

@cmyr
Copy link
Member

cmyr commented Nov 26, 2019

@Ralith can you use this widget in one of the examples, so it's easier to play around with/test?

@Ralith Ralith force-pushed the parsed branch 2 times, most recently from c9dd358 to d927640 Compare December 1, 2019 19:10
@Ralith
Copy link
Collaborator Author

Ralith commented Dec 1, 2019

Done, along with a tweak to fix some poor behavior thereby identified.

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.

Okay, I can see this having some value, although I suspect we will want something more flexible before too long? In particular I think the fact that a parse failure deletes the existing data might be a liability. In any case, not hard to deprecate it down the road.

BaseState, BoxConstraints, Data, Env, Event, EventCtx, LayoutCtx, PaintCtx, UpdateCtx, Widget,
};

/// Converts a `Widget<String>` to a `Widget<Option<T>>`, mapping parse errors to None
Copy link
Member

Choose a reason for hiding this comment

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

I would explicitly document that this works with the FromStr trait, and that T has to be a type that implements FromStr.

@cmyr
Copy link
Member

cmyr commented Dec 7, 2019

@Ralith this looks ready to go?

@Ralith Ralith merged commit 4cdd784 into linebender:master Dec 8, 2019
@Ralith Ralith deleted the parsed branch December 8, 2019 16:27
@Ralith
Copy link
Collaborator Author

Ralith commented Dec 8, 2019

Yeah; looking forward to a better solution, but in lieu of infinite free time there's no sense blocking a simple one.

@cmyr cmyr mentioned this pull request Dec 31, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants