Unify event behaviour for points and its qt controls #5722

brisvag · 2023-04-13T15:24:13Z

Fixes/Closes

Closes #5718.

Description

Point controls were not firing events as intended. Most of the event blockers were either too aggressive (block everything, rather than just the relevant callback) or inverted.

Additionally, there was a lot of asymmetry and confusion between how various elements worked. I clarified the gui labels to make it clearer when the current_* property is being affected. I also added a few events to match expectations with other parts of the Points layer.

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

codecov · 2023-04-13T15:50:05Z

Codecov Report

Merging #5722 (856f43e) into main (a79537c) will increase coverage by 0.01%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##             main    #5722      +/-   ##
==========================================
+ Coverage   89.85%   89.87%   +0.01%     
==========================================
  Files         608      608              
  Lines       51756    51796      +40     
==========================================
+ Hits        46504    46550      +46     
+ Misses       5252     5246       -6

Impacted Files	Coverage Δ
napari/_qt/layer_controls/qt_points_controls.py	`90.00% <66.66%> (-2.91%)`	⬇️
napari/layers/points/points.py	`98.85% <100.00%> (+<0.01%)`	⬆️

... and 16 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.

andy-sweet · 2023-04-13T16:13:55Z

napari/_qt/layer_controls/qt_points_controls.py

@@ -208,11 +211,15 @@ def __init__(self, layer) -> None:

        self.layout().addRow(button_row)
        self.layout().addRow(self.opacityLabel, self.opacitySlider)
-        self.layout().addRow(trans._('point size:'), self.sizeSlider)
+        self.layout().addRow(trans._('current point size:'), self.sizeSlider)


I think changing the programmatic names helps clarify what's happening in the code. I understand the intent of making the GUI label consistent, but my initial reaction is not great.

Mostly because they just take up too much valuable horizontal space after this

My other issue is with the semantics of what current means, so publicizing those semantics more widely (i.e. in the GUI) worries me. One description is that it's the size of the currently selected point, but that's not true since I still see something when no points are selected. Or maybe it's the size of the next of the point I add - but then shouldn't it be next point size? Also, changing this when multiple points are selected will actually change the sizes of those points, so maybe this should be just be point size.

Also, when I've brought up this complexity before, I've heard that we're reusing the interaction people might be used in word processing software for selecting the font or font size of selected/next text, which seems pretty reasonable. But those software typically don't prefix the label or tooltip with current or selected - it's just font or font size.

In short, consider just leaving the label alone. But if you feel strongly here, I won't block the review on it.

One description is that it's the size of the currently selected point, but that's not true since I still see something when no points are selected. Or maybe it's the size of the next of the point I add - but then shouldn't it be next point size?

This is an actual conflation in the code, not just semantics; we don't distinguish between these two cases. I do feel that current point size more intuitively encapsulates "selected point sizes" and "current default size" than just point size, which several times caused people (me included) to expect a different behaviour. The most confusing thing to me is: if you have no points selected and you move the slider, nothing happens. I would expect all point sizes to change in that case.

I don't feel strongly about the label, but I think there are more upsides than downsides... though to be fair, I don't typically use small screens, so I'm not affected by the size issue.

I'm on Andy's side here — I definitely agree (I've probably made the point myself) that the analogous interaction is wysiwyg UIs for text or shapes, e.g. MS Word or Keynote. (Or LibreOffice if you prefer @brisvag! 😂) In none of those cases is the word "current" used, but they behave the same way as these controls — and if we diverge in behaviour somewhere, we should harmonise with those interactions, rather than move away from them. (For example, when multiple points with different face colors are selected, the face color box should "blank out" in some way, and clicking on it and selecting a color should find the right one.

So anyway, yes, I would ask to revert this change. And I feel more strongly about it than @andy-sweet! 😂 I actually think our controls are too big already!

Not sure I agree with the parallel with a text editor; in a text editor, your cursor is always somewhere and visible, so it's obvious where a change is happening. Here, it can be nowhere (i.e: no selection). You're maybe arguing that in that case it "at the end of the points", but it's not visible or obvious.

In fact, in those cases where it can be nowhere (e.g: powerpoint when you select the text box, rather than having the cursor inside), those controls do affect the whole content :P

Anyways, don't feel strongly enough about this to block the PR, but I do think we should "somehow" clarify this behaviour, because IMO the first thing one expects when moving the size slider is for the size of the points in the layer to change.

Man has someone used PowerPoint recently? It's hideous! 😂 All of the formatting elements disappear when you are in "shape adding" mode, which I find even more confusing. And the arrangement when you're editing shape formatting???

Anyway, not trying that again... 😂

your cursor is always somewhere and visible, so it's obvious where a change is happening. Here, it can be nowhere (i.e: no selection). You're maybe arguing that in that case it "at the end of the points", but it's not visible or obvious.

I really disagree that this is confusing — and btw in Word you can definitely scroll out of the view of the cursor and still have text formatting options and not think "omg what does this apply to??" It applies to either the next thing you type or to the thing that is selected, if any.

Regarding your interpretation of "point size", I would argue that "current point size" could be equally misinterpreted, and @andy-sweet is right that "next point size" makes more sense — unless you have a selection. We could switch the label around as people select and deselect, but I don't think we should go there just yet.

So I would simply add a tooltip here — "choose the size of selected points or points to be added" — and leave the text as is.

In fact, in those cases where it can be nowhere (e.g: powerpoint when you select the text box, rather than having the cursor inside), those controls do affect the whole content :P

Erm, that's selecting the whole text inside the box, ie cheating 😜. If you truly have nothing selected, then the controls disappear (though they used to control the next thing... I guess someone at Microsoft agrees with you 🤣)

We could switch the label around as people select and deselect, but I don't think we should go there just yet.

Yeah, agree this is not great. Let's just stick with the status quo for now :)

andy-sweet · 2023-04-13T16:32:46Z

napari/_qt/layer_controls/qt_points_controls.py

@@ -283,29 +297,32 @@ def change_text_visibility(self, state):
        state : QCheckBox
            Checkbox indicating if text is visible.
        """
-        # needs cast to bool for Qt6
-        self.layer.text.visible = bool(state)
+        with self.layer.text.events.visible.blocker(


Are you adding these because they're needed to avoid infinite recursion? Or if they're not needed right now, to prevent it in the future? Or is this more about efficiency - i.e. blocking a callback we know doesn't need to be executed?

I removed this one (and its control equivalent) and didn't get infinite recursion.

My initial reaction is just to omit blockers if they're not needed, but I think the other two reasons are fair. Regardless of what you choose to do, would be good to clarify why these are being added in this PR.

To prevent in the future; also, maybe we don't get infinite recursion cause we check somehwere (in the layer model, likely) if the value is the same, but we still get at least a double call to the setter, and potentially two events. There are several cases in the past where I was debugging events and found duplicated events. I think preventing these is valuable in case we (or a user) connects expensive callbacks. There's probably a cost to context managers, but hopefully that's negligible?

andy-sweet · 2023-04-13T16:38:48Z

napari/_qt/layer_controls/qt_points_controls.py

        """Update face color of layer model from color picker user input."""
-        with self.layer.events.current_face_color.blocker():


Ah, pretty clear why the bug in #5718 was happening then :D

andy-sweet · 2023-04-13T16:39:51Z

napari/_qt/layer_controls/qt_points_controls.py

@@ -208,11 +211,15 @@ def __init__(self, layer) -> None:

        self.layout().addRow(button_row)
        self.layout().addRow(self.opacityLabel, self.opacitySlider)
-        self.layout().addRow(trans._('point size:'), self.sizeSlider)
+        self.layout().addRow(trans._('current point size:'), self.sizeSlider)
        self.layout().addRow(trans._('blending:'), self.blendComboBox)
        self.layout().addRow(trans._('symbol:'), self.symbolComboBox)


If you want current in front of the other attributes, then I think you need it here too?

andy-sweet

I left some optional comments that could be addressed, but there's nothing in this PR that I oppose and it fixes the bug.

I thought about requesting tests partly because I know the Qt controls are poorly covered (and possibly related, we get fairly frequent bug reports) and because codecov reminded me of this. I appreciate some of these tests would be pretty shallow, so I'm not convinced of their value - would be ideal if they could be automatically generated.

jni

I like the code changes 🙏 but I'd like to please revert the UI changes. In the future I think we might even want to go more minimal and use symbols! e.g. like this but with points:

https://fontawesome.com/icons/text-size?f=classic&s=regular

alisterburt · 2023-04-15T09:27:33Z

Great work here - merging! 🚀

# Fixes/Closes Closes #5718. # Description Point controls were not firing events as intended. Most of the event blockers were either too aggressive (block everything, rather than just the relevant callback) or inverted. Additionally, there was a lot of asymmetry and confusion between how various elements worked. I clarified the gui labels to make it clearer when the `current_*` property is being affected. I also added a few events to match expectations with other parts of the `Points` layer. ## Type of change  - [x] Bug-fix (non-breaking change which fixes an issue) - [x] 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

unify event behaviour for points and its qt controls

70f88f9

brisvag mentioned this pull request Apr 13, 2023

Events are not emitted when colors are changed manually #5718

Closed

github-actions bot assigned brisvag Apr 13, 2023

github-actions bot added the qt Relates to qt label Apr 13, 2023

andy-sweet reviewed Apr 13, 2023

View reviewed changes

andy-sweet approved these changes Apr 13, 2023

View reviewed changes

jni requested changes Apr 14, 2023

View reviewed changes

remove current from labels

856f43e

jni approved these changes Apr 14, 2023

View reviewed changes

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

alisterburt merged commit cf55aa0 into napari:main Apr 15, 2023
31 of 33 checks passed

Czaki added bugfix PR with bugfix and removed ready to merge Last chance for comments! Will be merged in ~24h labels Apr 17, 2023

brisvag mentioned this pull request Apr 24, 2023

GUI point size slider does not control size of points #5748

Closed

brisvag mentioned this pull request May 2, 2023

Update annotate_points.md napari/docs#145

Merged

1 task

Czaki mentioned this pull request Jun 7, 2023

v0.4.18 - release process #5911

Closed

psobolewskiPhD added this to the 0.4.18 milestone Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify event behaviour for points and its qt controls #5722

Unify event behaviour for points and its qt controls #5722

brisvag commented Apr 13, 2023

codecov bot commented Apr 13, 2023 •

edited

andy-sweet Apr 13, 2023

brisvag Apr 13, 2023

jni Apr 14, 2023

brisvag Apr 14, 2023

jni Apr 14, 2023

brisvag Apr 14, 2023

andy-sweet Apr 13, 2023 •

edited

brisvag Apr 13, 2023

andy-sweet Apr 13, 2023

andy-sweet Apr 13, 2023

andy-sweet left a comment

jni left a comment

alisterburt commented Apr 15, 2023

		"""Update face color of layer model from color picker user input."""
		with self.layer.events.current_face_color.blocker():

Unify event behaviour for points and its qt controls #5722

Unify event behaviour for points and its qt controls #5722

Conversation

brisvag commented Apr 13, 2023

Fixes/Closes

Description

Type of change

codecov bot commented Apr 13, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet Apr 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-sweet left a comment

Choose a reason for hiding this comment

jni left a comment

Choose a reason for hiding this comment

alisterburt commented Apr 15, 2023

codecov bot commented Apr 13, 2023 •

edited

andy-sweet Apr 13, 2023 •

edited