[widget Audit] toga.Selection #1955

freakboy3742 · 2023-05-30T07:55:23Z

Audit of the Selection widget.

Changes on_select to on_change
Modifies selection to use a ListSource internally rather than a static list of data. This solves a bunch of data representation problems (e.g., selecting from a list of integers, and associating data with the selected item). This means the PR includes ported tests and docs for the ListSource as well.
macOS doesn't support any style properties - to get color/font/alignment changes, the underlying Cocoa rendering would need to be completely rewritten.
GTK doesn't support color or alignment style changes. This is another example of an internal CSS node that needs to be targeted, but I can't work out how to successfully target it in a Toga context.
GTK "remembers" its historical size. If you add a long label, the widget increases in size; but if you then remove that label, the widget doesn't shrink again. This also manifests if you increase and then reduce the size of the font.
GTK's focus for Selection is just screwed up. It "works" in the sense that you can give a Selection focus with the keyboard. If you use 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.
Includes a seemingly unrelated change to the cocoa Slider tests. It turns out that the handling of events in the Slider test was dependent on the last GUI event that was processed. Previously, the last event was a keydown; however, with the introduction of the Selection widget, the last GUI event was a mousedown, resulting in a drop in coverage because the "not a mousedown, not a mouseup" branch of logic in the slide hander was never exercised.

Fixes #1471.
Fixes #2013.

Audit checklist

…indow yet.

…heory...)

mhsmith · 2023-06-14T20:39:00Z

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.

docs/reference/api/index.rst

core/src/toga/widgets/selection.py

cocoa/tests_backend/widgets/slider.py

mhsmith · 2023-06-14T18:31:45Z

docs/reference/api/resources/list_source.rst

+ListSource
+==========


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.

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.

docs/reference/data/widgets_by_platform.csv

mhsmith · 2023-06-14T19:06:23Z

docs/reference/api/resources/list_source.rst

+    # Get the first item in the source
+    item = source[0]


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 of MutableSequence either, but we should still keep it because it's implemented by list

append and insert 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 and remove. If this is the intended meaning of "an object whose value is equivalent", then that should be made explicit.

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.

On second thought - it's not as big a change as I originally thought. I've made this change.

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.

mhsmith · 2023-06-14T19:35:30Z

core/src/toga/widgets/selection.py

    @property
    def value(self):
-        """The value of the currently selected item.
+        """The currently selected item.


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.

Agreed it's confusing; I've tried to clarify in the latest draft.

I've also elaborated the example in the usage guide.

docs/reference/api/resources/list_source.rst

mhsmith · 2023-06-14T20:22:29Z

core/src/toga/widgets/selection.py

        """
-        return self._impl.get_selected_item()
+        item = self._impl.get_selected_item()


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?

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.

core/src/toga/widgets/selection.py

cocoa/src/toga_cocoa/widgets/selection.py

mhsmith · 2023-06-14T21:11:51Z

cocoa/src/toga_cocoa/widgets/selection.py

-        if selected:
-            return str(selected)
+    def change(self, item):
+        index = self.interface._items.index(item)


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?

No - index operates on instances, not value equality, so the reference is guaranteed to be to the second (old-B) instance.

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

freakboy3742 · 2023-06-26T02:25:17Z

@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.

freakboy3742 · 2023-06-26T02:27:03Z

testbed/tests/widgets/test_slider.py


    on_change.reset_mock()
    widget.on_change = None
-    probe.change(0.42)
-    on_change.assert_not_called()
+    await probe.change(0.42)


This was the bug - it was causing a warning about an unawaited co-routine

freakboy3742 added 13 commits May 30, 2023 09:18

Document Selection and ListSource.

459b9ef

Update tests for ListSource.

4987f6d

Modify core API to use sources for selections.

3734e83

Cocoa to 100%.

00f5a78

Correct a coverage problem revealed by selection tests.

0a95eb0

iOS to 100%.

f088bae

Add Changenotes.

250e965

Enable future annotations.

5ef461b

Correct a docs typo.

e420203

GTK to 100% (ish...)

00d3daa

Tweaked Cocoa focus implementation when widget hasn't been added to w…

2ca8094

…indow yet.

Add test and fix for resizing on content changes.

5cfddaa

Document edge cases for Selection.

bf91354

freakboy3742 force-pushed the audit-selection branch from 4943695 to bf91354 Compare May 31, 2023 02:40

freakboy3742 added 5 commits May 31, 2023 10:55

Add note about alignment.

d9839dc

Update screenshot.

3c4cde7

Mark GTK test probes as *skips*, because they should be fixable (in t…

144cb75

…heory...)

iOS doesn't resize Selection based on content.

5316d50

Correct docs typos.

7af2279

freakboy3742 mentioned this pull request Jun 8, 2023

SimpleTree widget #1767

Closed

Merge branch 'main' into audit-selection

2c9ab1e

mhsmith reviewed Jun 14, 2023

View reviewed changes

freakboy3742 and others added 6 commits June 15, 2023 07:11

Merge branch 'main' into audit-selection

b15f382

Correct docs markup.

c3abe32

Apply suggestions from code review

8965d30

Co-authored-by: Malcolm Smith <smith@chaquo.com>

Remove cross-platform utils from the widgets-by-platform chart.

aecc472

Cleanup of Source APIs and docs.

60c5abe

Implement change_source with interface-level APIs.

a30d020

freakboy3742 and others added 12 commits June 15, 2023 12:42

Disambiguate index and find methods on ListSource.

c7a9720

Modify selection API to use index rather that item.

4718b9f

Try a better explanation of items and value.

033d465

Add Py3.7 compatibility.

9e09725

Correct API usage in GTK tests.

e8ad994

Correct iOS edge cases with empty lists.

e43b672

Merge branch 'main' into audit-selection

761e9a2

Merge remote-tracking branch 'origin/main' into audit-selection

c56d6d9

# Conflicts: # docs/reference/data/widgets_by_platform.csv

Winforms to 100%

ba6bdbf

Merge branch 'main' into audit-selection

809c22d

Correct GTK implementation and Cocoa tests for recent testbed additions.

62cadea

Minor test cleanups.

186f36b

This was referenced Jun 23, 2023

[widget audit] toga.Table #2011

Merged

Update Documentation (For DetailedList, etc.) #2013

Closed

freakboy3742 and others added 2 commits June 25, 2023 13:28

Restructure background section to provide better organization.

c548a2d

Android to 100%

303ec59

mhsmith force-pushed the audit-selection branch from 3d4a979 to 303ec59 Compare June 25, 2023 23:33

freakboy3742 added 2 commits June 26, 2023 08:24

Merge branch 'main' into audit-selection

2ae2767

Corrected issues with probe.redraw() use in slider tests.

d12a197

freakboy3742 commented Jun 26, 2023

View reviewed changes

mhsmith added 2 commits June 26, 2023 12:26

Fix Windows failure

5997004

Make Selection consistently unfocusable on Android

a8a36fa

mhsmith approved these changes Jun 26, 2023

View reviewed changes

freakboy3742 added 2 commits June 27, 2023 07:17

Add explicit warnings for handler name change.

d1e0b77

Add an explicit note that ListSource is list-like.

a7d7cbf

freakboy3742 merged commit 19b3a67 into beeware:main Jun 26, 2023
43 checks passed

freakboy3742 deleted the audit-selection branch June 26, 2023 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[widget Audit] toga.Selection #1955

[widget Audit] toga.Selection #1955

freakboy3742 commented May 30, 2023 •

edited by mhsmith

mhsmith commented Jun 14, 2023

mhsmith Jun 14, 2023 •

edited

freakboy3742 Jun 14, 2023

mhsmith Jun 14, 2023

freakboy3742 Jun 15, 2023

freakboy3742 Jun 15, 2023

freakboy3742 Jun 15, 2023

mhsmith Jun 14, 2023

freakboy3742 Jun 15, 2023

freakboy3742 Jun 15, 2023

mhsmith Jun 14, 2023 •

edited

freakboy3742 Jun 15, 2023

mhsmith Jun 14, 2023

freakboy3742 Jun 15, 2023

freakboy3742 Jun 15, 2023

freakboy3742 commented Jun 26, 2023

freakboy3742 Jun 26, 2023

		ListSource
		==========

[widget Audit] toga.Selection #1955

[widget Audit] toga.Selection #1955

Conversation

freakboy3742 commented May 30, 2023 • edited by mhsmith

Audit checklist

mhsmith commented Jun 14, 2023

mhsmith Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsmith Jun 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 commented Jun 26, 2023

Choose a reason for hiding this comment

freakboy3742 commented May 30, 2023 •

edited by mhsmith

mhsmith Jun 14, 2023 •

edited

mhsmith Jun 14, 2023 •

edited