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 Audit] toga.NumberInput #1946

Merged
merged 31 commits into from
Jun 2, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented May 22, 2023

Audit of the Numberinput widget.

Builds on #1944.

Notable implementation details:

  • Broadly, desktop platforms provided an up/down stepper; mobile platforms provide a numeric keypad.
  • The min/max/step attributes are mostly managed on the interface side. This allows us to use Decimal as a representation (ensuring that we can round trip values); it also simplifies some of the implementation, as the widget becomes "text that can be interpreted numerically", with the interface performing final validation of that text. This is helpful because the number input needs to be in an invalid state a lot of the time (e.g., values that are outside min/max range)

Fixes #1455
Fixes #1234

Audit checklist

  • Core tests ported to pytest
  • Core tests at 100% coverage
  • Core docstrings complete, and in Sphinx format
  • Documentation complete and consistently formatted
  • cocoa backend at 100% coverage
  • winforms backend at 100% coverage
  • gtk backend at 100% coverage
  • iOS backend at 100% coverage
  • android backend at 100% coverage
  • Widget support table updated
  • Widget Issue backlog reviewed

@freakboy3742
Copy link
Member Author

@mhsmith This has now been rebased against the changes from #1944.

@freakboy3742 freakboy3742 marked this pull request as ready for review May 30, 2023 00:33
docs/reference/api/widgets/numberinput.rst Outdated Show resolved Hide resolved
docs/reference/api/widgets/numberinput.rst Outdated Show resolved Hide resolved
core/src/toga/widgets/numberinput.py Outdated Show resolved Hide resolved
core/src/toga/widgets/numberinput.py Outdated Show resolved Hide resolved
core/src/toga/widgets/numberinput.py Outdated Show resolved Hide resolved
core/src/toga/widgets/numberinput.py Outdated Show resolved Hide resolved
core/src/toga/widgets/numberinput.py Show resolved Hide resolved
core/src/toga/widgets/numberinput.py Outdated Show resolved Hide resolved
core/src/toga/widgets/numberinput.py Outdated Show resolved Hide resolved
cocoa/src/toga_cocoa/widgets/numberinput.py Outdated Show resolved Hide resolved
@mhsmith
Copy link
Member

mhsmith commented May 30, 2023

It doesn't look like #1455 has been fixed on Cocoa. After clicking Clear followed by the decrement button:

Screenshot 2023-05-30 at 21 15 25

@freakboy3742
Copy link
Member Author

It doesn't look like #1455 has been fixed on Cocoa. After clicking Clear followed by the decrement button:

Fixed (Again).

# ROUND_DOWN would not be unreasonable for manually-entered strings, but it
# interacts badly with native increment operations that use floats. For example,
# 1.2 might be incremented to 1.299999997, and then rounded back down to 1.2,
# so nothing would change.
Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - but I guess my question is why HALF_UP rather than HALF_DOWN? I guess it doesn't really matter, as it's literally only the 1.500000000 case that matters - but HALF_DOWN would seem more consistent with the ROUND_DOWN in that case.

Copy link
Member

Choose a reason for hiding this comment

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

This only makes a difference when the app user manually enters a number that ends with a 5, so we should do whatever will confuse the smallest number of those users, who are likely not to have any advanced mathematics education.

ROUND_HALF_DOWN is not commonly used by anyone, so the real choice is between ROUND_HALF_UP (which I was taught in school), and ROUND_HALF_EVEN (which @freakboy3742 was taught in school).

ROUND_HALF_EVEN is used by the Python round function, and in most scientific contexts.

ROUND_HALF_UP is used by spreadsheets, and, based on my searches, in the majority of basic mathematics education, at least in English. For example, see "as taught in school" in the AppleScript documentation.

So I think we should go with ROUND_HALF_UP.

Copy link
Member Author

Choose a reason for hiding this comment

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

That reasoning all makes sense to me. (also - lol at AppleScript having an actual "as taught in school" keyword....)

self.interface.on_change(None)
validate = getattr(self.interface, "_validate", None)
if validate:
validate()
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like it should be managed by subclassing, rather than probing attribute access.

@freakboy3742 freakboy3742 merged commit 0c527bb into beeware:main Jun 2, 2023
@freakboy3742 freakboy3742 deleted the audit-numberinput branch June 2, 2023 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants