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

QCheckComboBox for Easy Multiple Items Selection #91

Closed
wants to merge 27 commits into from
Closed

QCheckComboBox for Easy Multiple Items Selection #91

wants to merge 27 commits into from

Conversation

MosGeo
Copy link
Contributor

@MosGeo MosGeo commented Jun 9, 2022

This implements the QCheckComboBox that is discussed in #89. A few notes:

  • addItems and addItem included checked parameters to quickly initializes the items while adding. We can remove that if you feel that it doesn't follow the "zen" of qt.
  • Quite a few convenience functions are added to the widget to retrieve and control the items, e.g., get the indices of all checked items, set checked states of all items.
  • The label of the combobox has two modes: 1) static set by the user or a list of all selected items. The example included showcases this feature.
  • Still getting to know qtbot for actual interface testing. I was planning on using it for _handleItemPressed test but I failed. For now, it is being tested like any other function.

As always, I am open to suggestions and modifications. In terms of functionality, this is what I needed for my use case but others might have other ideas.

p.s. I see that the list of comboboxes in superqt is growing (enum, search) which is great.

@MosGeo MosGeo changed the title QCheckComboBox for Easy Item Selection in ComboBox QCheckComboBox for Easy Multiple Items Selection in ComboBox Jun 9, 2022
@MosGeo MosGeo changed the title QCheckComboBox for Easy Multiple Items Selection in ComboBox QCheckComboBox for Easy Multiple Items Selection Jun 9, 2022
@MosGeo
Copy link
Contributor Author

MosGeo commented Jun 13, 2022

@tlambert03 I think this is ready for your initial review. The example included is a good start.

  • For checkedIndices and uncheckedIndices, I was using model match but it wasn't working in version 6. For now, I did it in a Python for loop instead of using the match function.
  • For macos, I am not sure what is happening. I am not sure if the issue is just in the test_paint_event or the widget actually crashes too. I do not have a macos to test on. Any advise?

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

Thanks for the work @MosGeo! and very sorry for the delay.

to be honest i find this interface really strange 😅 ... I've never seen a dropdown like this where a single click toggles the check-state and a double click "selects/runs" the item... is that what's happening here? (actually, I don't know that I've seen many dropdowns used for multiple selection at all)

I'll ping @goanpeca and @Czaki for second/third opinions. If at least one of them also wants this in main here, then I'd be happy to go for it...

I might also like it a bit more if there wasn't an option for double click (i.e. if it was only a dropdown list of checkable uncheckable items)? though then it becomes a little strange to figure out how to close the thing :) Did you see this implemented in another app you use?

examples/check_combobox.py Outdated Show resolved Hide resolved
@MosGeo
Copy link
Contributor Author

MosGeo commented Jul 4, 2022

@tlambert03 Thanks for checking and no problem on waiting. This is a low priority thing anyway.

Use of Check Combobox

Check Combobox is pretty standard in control libraries nowadays. It goes by many names though (multiple select comobobox, check combobox, ...). Here are some examples for different programming languages:

Some notes:

Double Click Issue

I agree that it is a weird behavior. I didn't catch it while testing as i always close it by clicking outside the box. It is actually a remanent of the original comobox. It shouldn't behave that way. I suggest I remove the behavior and make it do what is expected: double clicking on the item should check/uncheck twice. That is the behavior that is usually implemented in other ones (https://vuetifyjs.com/en/components/combobox/#multiple-combobox)

@MosGeo
Copy link
Contributor Author

MosGeo commented Jul 6, 2022

@tlambert03 I think the behavior is now much better. Double click does not close the pop-up/run the item.

I still have the issue of passing the test with macos. I don't know what to do with that as I do not have a macos!

Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

I see the value of this contribution but I think that it may be improved.

  1. lack of insetItem overwrite https://doc.qt.io/qt-6/qcombobox.html#insertItem
  2. lack of signal informing that selection was changed (selectionUpdated = Signal(int, bool) providing index and value)
  3. lack of documentation in docs/combobox.md

src/superqt/combobox/_check_combobox.py Outdated Show resolved Hide resolved
@goanpeca
Copy link

goanpeca commented Jul 6, 2022

Hi @MosGeo thanks for working on this. To answer @tlambert03 question, yes I have seen this used in other places and I think it is useful now that the double click behavior has been fixed :) 🚀

I agree with @Czaki suggestions also. It would make the implementation complete and much better

Thanks a lot!

@tlambert03 tlambert03 added duplicate This issue or pull request already exists enhancement New feature or request labels Jul 6, 2022
@tlambert03
Copy link
Member

yeah, once you provided the links to the other examples, I get it better 😂

It's kinda the same thing as the labels selector in github:
Screen Shot 2022-07-06 at 11 03 19 AM

I think the primary things that confused me were the double click issue, and what seemed like an inability to "escape" the dropdown easily. (some of the examples you provided have OK/Cancel buttons which I think helped). Another nice thing that most of these "token selector" dropdowns have is some way to show what's currently selected (i.e. tokens with little "x" to remove it). That could come in a later PR, but that's the bit that made me realize what this was really going for.

thanks again @MosGeo

