Android DatePicker widget #1408
Conversation
There was a problem hiding this comment.
Thanks for the PR - this is a really good start.
The implementation looks great for what it does; the only nitpick is a couple of places where the implementation isn't consistent with other platforms.
I've updated the demo app to highlight these problems:
- Argument list to the changed_date handler
- More extensive testing of the min/max capability
- Making the example app a little more generic, covering both dates and times (although I've commented out times for now, so it doesn't break the Android case)
| self.picker_impl._dialog = None | ||
| self.picker_impl.set_value(new_value) | ||
| if self.picker_impl.interface.on_change: | ||
| self.picker_impl.interface.on_change(self.picker_impl, new_value) |
There was a problem hiding this comment.
The value shouldn't be passed as part of the handler.
There was a problem hiding this comment.
Oh, alright, that is of corrected now
|
Hi, sure, I will fix those. For some reason I thought that it makes sense to keep passing the value in the handler, but to be fair, it's much better to be consistent. I will implement the TimePicker laster as well. Couple things though, what do you think about keeping the values in DatePicker as instances of Btw, one thing which would also help to highlight the required format for the users is to make the whole codebase typed - what do you think about that? I believe that is another piece of enhancement I could easily tackle. |
👍
I completely agree with this. Using strings as the internal format is a decision oversight in the original patch - it makes a lot more sense for date/time to be the internal format, with string being allowed as an input if possible (parsed using ISO8601 format Y-M-D)
I agree that adding typing would be desirable as a form of implicit documentation. The only exception I can see to this would be when the typing is so broad that formally specifying it becomes more complicated than clarifying. This is something we saw in attempting to add typing to Django - there are some APIs where the types accepted are so broad that the type declaration becomes longer than the implementation of the method. |
|
Hi, So TimePicker and DatePicker of Android are implemented. It is a bit more complex than it maybe needs to be, however, it could be easily extensible to other forms of pickers, such as colorpicker (with a bit of tweaking, granted) |
There was a problem hiding this comment.
The general idea of a base class for "Pickers" is a really good one, but I've got some suggestions for improving the specific implementation here; comments inline.
I've also got concerns about whether the changes to the Toga core layer will break the existing implementations - we'll need to address those before merging. If you don't have access to the platforms that need to be updated, let me know and I can poke around.
examples/date_and_time/README.rst
Outdated
|
|
||
| Test app for the Date Picker Example widget. | ||
|
|
||
| Date Picker is available only on Android and Win platforms |
There was a problem hiding this comment.
This README is now out of date - that's my fault for not corrected it when I renamed the example, but it needs to be corrected before merging.
| @@ -0,0 +1,113 @@ | |||
| from ..libs.android.view import OnClickListener, View__MeasureSpec | |||
| from ..libs.android.widget import EditText | |||
There was a problem hiding this comment.
Pulling these utility classes into a standalone shared module is a good idea; however, convention inside Toga is to put these in a widgets/internal submodule, rather than using a 'protected' module name. See the Cocoa backend for an example of this.
| _value_pack_fn = None | ||
| _extra_dialog_setters = None | ||
| _extra_dialog_args = None | ||
| _dialog_listener_class = None |
There was a problem hiding this comment.
Many of these method stub definitions are effectively reproducing what subclassing is for.
For example, rather than defining _to_obj_converter_class and _value_pack_fn, we could define a method on the PickerBase to_obj() that is given the args, and does the unpacking.
We can then declare a stub implementation in the base class that raises NotImplemented(), which is a more pythonic approach to ensuring subclasses implement all the required methods.
There was a problem hiding this comment.
Made everything as methods (mostly "private" with a single underscore), so this is removed. On top of that, the methods are defined as abstract methods, so they will throw an error if not overridden. I personally am not a big fan of this way of implementing, since most of these just return a single, static value, but here you go :)
|
Hi, so I worked in the requested changes. I've also reverted the changes to toga core - the only other platform in which Pickers are implemented is Windows and I don't have an access to it to test it. My Android implementation is able to live with strings (converting everything behind the scenes to the appropriate objects), so it's not necessary for the changes to the core to be present. |
|
Hi @freakboy3742 , is there anything more needed from me? Thanks! |
There was a problem hiding this comment.
Apologies for the delayed review; I've been busy lately.
This looks great - some aspects are a little over-engineered by trying to be a little bit too abstract, but it's close enough that I'm happy to get it the rest of the way.
While I'm at it, I'll try to restore the "date/time as native format" piece.
Thanks for the contribution - it's much appreciated!
Codecov Report
|
|
Thank you, you pushed it quite far to the finish line! Happy to help, let me know if there is any other issue I can tackle, it was fun :) |
|
@otaj Oh, there's always more issues to tackle :-) On the simple end, any widget missing from the platform compatibility table is up for grabs. Some will be trivial (e.g. Divider); some have partial implementations (e.g., Canvas); and some will require some very deep thought, as well as consideration for what "platform native" means (e.g., Tree). There are also some bugs on widgets in Android that may require a radical re-think of how widgets are implemented. For example, we have a Table widget for Android, but #1392 points out that the implementation won't work if there's a lot of data in the table. Then there are the other platforms to consider. In terms of "bigger picture" issues - #1436 is one of the biggest at the moment, but at the moment, it's at the level of "research project". I don't know what might be a viable solution here - if you want to do some research on what might be possible, that would be incredibly helpful. |
This PR implements DatePicker for Android, as mentioned in #1407 Please go over it whether this is a good direction in which this should be heading. If yes, I will implement TimePicker for Android in a similar fashion.
PR Checklist: