Bugfix: ensure Checkbox state comparisons are correct by using Qt.CheckState(state) #5541

psobolewskiPhD · 2023-02-07T22:16:39Z

Description

This PR fixes #5540
Basically, the existing Checkbox comparisons fail on Qt6 because we compare the returned state, which is 0 or 2, with the enum Qt.CheckState.Checked. This always fails.
Now instead of using state, I make the comparison with Qt.CheckState(state).
See pyside6 docs:
https://doc.qt.io/qtforpython/PySide6/QtCore/Qt.html#PySide6.QtCore.PySide6.QtCore.Qt.CheckState
And here's QT5 docs:
https://doc.qt.io/qt-5/qt.html#CheckState-enum

Note I also found 2 other places in the code where I fixed this, outside of labels layer checkboxes.

Type of change

Bug-fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

References

closes #5540
closes #5556

How has this been tested?

example: the test suite for my feature covers cases x, y, and z
example: all tests pass with my change
example: I check if my changes works with both PySide and PyQt backends
as there are small differences between the two Qt bindings.

Final checklist:

My PR is the minimum possible work for the desired functionality
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
If I included new strings, I have used trans. to make them localizable.
For more information see our translations guide.

codecov · 2023-02-07T22:46:05Z

Codecov Report

Merging #5541 (c899d19) into main (bed44b9) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5541      +/-   ##
==========================================
+ Coverage   89.37%   89.40%   +0.02%     
==========================================
  Files         608      609       +1     
  Lines       51097    51146      +49     
==========================================
+ Hits        45668    45726      +58     
+ Misses       5429     5420       -9

Impacted Files	Coverage Δ
napari/_qt/containers/qt_layer_model.py	`83.72% <100.00%> (ø)`
.../_qt/layer_controls/_tests/test_qt_labels_layer.py	`100.00% <100.00%> (ø)`
.../_qt/layer_controls/_tests/test_qt_shapes_layer.py	`100.00% <100.00%> (ø)`
..._qt/layer_controls/_tests/test_qt_vectors_layer.py	`100.00% <100.00%> (ø)`
napari/_qt/layer_controls/qt_labels_controls.py	`92.07% <100.00%> (+0.37%)`	⬆️
napari/_qt/layer_controls/qt_shapes_controls.py	`88.73% <100.00%> (ø)`
napari/_qt/layer_controls/qt_vectors_controls.py	`72.65% <100.00%> (+2.34%)`	⬆️
napari/_qt/dialogs/qt_package_installer.py	`81.81% <0.00%> (+0.39%)`	⬆️
napari/utils/theme.py	`94.04% <0.00%> (+0.59%)`	⬆️
napari/utils/info.py	`85.10% <0.00%> (+1.06%)`	⬆️
... and 2 more

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

Czaki · 2023-02-07T23:04:44Z

napari/_qt/layer_controls/qt_labels_controls.py

+        self.layer.preserve_labels = (
+            Qt.CheckState(state) == Qt.CheckState.Checked
+        )


Why we use this instead of bool(state)? There is reason why we need to distinguish partially checked state?

That won't work, the problem is on the right hand side.
Qt.CheckState.Checked is an enum so the comparison still fails.
This can work:
self.layer.contiguous = state == int(Qt.CheckState.Checked)
but I'm not sure I like it, because it seems more likely to break again in the future.

Actually no, this self.layer.contiguous = state == int(Qt.CheckState.Checked) doesn't work:
TypeError: int() argument must be a string, a bytes-like object or a real number

Maybe: self.layer.contiguous = state == Qt.CheckState.Checked.value would work?
But still I think I prefer the more explicit comparison. 🤷‍♂️

