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.Table #2011

Merged
merged 46 commits into from Aug 28, 2023
Merged

[widget audit] toga.Table #2011

merged 46 commits into from Aug 28, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jun 23, 2023

Audit of Table.

Includes an audit of Icon as well, because Icons can be used in tables.

Builds on #1955 because of dependencies on ListSource.

  • Adds the ability to have a table with no header row.
  • Renames on_double_click to on_activate to avoid a specific GUI action.
  • Defaults to an empty string for missing data. This message was causing more confusion than benefit.
  • Deprecates add_column in favor of a full insert/append capability.

Fixes:

Related:

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 - no implementation
  • android backend at 100% coverage - incomplete implementation not worth testing
  • Widget support table updated
  • Widget Issue backlog reviewed

@freakboy3742 freakboy3742 force-pushed the audit-table branch 3 times, most recently from 6eb5d02 to de35b5a Compare June 26, 2023 06:34
@@ -74,8 +76,8 @@ used as a data source. This means they must provide:

* ``__getitem__(self, index)`` returning the item at position ``index`` of the list.

A custom ListSource must also generate ``insert``, ``remove`` and ``clear``
notifications when items are added or removed from the source.
A custom ListSource must also inherit from :any:`Source`, and generate ``insert``,
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 is an interesting case of a subtle duck-typing change. The previous implementation would have allowed any object that honoured the (implicit) Source protocol; as a result of the tweak you made to core, the default behavior is "list like", and a Source must literally inherit from source, and any other type will be coerced (to varying degrees of success) into a ListSource. This then implies that ListSource will be robust to all possible input types, which I don't think is necessarily true.

Copy link
Member

@mhsmith mhsmith Aug 23, 2023

Choose a reason for hiding this comment

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

After discussion, we agreed:

  • The ListSource constructor already accepts any iterable.

  • We need some way for the user to distinguish an iterable which is to be used as a Source directly, versus copied into a new ListSource. Inheriting the Source class is the simplest way of doing this.

  • The statement "Any object that adheres to the collections.abc.MutableSequence protocol can be used as a data source" isn't really true, because:

    • The find method isn't part of the standard Sequence API.
    • There are also differences in whether the other methods use identity versus equality, as discussed in [widget Audit] toga.Selection #1955.

    So this wording should instead refer to the ListSource API directly, and that API documentation should include all the relevant dunder methods. Later we may redefine it as a Protocol.

@mhsmith
Copy link
Member

mhsmith commented Aug 24, 2023

@mhsmith #1811 and #2007 are both windows specific issues.

Confirmed that both of these are now fixed.

core/src/toga/widgets/table.py Outdated Show resolved Hide resolved
Comment on lines +125 to +126
if ctrl:
key_code = "^" + key_code
Copy link
Member

Choose a reason for hiding this comment

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

It turns out the Winforms ListView doesn't have a built-in select-all key combination, but I'll leave this here in case it's useful in the future.

@mhsmith
Copy link
Member

mhsmith commented Aug 27, 2023

Android issues:

@freakboy3742 freakboy3742 merged commit da6e7b3 into beeware:main Aug 28, 2023
41 checks passed
@freakboy3742 freakboy3742 deleted the audit-table branch August 28, 2023 01:49
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