Add Formatter trait for validating & formatting text#1377
Conversation
|
I feel that I also dont think that |
67cd98c to
feb602c
Compare
feb602c to
e63b6fd
Compare
JAicewizard
left a comment
There was a problem hiding this comment.
My previous comment still stands.
In addition to that I think it might be useful to have some easy "undo", but that might be a validation error?
| /// | ||
| /// [`Formatter`]: Formatter | ||
| pub struct Validation { | ||
| result: Result<(), ValidationError>, |
There was a problem hiding this comment.
Make this an option? Seems more appropriate since this is an optional variable and not directly the result of a function.
Would also be nice if it had just a little documentation on what this does.
There was a problem hiding this comment.
This whole Validation struct is sort of an augmented result; it includes not only the Result itself, but also additional information that exists both in the success and failure cases. It would definitely be possible to store this as an Option, but I think keeping it as a Result more clearly reflects the semantics of the program.
| } | ||
|
|
||
| /// Create a `Validation` with an error indicating the failure reason. | ||
| pub fn failure(err: impl std::error::Error + 'static) -> Self { |
There was a problem hiding this comment.
Would also be nice if it had just a little documentation on what this does. Does this result in the edit being undone? Does something happen to the selection? Does the user see an error indicating they typed something wrong? (probably not the later 2 but it should be clear)
There was a problem hiding this comment.
This is a function that creates a Validation object with a given inner error. In the context of text editing, how the textbox reacts when validation fails is dependent on how that text box is configured.
|
I forgot to put this in, but I would also change the name of the example, then we keep the 'validation' name open for a more simplified example. |
cmyr
left a comment
There was a problem hiding this comment.
I feel that
format_for_editingmight be a bit useless of a method. If someone really wanted to have a different formatting for editing they could just as well use some kind of internal state for that. It might also be that the user wants to format the same value differently for 2 labels even if both are not editable.
format_for_editing: I can see how this method might seem a bit superfluous, but I think it will be used in common cases; for instance it is common to edit a currency value, but to not want to let the user edit the currency symbol, and I can't think of a better way of doing this. If I'm totally honest, this is pretty much directly a copy of the API of the Cocoa Formatter class, and I sort of trust that they thought this through and decided this was the best option.
Different formats for same value: this is the beauty of our approach here; you can format the same value in different ways by providing different Formatter instances.
I also dont think that Formatter is the right name, it does way more then just formatting. Parser sounds more accurate to me but that might clash with .parse.
This is a challenge; the Formatter does both formatting and validation. You can think of it as a type that handles the conversion between a value and a string; I think that Parser suffers a similar problem in that it only captures half of the job. You could imagine other names like Stringify or something, but that's also pretty incomplete. 😒
| /// | ||
| /// [`Formatter`]: Formatter | ||
| pub struct Validation { | ||
| result: Result<(), ValidationError>, |
There was a problem hiding this comment.
This whole Validation struct is sort of an augmented result; it includes not only the Result itself, but also additional information that exists both in the success and failure cases. It would definitely be possible to store this as an Option, but I think keeping it as a Result more clearly reflects the semantics of the program.
| } | ||
|
|
||
| /// Create a `Validation` with an error indicating the failure reason. | ||
| pub fn failure(err: impl std::error::Error + 'static) -> Self { |
There was a problem hiding this comment.
This is a function that creates a Validation object with a given inner error. In the context of text editing, how the textbox reacts when validation fails is dependent on how that text box is configured.
This is a mechanism for handling conversions back and forth between values and their textual representations. Its major use case is managing user input in text fields when the input is intended to represent some other value type. This patch includes the trait, a simple implementation based on `FromStr` (intended as a replacement for the `Parse` widget) and a new ValueTextBox widget that uses some Formatter to manage a TextBox. The behaviour of the ValueTextBox differs from the normal TextBox in one very important way; it does not attempt to update the data until editing has 'completed'. Editing is completed when the user presses <return> or attempts to navigate away from the textbox; the user can also manually cancel editing by pressing <esc>, which discards the edit. These interactions (especially the hard-wired key handling) feels a bit brittle, and I'd like to try and find a more systematic approach. This is still a draft; there are a number of rough edges, and other things I'm not totally happy with, including: - it uses String everywhere; I might prefer Arc<String> at least - there is no good mechanism for actually reporting errors back up the tree - the error type itself is a bit too fancy, and should be simplified - I'd like to add a general purpose NumberFormatter - I'd like to look into incorporating this into the existing TextBox, including the idea of 'on_completion' and 'on_cancel' handling. In any case, I'm happy with the progress here; at the very least this is a much more robust and flexible alternative to the current Parse widget. I have updated `examples/parse.rs` to show off some of what is possible with this API; that example should probably be renamed.
This had gotten well over 700 lines, and I think it makes sense to start trying to split examples like this up.
This resolves a bunch of fixmes and loose ends
It almost seems like these are 2 completely separate concepts, one if of displaying text, like the This also sparked the question in me: are custom lenses still useful, besides the derived ones? but thats something for zulip
If the trait would be split like suggested above, maybe |
|
The thing is, they do need to work together; in the case of a text editing box, you format the text that populates the box, and then you validate the input as it occurs, so I don't think it makes a lot of sense to separate them :( Custom lenses are very useful; they're used heavily in runebender. |
e63b6fd to
545b078
Compare
|
I mean split them in editable formatting + validation and normal formatting, not splitting them editable formatting + normal formatting and validating. |
545b078 to
1af9491
Compare
|
I agree that you shouldn't need to worry about the editing stuff if you don't need it; that's why there's a default impl of It's also worth thinking of this as a very low-level trait, that ultimately you shouldn't need to implement manually much, because we should provide default formatters for common types of values that handle things like localization. One way to think about this |
|
ah ok then I misunderstood the goal I think |
|
okay, going to merge this, can shake out problems as they arise. |
This is a mechanism for handling conversions back and forth
between values and their textual representations. Its major
use case is managing user input in text fields when the input
is intended to represent some other value type.
This patch includes the trait, a simple implementation based
on
FromStr(intended as a replacement for theParsewidget)and a new ValueTextBox widget that uses some Formatter to manage
a TextBox.
The behaviour of the ValueTextBox differs from the normal TextBox
in one very important way; it does not attempt to update the data
until editing has 'completed'. Editing is completed when the user
presses return or attempts to navigate away from the textbox;
the user can also manually cancel editing by pressing esc, which
discards the edit.
These interactions (especially the hard-wired key handling) feels
a bit brittle, and I'd like to try and find a more systematic
approach.
This is very strongly inspired by Cocoa's
Formatterclass.update:
This patch led me down a rabbit hole, because it encounters a bunch of situations that are hard to deal with given druid's current data model. In this particular case, this is related to error handling; the solution here is not satisfactory, (it involves setting a callback on the textbox and then manually sending commands as errors occur) but it makes error handling at least possible, until we're able to come up with a more robust, longer-term solution.
I intend to write up the problems i encountered and how I'm thinking about them in a subsequent document.
This now replaces the
parseexample with an example package,validation, which can be run with,progress on #1140