@Czaki
Copy link
Contributor

Czaki commented Jul 6, 2022

@tlambert03 base on your comment maybe we should also add a searchable and checkable combo box also?

And to this PR. There should be also the following method (inspired by currentText):

    def checkedTexts(self) -> List[str]:
        """Returns the checked indices"""
        texts = []
        for i in range(self.count()):
            item = self.model().item(i)
            if item.checkState() == Qt.Checked:
                texts.append(item.text())
        return texts

@tlambert03
Copy link
Member

some typing and namespace suggestions

diff --git a/src/superqt/combobox/_check_combobox.py b/src/superqt/combobox/_check_combobox.py
index a95e3e0..0149846 100644
--- a/src/superqt/combobox/_check_combobox.py
+++ b/src/superqt/combobox/_check_combobox.py
@@ -1,8 +1,8 @@
 from enum import Enum, auto
-from typing import Any, List, Union
+from typing import Any, List, Union, cast
 
-from qtpy.QtCore import QEvent, Qt
-from qtpy.QtGui import QStandardItem
+from qtpy.QtCore import QEvent, QModelIndex, Qt
+from qtpy.QtGui import QStandardItem, QStandardItemModel
 from qtpy.QtWidgets import QComboBox, QStyle, QStyleOptionComboBox, QStylePainter
 
 
@@ -24,10 +24,14 @@ class QCheckComboBox(QComboBox):
     def __init__(self) -> None:
         """Initializes the widget"""
         super().__init__()
-        self.view().pressed.connect(self._handleItemPressed)
-        self.view().doubleClicked.connect(self._handleItemPressed)
+        self.view().pressed.connect(self._handleItemPressed)  # type: ignore
+        self.view().doubleClicked.connect(self._handleItemPressed)  # type: ignore
         self._changed = False
 
+    def model(self) -> QStandardItemModel:
+        # this is true, but annotated incorrectly in pyside2
+        return cast(QStandardItemModel, super().model())
+
     def _update_label_text_with_selected_items(self) -> None:
         checked_indices = self.checkedIndices()
         selected_text_list = []
@@ -53,13 +57,13 @@ class QCheckComboBox(QComboBox):
         """Returns label type"""
         return self._label_type
 
-    def _handleItemPressed(self, index: int) -> None:
+    def _handleItemPressed(self, index: QModelIndex) -> None:
         """Updates item checked status"""
         item = self.model().itemFromIndex(index)
-        if item.checkState() == Qt.Checked:
-            item.setCheckState(Qt.Unchecked)
+        if item.checkState() == Qt.CheckState.Checked:
+            item.setCheckState(Qt.CheckState.Unchecked)
         else:
-            item.setCheckState(Qt.Checked)
+            item.setCheckState(Qt.CheckState.Checked)
 
         if self._label_type == QCheckComboBox.QCheckComboBoxLabelType.SELECTED_ITEMS:
             self._update_label_text_with_selected_items()
@@ -73,7 +77,7 @@ class QCheckComboBox(QComboBox):
         self.setItemChecked(self.count() - 1, checked=checked)
         if (
             self._label_type == QCheckComboBox.QCheckComboBoxLabelType.SELECTED_ITEMS
-            and checked is True
+            and checked
         ):
             self._update_label_text_with_selected_items()
 
@@ -89,7 +93,7 @@ class QCheckComboBox(QComboBox):
 
         if (
             self._label_type == QCheckComboBox.QCheckComboBoxLabelType.SELECTED_ITEMS
-            and any(checked) is True
+            and any(checked)
         ):
             self._update_label_text_with_selected_items()
 
@@ -102,14 +106,15 @@ class QCheckComboBox(QComboBox):
     def itemChecked(self, index: int) -> bool:
         """Returns current checked state as boolean"""
         item: QStandardItem = self.model().item(index, self.modelColumn())
-        is_checked: bool = item.checkState() == Qt.Checked
-        return is_checked
+        return bool(item.checkState() == Qt.CheckState.Checked)
 
     def setItemChecked(self, index: int, checked: bool = True) -> None:
         """Sets the status"""
         item: QStandardItem = self.model().item(index)
         checked_state_old = item.checkState()
-        checked_state_new = Qt.Checked if checked else Qt.Unchecked
+        checked_state_new = (
+            Qt.CheckState.Checked if checked else Qt.CheckState.Unchecked
+        )
 
         # Stopping condition
         if checked_state_old == checked_state_new:
@@ -130,7 +135,7 @@ class QCheckComboBox(QComboBox):
         indecies = []
         for i in range(self.count()):
             item = self.model().item(i)
-            if item.checkState() == Qt.Checked:
+            if item.checkState() == Qt.CheckState.Checked:
                 indecies.append(i)
         return indecies
 
@@ -139,7 +144,7 @@ class QCheckComboBox(QComboBox):
         indecies = []
         for i in range(self.count()):
             item = self.model().item(i)
-            if item.checkState() == Qt.Unchecked:
+            if item.checkState() == Qt.CheckState.Unchecked:
                 indecies.append(i)
         return indecies
 
