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

Remove max brush size from increase brush size keybinding #5761

Merged
merged 9 commits into from
Jul 4, 2023

Conversation

alisterburt
Copy link
Contributor

Fixes/Closes

@RobAnKo added autorepeatable key bindings and set an upper bound on the brush size equal to the default max brush size in the GUI (40px)

Often we want to increase it to more than that, so this PR removes that upper bound

discussed in #5753 - I don't think we need an upper bound here

We could (should?) also set the range for brush size in the gui dynamically based on some heuristic involving image shape/zoom at layer creation time, at least when creating from the viewer when an image layer is selected

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

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.

@alisterburt alisterburt changed the title Remove max brush size Remove max brush size from increase brush size keybinding Apr 22, 2023
@codecov
Copy link

codecov bot commented Apr 22, 2023

Codecov Report

Merging #5761 (444bbad) into main (19f83a2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5761   +/-   ##
=======================================
  Coverage   90.21%   90.21%           
=======================================
  Files         616      616           
  Lines       52154    52152    -2     
=======================================
  Hits        47049    47049           
+ Misses       5105     5103    -2     
Impacted Files Coverage Δ
napari/layers/labels/_labels_key_bindings.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

Looks good, two small comments:

  1. I can't click the number by the slider to increase it above whatever I manually ] to achieve. Initially it's 40--you can't type a larger value.
    This is because the QSlider maximum is still set to 40 here:

    But here we let setting the size override maximum, so ] works:
    def _on_brush_size_change(self):
    """Receive layer model brush size change event and update the slider."""
    with self.layer.events.brush_size.blocker():
    value = self.layer.brush_size
    value = np.maximum(1, int(value))
    if value > self.brushSizeSlider.maximum():
    self.brushSizeSlider.setMaximum(int(value))
    self.brushSizeSlider.setValue(value)

    I think if we're dropping the upper bound, then the field should be free-fill with a desired size.

I think you need to change:

def changeSize(self, value):
"""Change paint brush size.
Parameters
----------
value : float
Size of the paint brush.
"""
self.layer.brush_size = value

To something like:

        if value > self.brushSizeSlider.maximum():
            self.brushSizeSlider.setMaximum(int(value))
        self.layer.brush_size = value

and 2. is for a new PR:
would be nice to have some acceleration feature for [ and ]. the +1 -1 is awefully slow once you're you're in >100 range. Something like after some amount of holding it's +2, +3, etc?

@Czaki
Copy link
Collaborator

Czaki commented Jun 15, 2023

@alisterburt As it is marked for 0.4.18 cold you finish it, or I should do this?

@alisterburt
Copy link
Contributor Author

I've taken a look at this - for the GUI to work I have to make some changes to superqt - will submit a PR there

@alisterburt
Copy link
Contributor Author

but I don't know how long that process will take and don't want to hold up release

@alisterburt
Copy link
Contributor Author

PR opened at superqt pyapp-kit/superqt#172

@Czaki
Copy link
Collaborator

Czaki commented Jun 21, 2023

I'm not sure if @tlambert03 will accept it fast. The minimum and maximum should not be updated so silently in Slider.

But it is not a problem to create a subclass in napari to achieve the same result.

@alisterburt
Copy link
Contributor Author

@Czaki let's see how it goes over there first, @tlambert03 is fast and seems ready to merge - I'd rather fix upstream properly for this :-)

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

❤️

@jni
Copy link
Member

jni commented Jun 30, 2023

Just to update myself on this PR: things work from the keyboard, but the slider then falls out of sync, but to fix that, we need to fix superqt — but will the superqt fix then instantly fix napari or do we need concerted changes? ie, should we merge this. 😂

@alisterburt
Copy link
Contributor Author

@jni almost! The slider doesn't even fall out of sync if you use the keyboard, it's just not possible to achieve the keyboard behaviour (moving past the edge of the slider) with the mouse.

This is working fine it's just not perfect perfect, I vote merge this as is and follow up with slider changes in superqt - the change there is proving a little annoying and I don't know if I'll get it done

@jni jni dismissed psobolewskiPhD’s stale review July 3, 2023 01:09

changes require upstream changes, will follow up in later PRs

@jni jni added the ready to merge Last chance for comments! Will be merged in ~24h label Jul 3, 2023
@jni
Copy link
Member

jni commented Jul 4, 2023

@alisterburt Great, I will take advantage of the fact that @psobolewskiPhD is busy with a transconstinental move to sneakily merge this in 😂

@jni jni merged commit 43e8b9c into napari:main Jul 4, 2023
37 checks passed
@jni jni removed the ready to merge Last chance for comments! Will be merged in ~24h label Jul 4, 2023
jni added a commit that referenced this pull request Jul 4, 2023
# Fixes/Closes

@RobAnKo added autorepeatable key bindings and set an upper bound on the
brush size equal to the default max brush size in the GUI (40px)

Often we want to increase it to more than that, so this PR removes that
upper bound

discussed in #5753 - I don't think we need an upper bound here

We could (should?) also set the range for brush size in the gui
dynamically based on some heuristic involving image shape/zoom at layer
creation time, at least when creating from the viewer when an image
layer is selected

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants