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.DateInput, toga.TimeInput #1951

Merged
merged 28 commits into from Jun 20, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented May 26, 2023

Audit of the DateInput and TimeInput widgets.

Renames the widgets from Date/TimePicker to Date/TimeInput for consistency. The widget was previously undocumented, and is only currently implemented for Android and Windows, so the impact of this change should be minimal. A compatibility shim for the old name has been included.

Fixes #1962.

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 (not implemented)
  • winforms backend at 100% coverage
  • gtk backend at 100% coverage (not implemented)
  • iOS backend at 100% coverage (not implemented)
  • android backend at 100% coverage
  • Widget support table updated
  • Widget Issue backlog reviewed

@mhsmith
Copy link
Member

mhsmith commented Jun 5, 2023

Renames the widgets from Date/TimePicker to Date/TimeInput for consistency. The widget was previously undocumented, and is only currently implemented for Android and Windows, so the impact of this change should be minimal. A compatibility shim for the old name has been included.

For consistency with NumberInput, I've also renamed min_date and max_date to min_value and max_value, and similarly with the TimeInput. At some point we might want to do the same thing with the Slider, which is an outlier because it uses a combined range rather than separate min and max.

@freakboy3742
Copy link
Member Author

For consistency with NumberInput, I've also renamed min_date and max_date to min_value and max_value, and similarly with the TimeInput.

Good catch - 100% agreed.

At some point we might want to do the same thing with the Slider, which is an outlier because it uses a combined range rather than separate min and max.

Also agreed.

It occurs to me that we should start logging specific tickets for the resolveable issues that we're finding as a result of this audit (e.g., styling of Selection on GTK, ActivityIndicator not implemented on Informs, etc). Prior to the audit, there wasn't a whole lot of value to having tickets for all the micro issues and missing platform features; now that we have coverage testing, we have a specific list of things that are missing, the platforms where they're missing, and a known contract that they should be adhering to. This sort of API inconsistency fits that pattern.

@mhsmith mhsmith marked this pull request as draft June 6, 2023 21:29
@mhsmith
Copy link
Member

mhsmith commented Jun 6, 2023

@freakboy3742: The Android testbed is hanging intermittently, so this isn't quite ready to merge, but it would be useful for you to review it now.

Copy link
Member Author

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Periodic freezing notwithstanding, this looks pretty good; I've flagged a couple of things we should probably raise in the user-facing notes for the widgets, plus one possible approach for a "soft" min/max on Android TimeInput.

@@ -7,78 +7,67 @@
)
from .internal.pickers import PickerBase

NO_MIN = date(1799, 1, 1)
NO_MAX = date(9999, 1, 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Are these values with a significance to Android? We should document why these dates in particular has been picked - both in the code, and in the user-facing widget notes as a functional limitation.

Copy link
Member

Choose a reason for hiding this comment

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

As discussed, I've changed the implementation to allow years between 1800 and 8999 on all platforms, and updated the documentation to match.

android/src/toga_android/widgets/label.py Outdated Show resolved Hide resolved

# Unlike DatePicker, TimePicker does not natively support min or max, so these
# properties currently have no effect.
Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be worth doing a "soft" min/max? i.e., we have control over get_value(); if there's a min_value of 6AM, and the user picks 3AM, we could modify get_value() to return 6AM. It's not ideal UX, but it would at ensure the Android implementation doesn't return values that aren't invalid.

from .base import Widget

NO_MIN = WinForms.DateTimePicker.MinDateTime # Year 1753
NO_MAX = WinForms.DateTimePicker.MaxDateTime # Year 9998
Copy link
Member Author

Choose a reason for hiding this comment

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

Worth documenting these limits in the user-facing notes.

Copy link
Member

Choose a reason for hiding this comment

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

See above.

@mhsmith mhsmith mentioned this pull request Jun 15, 2023
11 tasks
@mhsmith
Copy link
Member

mhsmith commented Jun 19, 2023

I've changed the min and max properties as follows:

  • They always return an actual date or time rather than None.

  • Consider changing the min/max from 10/20 to 30/40. Previously, to avoid a ValueError, you would need to update the max before the min. But when going from 30/40 to 10/20, you would have to set the min before the max. This would make it inconvenient to write code that changed the limits between arbitrary values.

    So instead of throwing a ValueError, I've changed it so the min can now clip the max and vice versa, with the property currently being set taking priority.

@mhsmith mhsmith mentioned this pull request Jun 19, 2023
@mhsmith mhsmith marked this pull request as ready for review June 20, 2023 17:19
Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

Ready to merge.

Copy link
Member Author

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍

@freakboy3742 freakboy3742 merged commit a10ef30 into beeware:main Jun 20, 2023
43 checks passed
@freakboy3742 freakboy3742 deleted the audit-datetime branch June 20, 2023 23:46
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.

avc: denied { ioctl } messages in Android testbed
2 participants