feat: Add Separator to ComboBox #638

qin-yu · 2024-03-14T15:47:35Z

I looked into magicgui and added a very simple mechanism for ComboBox to add separators by introducing a Separator class. Please have a look and tell me if you like it or not. Do I need to do the documentation somewhere or someone is in charge of this?

Many thanks,
Qin

Example

By giving a list ["a", Separator(), "b"] to magicgui for creating a dropdown list, the ComboBox backend go through the list, check if the data is a Separator and insert a separator there using .insertSeparator().

I put a modified version of the example script from your repo:

from magicgui import magicgui
from magicgui.types import Separator

sep = [Separator(i) for i in range(4)]
a = [1, 2, sep[0], 4, sep[1], 6, 7, sep[2], 9, sep[3], 11, 12, sep[0], 14, sep[0]]
b = [1, 2, sep[0], 4, sep[1], 6, 7, sep[2], 9, sep[3], 6, 7, sep[0], 9, sep[0]]


@magicgui(call_button="Test Separator",
          a={"widget_type": "ComboBox", "choices": a},
          b={"widget_type": "ComboBox", "choices": b},
          result_widget=True)
def test_separator(a=a[0], b=b[0]):
    return


test_separator.show(run=True)

a with unique elements:

b with duplicates:

codecov · 2024-03-14T15:50:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.16%. Comparing base (781a54a) to head (86a5967).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #638      +/-   ##
==========================================
+ Coverage   88.90%   89.16%   +0.26%     
==========================================
  Files          39       39              
  Lines        4667     4689      +22     
==========================================
+ Hits         4149     4181      +32     
+ Misses        518      508      -10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This comment is likely to be unnecessary: ```pytb src/magicgui/schema/_guiclass.py:260: error: Unused "type: ignore" comment [unused-ignore] signals = {k: events[k] for k in events} # type: ignore ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Found 1 error in 1 file (checked 1 source file) ```

qin-yu · 2024-03-17T17:14:20Z

Hey @tlambert03, I've completed the necessary changes, and I believe this Pull Request is now ready for your review. Tbh, this is my first time using Codecov, so I would greatly appreciate your feedback on its integration and any adjustments you might recommend. Thank you for your support!

tlambert03

this looks excellent! nice and simple too. Thanks @qin-yu!

I think the only thing that gave me a little pause was the thickness parameter. Since it's not really "thickness" per-se, but rather "number of". Someone could also do Sep(), Sep() if they really wanted multiples, and Qt itself doesn't really have thickness.

Did you add it cause it's something you know you want though?

qin-yu · 2024-03-17T18:17:09Z

Since it's not really "thickness" per-se, but rather "number of". Someone could also do Sep(), Sep() if they really wanted multiples, and Qt itself doesn't really have thickness.

Did you add it cause it's something you know you want though?

Oops, I guess I went a bit too custom with my solution. I was working on this app using Napari, and I noticed that a single separator just wasn’t cutting it in terms of visibility. It took me a while to figure out that changing thickness/padding wasn’t straightforward. So, I ended up with this workaround in my own project.

You’re right about using Sep(), Sep() for multiple separators. If you don't like fake thickness implementation, let's remove it and mention this workaround in docstrings.

Thanks for the heads-up! @tlambert03

tlambert03 · 2024-03-17T18:18:59Z

yeah, i'm not totally opposed to it, but maybe would prefer to consider alternatives before adding it to the public API :) let's just go with a parameter-free sentinel for now.

qin-yu · 2024-03-17T18:32:28Z

Absolutely, I’m with you on maintaining a clean public API. I’ll go ahead and remove the parameter, then submit a new commit a bit later.

Maybe our conversation here could serve as a good reference for future users. Do you think it would be beneficial to include a note in the docstring?

tlambert03 · 2024-03-17T19:18:52Z

Do you think it would be beneficial to include a note in the docstring?

