-
-
Notifications
You must be signed in to change notification settings - Fork 668
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.Selection #1955
Conversation
4943695
to
bf91354
Compare
I guess dateinput.py, timeinput.py and navigationview.py (x2) were added to this PR accidentally when they became untracked files after switching from another branch. They should be removed. |
ListSource | ||
========== |
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 page overlaps with background/data-sources.rst. Suggest making "Data sources" a category under "API reference", with a general discussion at the top level, and sub-pages for each source type.
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'd completely forgotten that background/data_sources.rst
existed...
However - I disagree with this suggestion. Following the diataxis model, there's two different roles being performed here - task-based (background/how-to) and information based (API).
That said, there's definitely a need to consolidate the two - the API docs in this PR definitely include duplicated detail that should be in the background guide. I'll revise both.
# Get the first item in the source | ||
item = source[0] |
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 implies that ListSource implements__len__
and __getitem__
, which are in fact the only methods mentioned in background/data-sources.rst, but are not explicitly documented here.
In fact, background/data-sources.rst goes further and says that ListSource "supports the data manipulation methods you’d expect of a list
", so I suggest we simply reference collections.abc.MutableSequence
rather than listing all the methods here. And we should note that custom data sources which are read-only only need to implement collections.abc.Sequence
.
However, ListSource does not completely implement either of these interfaces yet, e.g. it's missing __contains__
, __setitem__
and __delitem__
. The easiest way to fix this is probably to make ListSource
inherit from list
.
Notes on other methods:
prepend
is redundant withinsert
: if Python standard lists don't have it, I'm not sure why we should. And the more non-standard methods we add to this interface, the more work it will be to implement a custom source.clear
is not part ofMutableSequence
either, but we should still keep it because it's implemented bylist
append
andinsert
take different arguments to the standard methods of the same name, which only accept a single object. We could match the standard API by allowing that object to be a dict or iterable.- In fact, any dict, iterable or "any other object, which will be mapped onto the first accessor" which would be accepted by the constructor, should also be accepted by
index
andremove
. If this is the intended meaning of "an object whose value is equivalent", then that should be made explicit.
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.
Mostly agreed on all these points; I guess my question is whether we tackle that immediately, or as a follow up "make ListSource a MutableSequence" PR. My inclination is to defer for now, as we can fix Sources independently to the audit of Selection.
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.
On second thought - it's not as big a change as I originally thought. I've made this change.
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.
Closing this out: AFAICT, index()
and remove()
are correct as is - they have to use row instances, because we can't use attribute equality comparisons. If I have a ListSource tracking 2 rows that have the same data value, they have to be treated as 2 distinct rows. On that basis, remove(row)
and index(row)
both need to use the row instance for lookup. find()
exists to help find a row based on data.
@property | ||
def value(self): | ||
"""The value of the currently selected item. | ||
"""The currently selected item. |
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.
It's not clear what types are accepted by the setter. Does it accept any "object whose value is equivalent", as mentioned in my previous comment? This would be useful, because it would allow you to create a ListSource whose first accessor was a unique ID, and whose value
accessor was the visible string, then pass the ID to the setter.
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.
Agreed it's confusing; I've tried to clarify in the latest draft.
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've also elaborated the example in the usage guide.
core/src/toga/widgets/selection.py
Outdated
""" | ||
return self._impl.get_selected_item() | ||
item = self._impl.get_selected_item() |
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.
To reduce duplication between the implementations, it would be simpler if this was just a get_selected_index
method.
More generally, it might be cleaner if instead of exposing the implementation directly to the source, we make the interface the listener, and write the implementation/interface API in terms of strings and indexes rather than items. Then there would be no need for the implementations to call _title_for_item, or even to be aware of interface.items.
I guess the reason for not doing it that way is that the iOS native widget gets the strings on demand, so the implementation would have to either retrieve them from the data source anyway, or cache them in the implementation layer?
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.
Good catch on get_selected_index()
; I didn't notice the duplication, so I've made that change.
As for making the implementation the listener - I definitely see the appeal with that. One complication is iOS, as you've noted; the other is collision with the API on the interface. insert
/remove
/clear
already exist as methods there, controlling the addition of children, so at the very least, we'd need to introduce a proxy class specifically to act as the listener; and for most implementations, all we'd really be abstracting is (a) access to interface.items
, and (b) converting an item to a string.
if selected: | ||
return str(selected) | ||
def change(self, item): | ||
index = self.interface._items.index(item) |
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.
Imagine a list [A, B]. If B changes to become a second A, won't index
return the first copy of A, and so the label of B will not be updated?
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.
No - index operates on instances, not value equality, so the reference is guaranteed to be to the second (old-B) instance.
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 think I understand the source of the confusion here - the docs for index
and find
aren't clear that index is an instance search, and find is an equality search. I'll clarify this.
Co-authored-by: Malcolm Smith <smith@chaquo.com>
# Conflicts: # docs/reference/data/widgets_by_platform.csv
@mhsmith FYI - I've merged with main; I've also corrected a small inconsistency (and related bug) in the slider tests. I've flagged the bug inline; the rest of the changes are to ensure that the probe.redraw() occurs before the assertion of properties that are modified by the test. It doesn't alter the test results in this case, as none of the properties were affected by redraws; but I figured we should be consistent. |
|
||
on_change.reset_mock() | ||
widget.on_change = None | ||
probe.change(0.42) | ||
on_change.assert_not_called() | ||
await probe.change(0.42) |
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 the bug - it was causing a warning about an unawaited co-routine
Audit of the Selection widget.
on_select
toon_change
widget.grab_focus()
, the widget is accessible, but it doesn't highlight in the same way as it does with the keyboard. In either case,widget.has_focus()
returns False. If you try to listen to focus-in and focus-out events, the widget doesn't generate those events. The best explanation I can find is this mailing list post, which suggests the underlying signals are generated by a private widget implementation that isn't visible to the Python bindings.Fixes #1471.
Fixes #2013.
Audit checklist