I also prefer the more explicit comparison (mostly because I don't need to go look up the enum values to understand the code). But I wouldn't oppose the cast to bool suggested because Qt's check state is quite well known - wouldn't self.layer.preserve_labels = bool(state) work?

Czaki

As we can see changed lines are not covered by tests. Could you add tests to prevent these problems in the future?

psobolewskiPhD · 2023-02-08T20:59:06Z

BTW, should the various checkboxes appear in the Class attributes?

napari/napari/_qt/layer_controls/qt_labels_controls.py

Lines 42 to 75 in f47aeed

    
           class QtLabelsControls(QtLayerControls): 
        
               """Qt view and controls for the napari Labels layer. 
        
               Parameters 
        
               ---------- 
        
               layer : napari.layers.Labels 
        
                   An instance of a napari Labels layer. 
        
               Attributes 
        
               ---------- 
        
               button_group : qtpy.QtWidgets.QButtonGroup 
        
                   Button group of labels layer modes: PAN_ZOOM, PICKER, PAINT, ERASE, or 
        
                   FILL. 
        
               colormapUpdate : qtpy.QtWidgets.QPushButton 
        
                   Button to update colormap of label layer. 
        
               contigCheckBox : qtpy.QtWidgets.QCheckBox 
        
                   Checkbox to control if label layer is contiguous. 
        
               fill_button : qtpy.QtWidgets.QtModeRadioButton 
        
                   Button to select FILL mode on Labels layer. 
        
               layer : napari.layers.Labels 
        
                   An instance of a napari Labels layer. 
        
               ndimSpinBox : qtpy.QtWidgets.QSpinBox 
        
                   Spinbox to control the number of editable dimensions of label layer. 
        
               paint_button : qtpy.QtWidgets.QtModeRadioButton 
        
                   Button to select PAINT mode on Labels layer. 
        
               panzoom_button : qtpy.QtWidgets.QtModeRadioButton 
        
                   Button to select PAN_ZOOM mode on Labels layer. 
        
               pick_button : qtpy.QtWidgets.QtModeRadioButton 
        
                   Button to select PICKER mode on Labels layer. 
        
               erase_button : qtpy.QtWidgets.QtModeRadioButton 
        
                   Button to select ERASE mode on Labels layer. 
        
               selectionSpinBox : superqt.QLargeIntSpinBox 
        
                   Widget to select a specific label by its index. 
        
                   N.B. cannot represent labels > 2**53.

I think so? But for Labels just contigCheckBox does, not the other 2...

Czaki · 2023-02-08T21:09:14Z

BTW, should the various checkboxes appear in the Class attributes?

yes

I think so? But for Labels just contigCheckBox does, not the other 2...

One of hole in our review process.

Add tests for the checkbox states The tests pass on main, Qt5, but fail with main, Qt6

napari/_qt/layer_controls/qt_labels_controls.py

andy-sweet

Thanks for following up with further fixes to some of my recent changes @psobolewskiPhD . Looks good to me now, but I'll let @Czaki have the final say.

andy-sweet · 2023-02-09T02:12:58Z

napari/_qt/layer_controls/_tests/test_qt_labels_layer.py

@@ -63,3 +63,22 @@ def test_changing_colormap_updates_colorbox(qtbot):
        color_box.color,
        np.round(np.asarray(layer._selected_color) * 255 * layer.opacity),
    )
+
+
+def test_layercontrols_checkboxes(qtbot):


Optional: slight preference for splitting this up into multiple tests and refactoring the setup code to be shared and maybe a fixture. But this is also good enough for me right now too.

Thanks for this comment! With the help of ChatGPT I learned about fixtures and arguments and implemented this. I hope it's correct though 😅

napari/_qt/layer_controls/_tests/test_qt_labels_layer.py

napari/_qt/layer_controls/_tests/test_qt_shapes_layer.py

@Czaki

with help of ChatGPT. Tweak all tests per @Czaki to make sure the state propagates. Merge branch 'qt6_checkstate_tests' into bugfix/qt6_checkstate

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

napari/_qt/layer_controls/qt_labels_controls.py

napari/_qt/layer_controls/qt_vectors_controls.py

psobolewskiPhD · 2023-02-10T21:02:15Z

I've gone over the docstrings one more time. I think this is good to go now?

psobolewskiPhD · 2023-02-11T19:10:57Z

Update: I missed a CheckedState and it's kinda a biggie, the layer visibility: #5556
Came across it sort of randomly while testing some blending things.
I think it fits in this PR, so I've added what I think is the fix.

@andy-sweet I think ironically the test checks it correctly because of:

napari/napari/_qt/containers/_tests/test_qt_layer_list.py

Lines 47 to 54 in ada343a

    
           def check_state_at_layer_index( 
        
               view: QtLayerList, layer_index: int 
        
           ) -> Qt.CheckState: 
        
               model_index = layer_to_model_index(view, layer_index) 
        
               value = view.model().data(model_index, Qt.ItemDataRole.CheckStateRole) 
        
               # The data method returns integer value of the enum in some cases, so 
        
               # ensure it has the enum type for more explicit assertions. 
        
               return Qt.CheckState(value)

andy-sweet · 2023-02-13T15:49:17Z

Update: I missed a CheckedState and it's kinda a biggie, the layer visibility: #5556 Came across it sort of randomly while testing some blending things. I think it fits in this PR, so I've added what I think is the fix.

@andy-sweet I think ironically the test checks it correctly because of:

napari/napari/_qt/containers/_tests/test_qt_layer_list.py

Lines 47 to 54 in ada343a

def check_state_at_layer_index(

view: QtLayerList, layer_index: int

) -> Qt.CheckState:

model_index = layer_to_model_index(view, layer_index)

value = view.model().data(model_index, Qt.ItemDataRole.CheckStateRole)

# The data method returns integer value of the enum in some cases, so

# ensure it has the enum type for more explicit assertions.

return Qt.CheckState(value)

Ah, I remember doing this cast in the tests, but didn't think enough about it. If you remove it, do all the tests pass now? If so, we should remove it to avoid any regressions later.

psobolewskiPhD · 2023-02-13T18:45:55Z

@andy-sweet no, I meant the other way: tests pass on Qt6 without this PR, because the test uses the CheckState, but the code currently (before this PR) doesn't (just uses value), so the behavior is bugged on Qt6. This PR fixes the code (and behavior) which makes it better match the test.

Edit: Yup, I can confirm: if I change the return of the function used in the test to return value the test passes on Qt5, but fails on Qt6. As is the unmodified test passes on Qt5 an Qt6, which masks the bugged behavior because the actual code uses just value (on main) and not Qt.CheckState(value).

andy-sweet

Thanks for the extra catch.

…ckState(state) (#5541) # Description This PR fixes #5540 Basically, the existing Checkbox comparisons fail on Qt6 because we compare the returned `state`, which is 0 or 2, with the enum Qt.CheckState.Checked. This always fails. Now instead of using `state`, I make the comparison with `Qt.CheckState(state)`. See pyside6 docs: https://doc.qt.io/qtforpython/PySide6/QtCore/Qt.html#PySide6.QtCore.PySide6.QtCore.Qt.CheckState And here's QT5 docs: https://doc.qt.io/qt-5/qt.html#CheckState-enum Note I also found 2 other places in the code where I fixed this, outside of labels layer checkboxes. ## Type of change - [x] Bug-fix (non-breaking change which fixes an issue) # References closes #5540 closes #5556 # How has this been tested? - [x] example: all tests pass with my change --------- Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

fix Qt6 checkstate

69604e4

psobolewskiPhD requested a review from Czaki February 7, 2023 22:16

github-actions bot added the qt Relates to qt label Feb 7, 2023

github-actions bot assigned psobolewskiPhD Feb 7, 2023

Czaki reviewed Feb 7, 2023

View reviewed changes

Czaki requested changes Feb 8, 2023

View reviewed changes

psobolewskiPhD added bugfix PR with bugfix Qt6 labels Feb 8, 2023

psobolewskiPhD added this to the 0.5.0 milestone Feb 8, 2023

psobolewskiPhD added 2 commits February 8, 2023 23:21

Tests for layer control checkboxes

ac98ea2

Merge branch 'qt6_checkstate_tests' into bugfix/qt6_checkstate

c0f815d

Add tests for the checkbox states The tests pass on main, Qt5, but fail with main, Qt6

github-actions bot added the tests Something related to our tests label Feb 8, 2023

andy-sweet reviewed Feb 9, 2023

View reviewed changes

napari/_qt/layer_controls/qt_labels_controls.py Outdated Show resolved Hide resolved

andy-sweet reviewed Feb 9, 2023

View reviewed changes

napari/_qt/layer_controls/qt_labels_controls.py Outdated Show resolved Hide resolved

andy-sweet approved these changes Feb 9, 2023

View reviewed changes

andy-sweet reviewed Feb 9, 2023

View reviewed changes

Merge branch 'main' into bugfix/qt6_checkstate

e15571e

Czaki requested changes Feb 9, 2023

View reviewed changes

napari/_qt/layer_controls/_tests/test_qt_labels_layer.py Show resolved Hide resolved

napari/_qt/layer_controls/_tests/test_qt_shapes_layer.py Show resolved Hide resolved

psobolewskiPhD and others added 5 commits February 9, 2023 20:37

Refactor labels_layer to use a fixture @andy-sweet

0412937

Tweak test per @Czaki

805cfb6

Refactor test_qt_labels_layer.py to use a fixture per @andy-sweet

22f961f

with help of ChatGPT. Tweak all tests per @Czaki to make sure the state propagates. Merge branch 'qt6_checkstate_tests' into bugfix/qt6_checkstate

Update contiguous docstring

d28b81e

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

Fix qt_labels_controls.py docstrings @andy-sweet

a0ee209

Czaki approved these changes Feb 9, 2023

View reviewed changes

fix qt_shapes_controls.py docstrings

dfb6cbc

psobolewskiPhD commented Feb 10, 2023

View reviewed changes

napari/_qt/layer_controls/qt_labels_controls.py Outdated Show resolved Hide resolved

copy-paste fix

992c250

psobolewskiPhD commented Feb 10, 2023

View reviewed changes

napari/_qt/layer_controls/qt_vectors_controls.py Outdated Show resolved Hide resolved

fix vectors layer checkbox docstring

1dab621

psobolewskiPhD added the ready to merge Last chance for comments! Will be merged in ~24h label Feb 10, 2023

psobolewskiPhD mentioned this pull request Feb 11, 2023

Qt6: toggle of layer visibility broken: can click to hide, but can't re-show #5556

Closed

fix visibility state CheckState

0fc3729

psobolewskiPhD removed the ready to merge Last chance for comments! Will be merged in ~24h label Feb 11, 2023

psobolewskiPhD requested a review from andy-sweet February 11, 2023 19:11

psobolewskiPhD mentioned this pull request Feb 13, 2023

Add GUI test coverage for changes to Labels.show_selected_label #5372

Merged

andy-sweet approved these changes Feb 14, 2023

View reviewed changes

psobolewskiPhD added the ready to merge Last chance for comments! Will be merged in ~24h label Feb 14, 2023

Merge branch 'main' into bugfix/qt6_checkstate

c899d19

psobolewskiPhD merged commit 6d27f81 into napari:main Feb 16, 2023

psobolewskiPhD deleted the bugfix/qt6_checkstate branch February 16, 2023 20:51

Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Feb 18, 2023

Czaki mentioned this pull request Jun 7, 2023

v0.4.18 - release process #5911

Closed

Czaki modified the milestones: 0.5.0, 0.4.18 Jun 16, 2023

Czaki added the triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process label Jun 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: ensure Checkbox state comparisons are correct by using Qt.CheckState(state) #5541

Bugfix: ensure Checkbox state comparisons are correct by using Qt.CheckState(state) #5541

psobolewskiPhD commented Feb 7, 2023 •

edited

codecov bot commented Feb 7, 2023 •

edited

Czaki Feb 7, 2023

psobolewskiPhD Feb 8, 2023

psobolewskiPhD Feb 8, 2023

psobolewskiPhD Feb 8, 2023

andy-sweet Feb 9, 2023

Czaki left a comment

psobolewskiPhD commented Feb 8, 2023

Czaki commented Feb 8, 2023

andy-sweet left a comment

andy-sweet Feb 9, 2023

psobolewskiPhD Feb 9, 2023

psobolewskiPhD commented Feb 10, 2023

psobolewskiPhD commented Feb 11, 2023 •

edited

andy-sweet commented Feb 13, 2023

psobolewskiPhD commented Feb 13, 2023 •

edited

andy-sweet left a comment

Bugfix: ensure Checkbox state comparisons are correct by using Qt.CheckState(state) #5541

Bugfix: ensure Checkbox state comparisons are correct by using Qt.CheckState(state) #5541

Conversation

psobolewskiPhD commented Feb 7, 2023 • edited

Description

Type of change

References

How has this been tested?

Final checklist:

codecov bot commented Feb 7, 2023 • edited

Codecov Report

Czaki Feb 7, 2023

Choose a reason for hiding this comment

psobolewskiPhD Feb 8, 2023

Choose a reason for hiding this comment

psobolewskiPhD Feb 8, 2023

Choose a reason for hiding this comment

psobolewskiPhD Feb 8, 2023

Choose a reason for hiding this comment

andy-sweet Feb 9, 2023

Choose a reason for hiding this comment

Czaki left a comment

Choose a reason for hiding this comment

psobolewskiPhD commented Feb 8, 2023

Czaki commented Feb 8, 2023

andy-sweet left a comment

Choose a reason for hiding this comment

andy-sweet Feb 9, 2023

Choose a reason for hiding this comment

psobolewskiPhD Feb 9, 2023

Choose a reason for hiding this comment

psobolewskiPhD commented Feb 10, 2023

psobolewskiPhD commented Feb 11, 2023 • edited

andy-sweet commented Feb 13, 2023

psobolewskiPhD commented Feb 13, 2023 • edited

andy-sweet left a comment

Choose a reason for hiding this comment

psobolewskiPhD commented Feb 7, 2023 •

edited

codecov bot commented Feb 7, 2023 •

edited

psobolewskiPhD commented Feb 11, 2023 •

edited

psobolewskiPhD commented Feb 13, 2023 •

edited