Lock dimensions / axes while rolling

[Chris-N-K]

Description

The ability to roll through dimensions is a useful functionality in napari. However, in many use cases, it is desirable to only roll through or reorder the spatial dimensions. Particularly when dealing with time-resolved data, the time dimension should remain fixed most of the time. Currently, achieving this requires manually reordering the dimensions, which can be a tedious process.

Original Behaviour

In the current version, all dimensions are in the reorder dimensions pop-up window are draggable, and users can freely reorder or roll through them.

New Behaviour

The dimensions in the reorder dimensions pop-up window have a lock symbol in front of there name. If clicked the lock closes and the item is no longer draggable. When rolling these dimensions will stay in place while the other dimensions are rolled.

lock_dimensions_demo.mp4

References

• vscode
• https://doc.qt.io/qt.html#qtforpython
• https://github.com/napari/docs

Type of change

• New feature (non-breaking change which adds functionality)
• Enhancement (non-breaking improvements of an existing feature)
• Documentation (update of docstrings and deprecation information)
• This change requires a documentation update

How has this been tested?

• added tests for all newly introduced / modified functions and classes
• added test for new functionality of QtDimsSorter
• reworked old tests of QtDimsSorter to match new structure

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.

[Chris-N-K]

I would be happy about some input whether my approach is fine or if I should change the way the settings value is connected to the Dims object and how I can properly connect to the Dims.events.n_moveable_dims and not the settings event.

Furthermore, there is a bug that appears sporadic. If one selects viewer then ndim dimensions, rolls, select more dimensions and rolls again it can happen that only the previously selected number of dimensions is rolled.

[andy-sweet]

Thanks for the PR and the detailed description! Will try to leave more detailed comments in the future, but for now, I had some high level comments and thoughts.

Summary

Overall, this is a convenient feature that changes what the roll action does - specifically which dimensions are rollable. That action is bound to Dims._roll. If we assume that we don't move Dims._roll to another module (as is the case in this PR), then Dims needs access to the state that tells it which dimensions are rollable. While I don't love adding new state to Dims because it's a core component of napari, I think that's probably the best option for this feature.

The rollable dimensions are defined through a combination of the n_moveable_dims setting and Dims.ndim states. While the code for the derived property getter/setter is short, it makes things a little harder to reason about and also makes detecting changes to the derived property more difficult, which might explain some of the problems with events here. Also, an instance of Dims is now dependent on the global state in settings, which makes Dims harder to test and its behavior harder to reproduce in different environments.

Suggestion 1: define Dims.n_moveable_dims

Therefore, one suggestion is to just directly expose n_moveable_dims as state in Dims. That should hopefully make changes easier to reason about and make this more testable. You will likely need to modify the the root validator to ensure the value of this is compatible with ndim, as with most of the state in Dims. We'll need to add a GUI control for this somewhere (e.g. in the roll dialog), but let's work out the logic first.

Suggestion 2: define Dims.rollable

While n_moveable_dims is just one number, I find it a little hard to reason about, especially its default value, which should always match ndim to keep the existing behavior. Instead, adding an attribute like rollable: Tuple[bool, ...] adds more state, but it's simple and explicit. And the default value of all True should change nothing. We also have many Tuple[...] attributes that all maintain the length of ndim (in the root validator), so adding another doesn't worry me too much.

I think you mentioned in the community meeting that you went with this approach first, but found it hard to implement in the GUI. If you can get the API and code in good shape, I (or someone) else should be able to help on the GUI side of things if that's blocking you.

[andy-sweet]

Also, the video in the description doesn't play for me (tried on macOS with Chrome and Safari). Not a big deal because I just pulled down the PR and tried it out, but you may want/need to update it for others to quickly understand this feature.

[andy-sweet]

This isn't used anywhere in this PR, which might explain why you're having problems with the event associated with this mutable property. EventedModel can support events for derived attributes using __field_dependents__, but I think that won't work here since one dependency is on the global settings.

[Chris-N-K]

@andy-sweet thanks for the review. I'm not certain if it is a good idea to try and build the logic around dimension individual locking without investigating the solution on the GUI side further.

I will make n_movable_dims a state of Dims like you suggest, is probably the best solution.

Moving the control of n_movable_dims to the QtDimsSorter widget is a good suggestion. Are Field's easily incorporatable? The syntax, and auto widget and event generation is quite nice.

Defining rollable as a Tuple[bool] synergies well with the logic for individual dimension locking. It might be a little less intuitive if we can not go for this solution.

[Chris-N-K]

Also, the video in the description doesn't play for me (tried on macOS with Chrome and Safari). Not a big deal because I just pulled down the PR and tried it out, but you may want/need to update it for others to quickly understand this feature.

You are absolutely right, the video is shown and "plays" but nothing happens. Not sure why xD

[andy-sweet]

I'm not certain if it is a good idea to try and build the logic around dimension individual locking without investigating the solution on the GUI side further.

Agreed. Right now, it feels a little like we're using a setting here because it gives us a GUI widget for free. Adding a spin box to the dimension order dialog should be fairly easy, but could be a little clunky/ugly. I think this is another advantage of the rollable tuple of bools, which naturally implies a design where each dimension is checkable, but that also requires more GUI work outside of the stuff we get for free.

[andy-sweet]

I'm fairly busy next week, but I'll try to find some time to take another look here. Also happy to make a PR with suggestions if that's helpful.

[Chris-N-K]

I'm fairly busy next week, but I'll try to find some time to take another look here. Also happy to make a PR with suggestions if that's helpful.

Feel free :)
I will take a look into the SelectableEventedListAgain, maybe I can find out how to do it.

[andy-sweet]

I'm fairly busy next week, but I'll try to find some time to take another look here. Also happy to make a PR with suggestions if that's helpful.

Feel free :) I will take a look into the SelectableEventedListAgain, maybe I can find out how to do it.

I tried adding Dims.rollable and then using a special extension of QtListModel to handle the list of axes/dimensions. It mostly works without too much extra code, though I think there are some details to think through. Let me know what you think!

andy-sweet#54

[Chris-N-K]

I'm fairly busy next week, but I'll try to find some time to take another look here. Also happy to make a PR with suggestions if that's helpful.

Feel free :) I will take a look into the SelectableEventedListAgain, maybe I can find out how to do it.

I tried adding Dims.rollable and then using a special extension of QtListModel to handle the list of axes/dimensions. It mostly works without too much extra code, though I think there are some details to think through. Let me know what you think!

andy-sweet#54

@andy-sweet I like your solution a lot and worked on it a little bit. I made a pull request to your fork. Could you take a look? If you like it, it would be perfect if you change the draft to a pull request. I will merge it into this draft then.

[Chris-N-K]

I think I found the reason for some event related errors!
Every time one opens the pop-up a new instance of the QtDimsSorter is generated and new events are registered.
In these different instances live separate SelectableEventedLists for the axes. Versions with less axes cause the key error when move_indices is called.

-> there is no error with the move_indices function itself, but with the QtDimsSorter / axes_list generation

[andy-sweet]

@andy-sweet I like your solution a lot and worked on it a little bit. I made a pull request to your fork. Could you take a look? If you like it, it would be perfect if you change the draft to a pull request. I will merge it into this draft then.

Great! I did both things you asked.

[andy-sweet]

the ability to drag locked axes is blocked, but if you do try to drag a locked axis you get multiple axes selected--feels like selection of a locked and unlocked axis shouldn't happen? Probably niche and doesn't break anything, so no biggie, but maybe solvable with a flag.

I think we can just move Qt.ItemFlag.ItemIsSelectable into the self.getItem(index).rollable: block in QtAxisModel.flags. Though I have this nagging feeling that there is something special about the selectable flag in Qt... let me see if I can remember, otherwise I think we should just make this change.

[Chris-N-K]

@psobolewskiPhD and @andy-sweet thanks for the reviews.

Peter: I totally agree but the BIG problem with the selectable flag is that the moment we add this we can't take it away easily. What I mean is, if the object is clicked to lock it and raise the flag, it can not be clicked anymore to unlock it. Tried it, couldn't make it work. If you have an idea how to implement this I'm open to add it.

Andy 1: Waited for a reply on the comment. I can resolve it as it is the current practice of the other pop-ups though.

[psobolewskiPhD]

Re: Andy's question, I'm more of the 'more is more' kinda guy, so adding rollable to the API doesn't phase me. I think having a programmatic way of rolling axes would also be useful--I didn't realize it was private. I guess one could import the viewer_key_binding? (i know you can do it using numpy, but I think that shouldn't be needed to match a GUI function.)

Peter: I totally agree but the BIG problem with the selectable flag is that the moment we add this we can't take it away easily. What I mean is, if the object is clicked to lock it and raise the flag, it can not be clicked anymore to unlock it.

Wait, that's really bad...but I can't reproduce that.
Can you elaborate?
If I lock a dim using the menu I can later unlock it and everything seems to work just fine.
There is just the attempt to drag a locked layer resulting in multiselection that is weird--more on this later.

The other thing is that while you can't drag the locked dim, but you can drag the others above and below, which is equivalent to moving the locked dim... Not sure that's intended or not, but frankly I feel that the lock should just prevent the roll button from rolling the layer--this is really useful!--but if I want to move it to a different place I should be able to, whether I unlock first or not? It's hard to do it by accident.
So I would suggest dropping this check in qt_axis_model:

 if self.getItem(index).rollable:
            # we only allow dragging if the item is rollable
            flags |= Qt.ItemFlag.ItemIsDragEnabled

And just adding Qt.ItemFlag.ItemIsDragEnabled to flags.
Upside is this should fix the unexpected multiselection I noted previously...

I did run into one other thing--sorry--that bothers me a bit more:
I deleted all layers and then started over, but noticed odd behavior when trying to roll cells3D--turns out the locks remained after I deleted the old layers and then reappliedwhen I added new layers. I guess this is expected behavior based on the implementation but not really from the end-user PoV. Personally, I think when the LayerList goes to zero the locks should be removed, but I won't block on this point because I expect strong pushback on this from other core devs.

I've gone through everything and I'm ready to approve: it's useful and the implementation is clear. But I would like to hear your thoughts on the dragging bit and the persistence of the lock state.

[Chris-N-K]

@psobolewskiPhD I was talking about the isSelectable flag. When you add this to an object you can no longer do the unlocking by clicking the lock icon or at least it did not work for me. Maybe I did something wrong.

I thought about ways to prevent moving the objects by moving their surroundings but I found no easy solution. We can just drop the drag lock from the locked items sure. @andy-sweet what do you think about this?

It seems that with the last update the behavior of Dims changed thought that it lost axes when the largest layer was removed, but this is no longer the case. Thus, it keeps the state of the axes. We could add this but this would need some tinkering with the Dims object.

[andy-sweet]

Re: Andy's question, I'm more of the 'more is more' kinda guy, so adding rollable to the API doesn't phase me.

Thanks for the input - that's enough to convince me as a less is more guy!

I think having a programmatic way of rolling axes would also be useful--I didn't realize it was private.

I think that's because you could just do viewer.dims.order = np.roll(viewer.dims.order, 1) instead of viewer.dims._roll(), so the utility was considered limited. Having fixed or unrollable dimensions maybe makes it more useful, though at that point I'd expect an explicit assignment to the order you want.

Anyway, there is definitely a GUI case for having fixed/unrollable dimensions and I think it's in the spirit of napari to mirror GUI functionality as closely as possible in the viewer model, so I think we're good to keep Dims.rollable.

@Chris-N-K : My last request is to make Dims._roll public (i.e. Dims.roll). Otherwise, it's really weird to have Dims.rollable which does not affect anything else!

Peter: I totally agree but the BIG problem with the selectable flag is that the moment we add this we can't take it away easily. What I mean is, if the object is clicked to lock it and raise the flag, it can not be clicked anymore to unlock it.

Wait, that's really bad...but I can't reproduce that. Can you elaborate? If I lock a dim using the menu I can later unlock it and everything seems to work just fine. There is just the attempt to drag a locked layer resulting in multiselection that is weird--more on this later.

I also could not reproduce this. My guess is that there may be some OS / Qt version dependence here? I tried on macOS, and Windows with PyQt5.

We can just drop the drag lock from the locked items sure. @andy-sweet what do you think about this?

That works for me!

So to summarize, I'll approve once roll is public and the item flags issue is resolved (e.g. by dropping the drag lock).

[psobolewskiPhD]

I'm in agreement with Andy, so I think this is very close to getting merged! 🎉

[andy-sweet]

Personally, I think when the LayerList goes to zero the locks should be removed, but I won't block on this point because I expect strong pushback on this from other core devs.

This could be done easily in Dims.reset, which is called in ViewerModel when the layers change and there are none of them.

So I don't have an issue with this side-effect because there are existing related side-effects when removing the last layer.

@Chris-N-K : we've already asked so much of you here, so this is a non-blocking request. If you feel like adding it when making the latest updates, go for it! Otherwise, @psobolewskiPhD and I can make a follow up PR.

[Chris-N-K]

Will implement the last two points. The other idea might be better as an additional PR. This could be bigger then one expects.

[Chris-N-K]

Should be ready now. :)

[psobolewskiPhD]

I took the liberty to push some typing fixes (we dropped python 3.8) and fix the test for the flag changes.
🤞

[psobolewskiPhD]

I pulled this down again and it works smoothly now.
The question of what to do when all layers are removed can be addressed later.
I took the liberty of fixing some typing and test. The windows 3.12 seg fault (if it happens again) is unrelated.
So everything looks good to me.

[andy-sweet]

Looks good to me now!

@Chris-N-K: thanks for all your patience on this marathon PR!

@psobolewskiPhD : thanks for providing a second look here!

I'll file two issues regarding the other points raised in this PR (popup references, resetting rollable). Done in #6792 and #6793