@@ -149,5 +154,5 @@ class QCheckComboBox(QComboBox):
         opt = QStyleOptionComboBox()
         self.initStyleOption(opt)
         opt.currentText = self._label_text
-        painter.drawComplexControl(QStyle.CC_ComboBox, opt)
-        painter.drawControl(QStyle.CE_ComboBoxLabel, opt)
+        painter.drawComplexControl(QStyle.ComplexControl.CC_ComboBox, opt)
+        painter.drawControl(QStyle.ControlElement.CE_ComboBoxLabel, opt)
diff --git a/tests/test_check_combobox.py b/tests/test_check_combobox.py
index c121cb5..0856ccb 100644
--- a/tests/test_check_combobox.py
+++ b/tests/test_check_combobox.py
@@ -99,7 +99,7 @@ def test_paint_event(qtbot: QtBot) -> None:
     """Simple test for paint event; execute without error"""
     check_combobox = QCheckComboBox()
     check_combobox.setLabelText("A new label")
-    check_combobox.paintEvent(QEvent(QEvent.Paint))
+    check_combobox.paintEvent(QEvent(QEvent.Type.Paint))
 
 
 def test_hidepopup(qtbot: QtBot) -> None:

@tlambert03
Copy link
Member

ugh, sorry, that was ugly! 😂 I was trying a feature in vscode that I've never tried before to suggest edits without having to do the whole suggestion thing. but it was supposed to give you a button to accept it. doesn't look like it though, so I'll just push it

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Patch coverage: 87.28% and project coverage change: -1.48 ⚠️

Comparison is base (6ce87d4) 85.30% compared to head (0fae340) 83.82%.

❗ Current head 0fae340 differs from pull request most recent head 6137bcb. Consider uploading reports for the commit 6137bcb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
- Coverage   85.30%   83.82%   -1.48%     
==========================================
  Files          31       32       +1     
  Lines        2607     2721     +114     
==========================================
+ Hits         2224     2281      +57     
- Misses        383      440      +57     
Impacted Files Coverage Δ
src/superqt/combobox/_check_combobox.py 86.95% <86.95%> (ø)
src/superqt/__init__.py 93.75% <100.00%> (ø)
src/superqt/combobox/__init__.py 100.00% <100.00%> (ø)

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tlambert03
Copy link
Member

tlambert03 commented Jul 7, 2022

ugh... and now I've made a real mess 😂 I pushed my suggestions to main. I'm a little confused, and I think my vscode might also be, because it looks like you made these changes to your main branch. So i hope i didn't mess anything up for you. I'm going to stop trying to suggest/make edits now :)

one observation I had: when you uncheck an item, whatever the last item was gets immediately rechecked the next time you click on the dropdown (note that I'm not trying to re-check item 2 in the movie below)

Untitled.mov

@tlambert03
Copy link
Member

as for the mac error, I just don't think it likes the way you're directly calling paint event. Do you need to do it that way? Can you perhaps call show() on the widget and then use .update() or something?

Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

add widgets to qtbot to prevent qt cleaning releated problems

tests/test_check_combobox.py Outdated Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
tests/test_check_combobox.py Show resolved Hide resolved
@tlambert03
Copy link
Member

@MosGeo, did I scare you off with my botched attempt to make suggestions above? or are you just busy :)

I think this could go in, if we can just figure out what's going on with the reselection of stuff in https://github.com/napari/superqt/pull/91#issuecomment-1178225161?

@MosGeo
Copy link
Contributor Author

MosGeo commented Jul 25, 2022

Sorry, yah, no problem. I was just busy with other stuff. I have to find a day to work on this. All comments here are good.

Yes, I agree that is a big bug that I need to fix. My guess is that this was added when I introduced the fix to the double click. I felt it was a hacky way of doing it and I was not sure why it worked 🤣

I will work on this week and get back to you. I want to check on icon, signal and so on in other comments too.

For documentation, honestly, I didn't know that the md files existed. Is it something of interest now or should we wait for the proper documentation?

@tlambert03
Copy link
Member

For documentation, honestly, I didn't know that the md files existed. Is it something of interest now or should we wait for the proper documentation?

It's fine to add in a later PR. We desperately need real docs 🙃, the md files are completely undiscoverable

MosGeo and others added 2 commits July 26, 2022 06:44
@MosGeo
Copy link
Contributor Author

MosGeo commented Jul 27, 2022

ugh... and now I've made a real mess 😂 I pushed my suggestions to main. I'm a little confused, and I think my vscode might also be, because it looks like you made these changes to your main branch. So i hope i didn't mess anything up for you. I'm going to stop trying to suggest/make edits now :)

one observation I had: when you uncheck an item, whatever the last item was gets immediately rechecked the next time you click on the dropdown (note that I'm not trying to re-check item 2 in the movie below)

Untitled.mov

@tlambert03 I am having issues reproducing. What Qt backend are you using? What OS?

@tlambert03 tlambert03 removed the duplicate This issue or pull request already exists label Oct 30, 2023
@MosGeo MosGeo marked this pull request as draft February 7, 2024 05:01
@MosGeo MosGeo closed this by deleting the head repository Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants