pymmcore-plus / pymmcore-widgets

A set of Qt-based widgets onto the pymmcore-plus model
https://pymmcore-plus.github.io/pymmcore-widgets
Other
12 stars 7 forks source link

refactor: refactor stage widget #334

Closed tlambert03 closed 4 months ago

tlambert03 commented 4 months ago

closes #331

edit: let's do #332 elsewhere

tlambert03 commented 4 months ago

@gselzer, if you happen to have a moment to try this widget instead, would be curious to see if there is anything else we should hit while we're in there

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 82.19895% with 34 lines in your changes missing coverage. Please review.

Project coverage is 90.59%. Comparing base (b7fa819) to head (aa4160f).

Files Patch % Lines
src/pymmcore_widgets/_stage_widget.py 82.19% 34 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #334 +/- ## ========================================== - Coverage 90.66% 90.59% -0.08% ========================================== Files 74 74 Lines 8443 8442 -1 ========================================== - Hits 7655 7648 -7 - Misses 788 794 +6 ```

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

tlambert03 commented 4 months ago

i made the position labels editable, as per #332 ... however now that I'm going to hook them up to the core to actually move the stage, I'm realizing that's probably a bit unsafe? It's much easier to enter an invalid number via text than it is to hit a move button too many times.

We should perhaps have a "halt" button or something?

tlambert03 commented 4 months ago

Is there any way for us to change the relative difference in step size? i.e. what if instead of 2 x stepSize for the double chevron, you wanted 10x?

not at the moment, that would be a good feature. The MMStudio one lets you specify the step for each individual arrow:

Screenshot 2024-07-16 at 6 13 27 PM

it's a nice feature but obviously takes up more space, making it hard to have this widget part of a dock widget layout by default. so, in future PRs, lets consider ways to expose that functionality in a space efficient way.

What if, instead of always moving in microns, we could consider changing the units to something else, like "fields"?

😂 did you or the suggester already use the MMStudio widget above? or just come up with that on your own :) yep, a way to auto-populate steps based on fields would be nice too. again, let's do in a future PR

marktsuchida commented 4 months ago

As for the safety aspect, it might make sense to hide the ability to enter an absolute value behind a button (next to the position display; the button could be "Enter..." or just "..."). And the small dialog that this pops up could also indicate the distance by which the stage will move when confirmed. Just an idea.

But then again, if somebody has a workflow where they need to frequently enter absolute values, they may complain. (But maybe they should be using a position list or some other specialized way if they are doing that.)

Or maybe it should just be a configurable setting of the widget to allow/disallow entering values. I can certainly envision places where I wouldn't want to expose this to unexperienced (or even experienced) users. And I also feel that, although it happened to come up with our colleague, entering absolute coordinates is probably not the most typical operation.

tlambert03 commented 4 months ago

I undid the changes here that turn the position labels into editable spinboxes. I think that needs additional consideration, and this already fixes a number of things. I think directly editable spinboxes are too dangerous. I like something like @marktsuchida proposes, probably a button that says "MoveTo" or something like that, that opens up a completely new dialog with two fields and a move button. It's relatively rare that someone knows the absolute stage coordinates that they want to move to, and given the dangers, i think it makes sense to hide it behind a couple very purposeful actions