sure, if you'd like, you could indicate that it can be used multiple times to create multiple lines 👍

tlambert03 · 2024-03-17T19:24:27Z

I was working on this app using Napari, and I noticed that a single separator just wasn’t cutting it in terms of visibility. It took me a while to figure out that changing thickness/padding wasn’t straightforward.

by the way, I see this as a problem with napari's style sheets, and something you could take up with napari developers. And not something we should make special considerations for here in magicgui.

The default styling of separators actually looks quite nice, and in-line with the OS standards:

napari applies custom styling to everything, which makes it hard to see:

qin-yu · 2024-03-17T22:53:53Z

by the way, I see this as a problem with napari's style sheets, and something you could take up with napari developers. And not something we should make special considerations for here in magicgui.

Indeed, my experience with magicgui has been primarily within Napari, so my thoughts were anchored in that context 😢

I've updated the code again. Given that the Separator sentinel would remain identical, I've made it a singleton too. I've also adjusted the tests accordingly and updated the docs. I removed a comma to keep the docstring <= 88 limit. Maybe it's the best thing we can do, but it looks okay.

src/magicgui/widgets/_concrete.py

tests/test_widgets.py

tlambert03 · 2024-03-17T23:27:15Z

one more question: can you perhaps look into how this could be implemented on the jupyter widgets backend? by modifying magicgui.backends._ipynb.widgets.ComboBox._mgui_set_choices

src/magicgui/backends/_qtpy/widgets.py

tests/test_widgets.py

qin-yu · 2024-03-18T02:12:55Z

Thanks for the thorough reviews. I've just updated the _qtpy backend based on your feedback. Planning to dive into magicgui.backends._ipynb.widgets.ComboBox._mgui_set_choices first thing tomorrow. Really appreciate your guidance here!

Co-authored-by: Talley Lambert <[email protected]>

qin-yu · 2024-03-18T15:32:01Z

one more question: can you perhaps look into how this could be implemented on the jupyter widgets backend? by modifying magicgui.backends._ipynb.widgets.ComboBox._mgui_set_choices

Regarding ipynb support nuances:

I noticed ipynb allows for duplicate choice names with distinct data. Should we align this with the Qt backend's behavior to ensure consistency across platforms?
Given Jupyter Widgets lacks native support for separators, I propose two approaches:
1. Exclude separators entirely in _mgui_set_choice() to adhere to our principle of not incorporating features unsupported by the backend.
2. Omit separators only within _mgui_set_choices() to parallel the Qt backend behavior. This way, a Separator in Qt equates to a conceptual Separator in ipynb, even though it's not visually represented.

What do you think? @tlambert03

qin-yu · 2024-03-18T15:56:16Z

I just realised that _mgui_set_choice() from ipynb is not used. Didn't fully understand this when you said "by modifying magicgui.backends._ipynb.widgets.ComboBox._mgui_set_choices"

- Fix pyapp-kit#639 - Note that `_mgui_set_choice()` is not used

qin-yu · 2024-03-18T16:48:49Z

I've adjusted magicgui.backends._ipynb.widgets.ComboBox's _mgui_set_choices() method to skip over Separators, given that Jupyter Widgets doesn’t natively support such a concept. I also fixed #639 in the same place, I hope the changes align with your design.

Regarding _mgui_set_choice() (without the ‘s’), it appears this method has no effects, so I've temporarily set it to pass. However, should there be a hidden use case or future need, here's a draft implementation I propose:

def _mgui_set_choice(self, choice_name: str, data: Any) -> None:
    """Set the data associated with a choice."""
    if data is Separator:
        self._ipywidget.options = (*self._ipywidget.options, ("----", Separator))
    else:
        options = self._ipywidget.options
        for item in options:
            print(item, choice_name, data)
            if (isinstance(item, tuple) and item[0] == choice_name) or item == choice_name:
                options = [
                    item for item in options
                    if (not isinstance(item, tuple) or item[0] != choice_name) and item != choice_name
                ]
        self._ipywidget.options = (*options, (choice_name, data))

qin-yu · 2024-03-18T16:49:58Z

(There is one failed test but I don't think it relates to this PR)

Revert the behaviour change of ipynb duplicate items

qin-yu · 2024-03-18T20:04:48Z

qin-yu · 2024-03-18T20:13:37Z

Hey @tlambert03 I encountered an error in the CI workflow Test (3.10, ubuntu-latest, pyside6) / ubuntu-latest py3.10 pyside6, and I'm not sure how to resolve it. Could you take a look and provide some guidance?

qin-yu · 2024-03-19T12:44:35Z

All checks passed. It looks like we're in a good position to wrap up the PR. I'm assuming there's nothing further I need to do on my end if you're planning to handle the squash merge?

tlambert03 · 2024-03-20T04:23:34Z

thank you @qin-yu! busy couple days here. yes I will merge soon after having one more look. thanks again!

tests/test_widgets.py

tlambert03 · 2024-03-20T18:55:31Z

src/magicgui/backends/_qtpy/widgets.py

@@ -908,7 +908,7 @@ def _mgui_bind_change_callback(self, callback):
        self._event_filter.valueChanged.connect(callback)

    def _mgui_get_count(self) -> int:
-        """Return the number of items in the dropdown."""
+        """Return the number of items in the dropdown, including separator items."""


this actually becomes a bit problematic. The protocol specifies that _mgui_get_count should return the number of choices:

magicgui/src/magicgui/widgets/protocols.py

Lines 412 to 415 in 781a54a

@abstractmethod

def _mgui_get_count(self) -> int:

"""Return number of choices."""

raise NotImplementedError

so, it's not really up to the backend here to change those semantics. The ComboBox itself (via CategoricalWidget) declares __len__ to be the number of choices (not the number of widgets)

magicgui/src/magicgui/widgets/bases/_categorical_widget.py

Lines 105 to 107 in 781a54a

def __len__(self) -> int:

"""Return the number of choices."""

return self._widget._mgui_get_count()

so, assert len(combo_a) == len(combo_a.choices) should technically be true, but is not currently so in the tests. So, I'm afraid this needs to account for that as well.

I know these are annoying things, but this feature is a revealing itself to be a bit tricky to implement in a way that isn't potentially surprising and too magical. It is blurring the lines between model (i.e. the values in choices) and the view (how they look in the combobox) a bit.

i'm honestly not sure what the best thing to do here is. one alternative is to go back to what you started with, and just allow for choices to include the separator. that is, when you call widget.choices you do see all the Separator singletons in there (they would need to be subbed back in for None). Or, we just fix the count issue as well and stick with it as it is. That would then mean that it's basically impossible to query where the separators are, and whether they're in there (but you could also set them). Which do you prefer?

qin-yu · 2024-03-21T00:07:35Z

i'm honestly not sure what the best thing to do here is. one alternative is to go back to what you started with, and just allow for choices to include the separator. that is, when you call widget.choices you do see all the Separator singletons in there (they would need to be subbed back in for None). Or, we just fix the count issue as well and stick with it as it is. That would then mean that it's basically impossible to query where the separators are, and whether they're in there (but you could also set them). Which do you prefer?

Indeed, the decision should be guided by the principle of least astonishment: opting for the approach that aligns with most developers' expectations and simplifies common tasks in ComboBox usage.

Although integrating separators as selectable items offers transparency and consistency—a notion shaped by my experiences with Qt, where separators are considered part of the item set—it's essential to acknowledge that separators are typically not viewed as actionable components. Therefore, omitting separators from counts and choices presents a more user-friendly approach, simplifying development by removing the need to distinguish between separators and actual items.

If you know that many of our users choose magicgui to bypass complexities in Qt, opting to conceal separators within the ComboBox would enhance simplicity.

Co-authored-by: Talley Lambert <[email protected]>

qin-yu · 2024-03-21T15:56:06Z

It's pretty straightforward, actually! I just made the necessary adjustments to align with the protocol.

tlambert03 · 2024-03-24T16:00:51Z

src/magicgui/backends/_ipynb/widgets.py

@@ -248,7 +250,7 @@ def _mgui_get_current_choice(self) -> str:

    def _mgui_set_choice(self, choice_name: str, data: Any) -> None:
        """Set the data associated with a choice."""
-        self._ipywidget.options = (*self._ipywidget.options, (choice_name, data))
+        pass


this change looks suspicious. why are we removing the ability to set a single choice? Won't this change mess up calling CategoricalWidget.set_choice?

You're absolutely right. I may have gotten hung up on the protocol's definition of choice_name update. While my commit seemed to function without breaking tests, this change was completely unnecessary. I'm changing it later.

tlambert03

thanks again @qin-yu . I think i'm happy to go with this now. Please just address the remaining comment on the new change to _IPySupportsChoices._mgui_set_choice which i think needs to be reverted

qin-yu · 2024-03-28T23:29:11Z

Hi @tlambert03, just pushed the final change for this PR! Apologies for the delay – I was recently on holiday. It's been a valuable learning experience collaborating with you on this PR. Working on a well-maintained Python package with comprehensive CIs like this is a great way to improve contribution practices. Thanks for your patience and guidance!

tlambert03

it was very pleasant working with you too! Thanks a lot for the PR. hope you'll do it again :)

qin-yu · 2024-05-26T11:53:54Z

Hey Talley, please don't forget to make available v0.8.3 on conda-forge!

tlambert03 · 2024-05-26T12:16:51Z

looks like the release failed: https://github.com/pyapp-kit/magicgui/actions/runs/8476765032 ... sorry about that, will do again

qin-yu · 2024-05-26T13:13:02Z

Thanks! But I think it failed again (actually no test can pass in the last few weeks)

tlambert03 · 2024-05-26T14:05:49Z

Yep I saw thanks, I'll fix when I get the chance. Won't be this weekend

qin-yu · 2024-05-26T14:30:50Z

No worries, thanks for the information. Enjoy the rest of your weekend!

qin-yu mentioned this pull request Mar 14, 2024

ComboBox with sections #637

Closed

qin-yu added 2 commits March 14, 2024 17:30

Add Separator to ComboBox

334c0e5

qin-yu force-pushed the qy/add-separator branch from 4e31f07 to 31395b1 Compare March 14, 2024 16:31

Add Separator tests

23f6753

tlambert03 approved these changes Mar 17, 2024

View reviewed changes

Refactor Separator into singleton sentinel

2116cc8

tlambert03 reviewed Mar 17, 2024

View reviewed changes

src/magicgui/widgets/_concrete.py Outdated Show resolved Hide resolved

tlambert03 reviewed Mar 17, 2024

View reviewed changes

tests/test_widgets.py Outdated Show resolved Hide resolved

tlambert03 reviewed Mar 17, 2024

View reviewed changes

src/magicgui/backends/_qtpy/widgets.py Outdated Show resolved Hide resolved

tests/test_widgets.py Outdated Show resolved Hide resolved

Add Separator: choices exclude separator items

2fe9693

Co-authored-by: Talley Lambert <[email protected]>

qin-yu force-pushed the qy/add-separator branch from a770359 to 2fe9693 Compare March 18, 2024 02:37

Fix duplicate choice names in ipynb Dropdown, add Separator support

2e7b7b3

- Fix pyapp-kit#639 - Note that `_mgui_set_choice()` is not used

qin-yu mentioned this pull request Mar 18, 2024

Dropdown list from ipynb allows duplicates but ComboBox from Qt only keep unique item names #639

Closed

Add Separator: ipynb compatibility

a3b306e

Revert the behaviour change of ipynb duplicate items

tlambert03 reviewed Mar 20, 2024

View reviewed changes

qin-yu and others added 2 commits March 21, 2024 15:44

Add Separator: improve test

51de937

Co-authored-by: Talley Lambert <[email protected]>

Add Separator: hide separator items from users

a6ed534

tlambert03 reviewed Mar 24, 2024

View reviewed changes

Fix ipynb backend _mgui_set_choice()

86a5967

tlambert03 approved these changes Mar 28, 2024

View reviewed changes

tlambert03 merged commit fb2367e into pyapp-kit:main Mar 28, 2024
39 checks passed

tlambert03 changed the title ~~Add Separator to ComboBox~~ feat: Add Separator to ComboBox Mar 29, 2024

tlambert03 added the enhancement New feature or request label Mar 29, 2024

qin-yu deleted the qy/add-separator branch March 29, 2024 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Separator to ComboBox #638

feat: Add Separator to ComboBox #638

qin-yu commented Mar 14, 2024

codecov bot commented Mar 14, 2024 •

edited

Loading

qin-yu commented Mar 17, 2024

tlambert03 left a comment

qin-yu commented Mar 17, 2024

tlambert03 commented Mar 17, 2024

qin-yu commented Mar 17, 2024

tlambert03 commented Mar 17, 2024

tlambert03 commented Mar 17, 2024

qin-yu commented Mar 17, 2024

tlambert03 commented Mar 17, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024 •

edited

Loading

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 19, 2024

tlambert03 commented Mar 20, 2024

tlambert03 Mar 20, 2024

qin-yu commented Mar 21, 2024 •

edited

Loading

qin-yu commented Mar 21, 2024

tlambert03 Mar 24, 2024

qin-yu Mar 28, 2024

tlambert03 left a comment

qin-yu commented Mar 28, 2024

tlambert03 left a comment

qin-yu commented May 26, 2024

tlambert03 commented May 26, 2024

qin-yu commented May 26, 2024

tlambert03 commented May 26, 2024

qin-yu commented May 26, 2024

	@abstractmethod
	def _mgui_get_count(self) -> int:
	"""Return number of choices."""
	raise NotImplementedError

	def __len__(self) -> int:
	"""Return the number of choices."""
	return self._widget._mgui_get_count()

feat: Add Separator to ComboBox #638

feat: Add Separator to ComboBox #638

Conversation

qin-yu commented Mar 14, 2024

Example

codecov bot commented Mar 14, 2024 • edited Loading

Codecov Report

qin-yu commented Mar 17, 2024

tlambert03 left a comment

Choose a reason for hiding this comment

qin-yu commented Mar 17, 2024

tlambert03 commented Mar 17, 2024

qin-yu commented Mar 17, 2024

tlambert03 commented Mar 17, 2024

tlambert03 commented Mar 17, 2024

qin-yu commented Mar 17, 2024

tlambert03 commented Mar 17, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024 • edited Loading

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 18, 2024

qin-yu commented Mar 19, 2024

tlambert03 commented Mar 20, 2024

tlambert03 Mar 20, 2024

Choose a reason for hiding this comment

qin-yu commented Mar 21, 2024 • edited Loading

qin-yu commented Mar 21, 2024

tlambert03 Mar 24, 2024

Choose a reason for hiding this comment

qin-yu Mar 28, 2024

Choose a reason for hiding this comment

tlambert03 left a comment

Choose a reason for hiding this comment

qin-yu commented Mar 28, 2024

tlambert03 left a comment

Choose a reason for hiding this comment

qin-yu commented May 26, 2024

tlambert03 commented May 26, 2024

qin-yu commented May 26, 2024

tlambert03 commented May 26, 2024

qin-yu commented May 26, 2024

codecov bot commented Mar 14, 2024 •

edited

Loading

qin-yu commented Mar 18, 2024 •

edited

Loading

qin-yu commented Mar 21, 2024 •

edited

Loading