feat: Add Separator to ComboBox

[qin-yu]

Hey @tlambert03,

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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[qin-yu]

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!

[qin-yu]

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]

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]

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]

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]

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]

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.

[tlambert03]

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

[qin-yu]

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!

[qin-yu]

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]

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"

[qin-yu]

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]

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

[qin-yu]

[qin-yu]

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]

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]

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

[qin-yu]

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.

[qin-yu]

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

[qin-yu]

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!

[qin-yu]

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

[tlambert03]

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

[qin-yu]

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

[tlambert03]

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

[qin-yu]

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