-
-
Notifications
You must be signed in to change notification settings - Fork 738
Added numberinput to web and textual backends. #3299
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good first pass, but a few things fell out in my testing. I've flagged some of them inline; however, the biggest one is value clipping.
It shouldn't be possible for a NumberInput to be storing an invalid value after losing focus. It might have an invalid value while it has focus (because you have to type 1 first in order to type 100, even if the number input has a range of 10-200); but if it loses focus, the value should be clipped to the min/max bound (as appropriate).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The widget screenshots should match the content of the other platforms; if you need an example to screenshot, the screenshots
example app has the sample data. In this case, it should be displaying "2.71818"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - I'm interested how you've generated this screenshot - text inputs should have a blue border around them Textual's default style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was generated in a normal terminal on Ubuntu. I don't remember exactly what I did but I do remember that the blue border came and went semi consistently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how that might be happening because of focus - but you're seeing no border at all when not-focussed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly yes. I did more testing with the textarea with making a multiline textinput but it seemed to behave similarly to the little testing I did with this input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did confirm, when the input is not focused I have no border on any inputs. Only when it is in focus do I have a blue border
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirming I'm seeing the same thing. Looking at the other screenshots, I don't think they have focus; so we should be consistent.
However, I do see a very slightly different color in the background between the "input" and the background:

so we should try to capture that context in the screenshot if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I have the screenshot correct now? I had to pull out a different monitor cause mine were not showing the background color differences for some reason.
self.interface.on_change() | ||
|
||
def on_input_submitted(self, event: TextualInput.Submitted) -> None: | ||
self.interface.on_confirm() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I press Enter when the input has focus, I get an exception because Numberinput doesn't support on_confirm()
return 2 | ||
|
||
def rehint(self): | ||
self.interface.intrinsic.width = at_least(len(self.native.value) + 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This geometry isn't working out right in my testing - it's consistently using a width of 4 characters, which won't display a value.
This is looks like it's an oversight on the TextInput class as well - the size of the widget should be clamped to a "useful minimum" (at_least(10)
), rather than being tied to the width of the content in the widget.
|
||
def dom_keyup(self, event): | ||
if event.key == "Enter": | ||
self.interface.on_confirm() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NumberInput doesn't have an on_confirm()
signal.
…oved unneeded on_confirm handlers
Co-authored-by: Russell Keith-Magee <[email protected]>
I believe the failure of the windows testbed is unrelated? It seemed to fail due to something not touched by this entirely. |
Yes - that's an intermittent failure we see because Github Actions runners seem to have an oddly difficult time remaining connected to the internet... 🤷 I've restarted the test to confirm the clean CI pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely on the right track; I've flagged a couple of issues that came up in my testing.
self.native.onblur = self.lost_focus | ||
|
||
def lost_focus(self, event): | ||
print(self.native.value == "-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like stray debugging code?
print(self.native.value == "-") |
self.native = self._create_native_widget("sl-input") | ||
self.native.type = "number" | ||
self.native.value = None | ||
self.native.onblur = self.lost_focus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a convention of clearly naming native event handlers based on their native naming - so in this case, lost_focus()
should be dom_onblur()
. That makes it easier for us to follow when/why certain methods are being invoked.
def on_input_changed(self, event: TextualInput.Changed) -> None: | ||
self.interface.on_change() | ||
|
||
def on_input_blurred(self, event: TextualInput.Blurred): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blurred definitely looks like the right feature here (side note... why they didn't call this "focus" I don't know, but whatever). However, it looks like it wasn't added until textual 2.0, so we need to bump the minimum Textual version in the pyproject.toml to accomodate this. There's no problem doing a version bump, as long as there's no other consequences - in my very quick testing, Textual 2.0 has some weird "on app close" logic that seemed to get in the way of actually exiting the app; Textual 3.0 works, but outputs:
focus was removed
Unmount() >>> TogaApp(title='Demo NumberInput', classes={'-dark-mode'}, pseudo_classes={'dark', 'focus'}) method=None
to the console on exit, which suggests there's some sort of new exit handling required.
There's also a small code re-use issue here. The clipping logic is almost identical to the clipping logic in the core. It would be preferable to avoid duplicating that logic is possible; it would be preferable to factor out a "_clipped_decimal" utility method in the interface that does all the value normalisation, and then using that method in both the value
setter and the on_input_blurred
implementation for Textual.
I suspect that might also reveal a couple of other cleaning issues (handling of -
-> None
rather than 0; and step clipping).
i have a suggestion for web implementation
i dont have any idea about textual implementation |
I can't 100% rule this out, but I'd be very surprised if the overhead of introducing a base class would actually be worth it - or, if it is worth it, that
I won't speak for @LunaMeadows, but unless you're actually co-located with them, it's going to be difficult to collaborate on a feature like this one. If you're looking for a way to contribute, I'd suggest starting with another "Good First Timer" ticket in Toga's repo. |
I could look into doing this but like said earlier, I don't know if the overhead is really worth it for such a small class. If freakboy3742 has suggestions on how to test/check if the overhead is worth it I can check that out. Not sure really tho the best way to really tell if it is or not.
I appreciate that but I should be good to handle this one. I know there are a lot of outstanding things that are needed to be done and I'm sure freakboy3742 would appreciate assistance on those. Also apologies on the delay on replay/with this PR, I had done some hardware upgrades on my PC which ended up requiring me to reinstall Ubuntu. I have it set up again now tho and will start working on getting this done! |
The acid test will be "how much code is being duplicated". In this case, if there's more than a line or two being duplicated, I'd be shocked - and on that basis, a base class won't really add much.
🎉 No problem at all on the delay - glad you're back in action! I'm planning to poke around the "log messages on quit" issue today; if I find anything, I'll leave a note here. |
The underlying cause of the Textual messages is Textualize/textual#5091; I've logged the Toga issue as #3343, and I have a fix in #3344. |
Added numberinput to web and textual backends. Also updated docs to show that these are now available to use.
Missing widgets for web and textual backends.
PR Checklist: