Numeric input does not allow non-numeric value #10457
Conversation
farmaazon
requested review from
Frizi,
vitvakatu,
kazcw and
AdRiley
as code owners
app/gui2/shared/ast/tree.ts
Outdated
| @@ -1825,6 +1826,10 @@ export class MutableNumericLiteral extends NumericLiteral implements MutableAst | |||
| export interface MutableNumericLiteral extends NumericLiteral, MutableAst {} | |||
| applyMixins(MutableNumericLiteral, [MutableAst]) | |||
|
|
|||
| export function isNumericLiteral(s: string) { | |||
| return is_numeric_literal(s.trim()) | |||
There was a problem hiding this comment.
Not sure about trim here. Submitting code expresisons with trailing spaces can cause issues, so effectively ignoring them in this method seems bad. If you want to trim the inputted code, do it before calling this method.
| props.onUpdate({ | ||
| portUpdate: { value: value.toString(), origin: props.input.portId }, | ||
| portUpdate: { value: value ?? '', origin: props.input.portId }, |
There was a problem hiding this comment.
undefined probably should be propagated as such, as it correctly signals a removal of an argument. Using empty string here can introduce issues by retaining unnecessary extra spaces.
There was a problem hiding this comment.
Two general notes:
- Should we give the user some kind of feedback before/after discarding their invalid input?
- I think committing a previous value is odd; for example, if a user types a number, selects it, pastes something invalid (replacing the previous number), and commits, then we would commit a value that the user never attempted to commit and that has no relation to the value the user attempted to commit.
| // representations. | ||
| const lastValidValue = ref<string>() | ||
| watch(editedValue, (newValue, oldValue) => { | ||
| if (newValue != oldValue) { |
There was a problem hiding this comment.
The watcher will not be called if this condition is false.
| @@ -91,13 +102,14 @@ const inputStyle = computed<CSSProperties>(() => { | |||
| }) | |||
|
|
|||
| function emitUpdate() { | |||
| if (valueString.value !== editedValue.value) { | |||
| emit('update:modelValue', editedValue.value) | |||
| if (props.modelValue !== lastValidValue.value) { | |||
There was a problem hiding this comment.
A number and a string will never be strictly equal.
| const lastValidValue = ref<string>() | ||
| watch(editedValue, (newValue, oldValue) => { | ||
| if (newValue != oldValue) { | ||
| if (newValue == '' || is_numeric_literal(newValue)) { |
There was a problem hiding this comment.
I think it would be nice to re-export is_numeric_literal from a higher level module with our JS naming convention. Whether it is an FFI call is an implementation detail.
I may refer to the previously committed value. I just wanted to keep user's input in case of trivial typos, like typing
The reverting value is sufficient feedback after IMO. As for feedback before, I feel it's a bit hard to implement in every case, because we commit on defocus, and we may be defocused with the entire window (so it would be impossible to give focus back). The only way I could think is make some highlight, but I'm not sure how to make it looking nice. |
Pull Request Description
Fixes #9838 (see the "backlog refinement" comment there).
When someone want's to accept invalid input, it gets reverted to last recorded valid value.
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
[ ] Unit tests have been written where possible.