sofroniewn commented 3 years ago

Description

It's been pointed out in numerous places that our QtLayerList implementation is very complex right now as it implements a lot of custom drag/drop and scroll functionality ourself. This could make it very hard when we come and try and represent a TreeView for LayerGroups as proposed in #790. This PR aims to use a QListWidget instead and reduce the amount of custom code.

It is very WIP right now, lots of functionality is still missing, but I am able to display the widgets and drag and drop works on the view side. I need to do some linking with our model code, but that can come later. I might even wait until after #1376 merges and do this in the new EVH style :-)

Also styles will need to be tweaked again, but here is what it looks like right now:

Type of change

[x] Refactor (non-breaking change which fixes an issue)

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

tlambert03 commented 3 years ago

Do you imagine this will be usable after layer groups? Or just feel like the need is immediate enough to do it anyway?

sofroniewn commented 3 years ago

Do you imagine this will be usable after layer groups? Or just feel like the need is immediate enough to do it anyway?

Combination of immediate need (more maintable code, will make EVH refactor easier), and thinking the transition to layer groups will be much easier from the QListWidget to the QTreeWidget rather than from our custom implementation.

What's your take on this?

tlambert03 commented 3 years ago

I think things will be learned, and it can’t hurt to start working on it if you’re motivated. But I think a fair amount will change with groups. I suspect we’ll use qtreeview and qabstractitemmodel instead of qtreewidget. (That doesn’t mean we won’t be able to borrow some code from a new layerlist widget though)

jni commented 3 years ago

Very nice @sofroniewn! In that screenshot it's quite a bit uglier than what we have now imho, but I suspect as you say that's just some style tweaking needed. I'm totally agnostic as to whether this is better than a direct path from custom to trees.

tlambert03 commented 3 years ago

note from meeting: don't forget about napari._qt.utils.drag_with_pixmap. example usage in _qt.qt_plugin_sorter.QtHookImplementationListWidget.startDrag

sofroniewn commented 3 years ago

The reordering is proving pretty tricky - see discussion here https://github.com/napari/napari/pull/1394#issuecomment-653623618, I might need to enlist some help from @tlambert03 and @jni.

We should probably also think about fixing as much of #1380 as we can. The layerlist is a pretty key object, and it's not intuitive how to do basic things with it like reorder/ remove, see people asking questions like https://twitter.com/kD3AN/status/1279086938650218497

[Edit] I think step 1 would be to make sure ListModel is rock solid and fully tested (see #312 too) and then worry about the rest

tlambert03 commented 3 years ago

I don't want to sound like a broken record, but I really do think a lot of this could be improved with LayerGroups... not just because of the layergroup itself, but because we would be using Qt's model/view architecture properly ... (wherein we provide the model, and use a Qt View, rather than using QWidgets, which come with their own models included.)

in case you're curious, here's a very rough implementation that I've been playing with for a QTreeModel that wraps our layergroup model: https://github.com/tlambert03/napari/blob/layergroups-tjl/napari/_qt/qt_layertree.py#L8

the main idea here is that you provide concrete a implementation (called QtLayerTreeModel in that example) for an interface that Qt declares. (Their view classes are designed to work with their model interfaces... but you don't need the model to actually be an internal Qt object). Then when our internal python model changes, you just need to tell the view to refresh itself, and it will pull data directly from the python object (via the concrete QtLayerTreeModel interface object). And vice versa: if the user changes something in the view, the model interface directly updates the layergroup object (whereas the QLayerList first updates its own internal model... and then we have to sync our external layerlist model)

sofroniewn commented 3 years ago

We could do take this approach right now with the QListView no? Instead of waiting for the layer groups and the QTreeView. I really don't think we want to try and make this transition all in one go and think it will still be a while before we have layer groups figured out.

I can see how using the Qt View/Model instead of the Widget might make things easier in this case, but the problem effects every single QWidget we have that can both update and be updated.

I'm also struggling with the reordering of just out custom ListModel - i.e. no Qt stuff at all. I've started adding some ListModel tests (there were none before!). Hopefully we can get that figured out first

tlambert03 commented 3 years ago

We could do take this approach right now with the QListView no?

yep!

sofroniewn commented 3 years ago

I've made progress with emitting events from the ListModel, but I am back to problems with the QListWidget, particularly because we're trying to style it with a custom widget - setItemWidget which comes with this rather ominous warning about only being for static lists - see here https://doc.qt.io/qt-5/qlistwidget.html#setItemWidget.

While I'm able to get drag and drop working if I try and programmatically reorder the widget nothing works as expected, and using setItemWidget again in the way I would have liked results in segfaults. Their recommended approach is the QListView approach, so we'll see if I start going down that path tomorrow!!

sofroniewn commented 3 years ago

Ok @tlambert03 - looks like our hand will really be forced here by limitations of setItemWidget and that to get any reordering programatically we need to go down the QListView road as mentioned in the docs link above (see this stackoverflow post and this thread on the Qt bug list to indicate that I really don't think we can achieve what we want with just a QListWidget

I'm a little worried about the added complexity of the QListView + QAbstractItemModel approach as we'll also have our own LayerList model that I really do want to keep off the QAbstractItemModel. We'll still be maintaining two separated models, but I guess we will have more flexibility now that the actual Qt one is decoupled from its view.

I might start down this road tomorrow, we'll see. Let me know if you have further thoughts. I'd love for us to get this wrapped up soon!!!

sofroniewn commented 3 years ago

I'm curious @tlambert03 if you think we can use https://doc.qt.io/QT-5/qabstractitemmodel.html#moveRow and stick with the QListWidget? I'm going to look into it in the morning, might make it easier if we could!!

tlambert03 commented 3 years ago

I'm a little worried about the added complexity of the QListView + QAbstractItemModel approach as we'll also have our own LayerList model that I really do want to keep off the QAbstractItemModel.

not sure I follow here... Are you saying that you don't want the QAbstractItemModel to have awareness of the the LayerList object?

tlambert03 commented 3 years ago

We'll still be maintaining two separated models

that's just it... you won't be. don't think of QAbstractItemModel as an independent model from LayerList (and by model here, I mean source of truth). Think of it as a proxy object for the LayerList that provides the appropriate interface for the Qt View classes. It doesn't need to actually hold data. (it can if you want to, but it doesn't need to). For this to work of course, the QAbstractItemModel does need to know about the LayerList object. But something needs to know about something, and it's better that way (IMO) than having two separate models (sources of truth) that need be syncronized.

I do agree it seems more complex than the widget classes provided by Qt. But from my reading on this, so far I think this is the "proper" way to do create a Qt View onto a complex python object without duplicating models.

sofroniewn commented 3 years ago

Are you saying that you don't want the QAbstractItemModel to have awareness of the the LayerList object?

I think that would be nice, and that we should try and keep as strong a separation as possible between the Qt code and our own models and have them communicate only via our event handler, with the goal of improving the modularity and separation of concerns in the codebase.

I'm still struggling to get the reordering stuff working, but that's because of Qt limitations, nothing to do with the EVH stuff ... but we'll see how it all goes ...

sofroniewn commented 3 years ago

For this to work of course, the QAbstractItemModel does need to know about the LayerList object. But something needs to know about something, and it's better that way (IMO) than having two separate models (sources of truth) that need be syncronized.

Yeah I guess this is the tradeoff. I'd still rather try and keep our Qt stuff separated, especially as the list views only need the name, thumbnail, visibility and selected status - i'd rather avoid giving them access to everything, but let's see how it goes!!

tlambert03 commented 3 years ago

I think that would be nice, and that we should try and keep as strong a separation as possible between the Qt code and our own models and have them communicate only via our event handler,

for me, keeping Qt separated means that our models shouldn't know about Qt... But it doesn't mean that Qt stuff shouldn't know about our models. In most MVC designs, the view has to see the model. The QtLayerList here is a view into the LayerList model. I don't think the event handler is anywhere close to being "rich" enough to provide everything necessary to the Qt objects. And even if it were, it necessitates duplicated sources of truth.

sofroniewn commented 3 years ago

Ok, I've made big progress here, all functionality is now working and all tests passing with minimal changes (no functionality dropped, some name changes, and some extra tests added!!). I was able to get it all working fairly elegantly with the QListWidget the only things I had to do that might appear slightly unnatural were leverage the item.text property for sorting and overwrite the dropEvent and manually determine the parameters of the reorder call, but neither of those are too bad.

One thing I can't figure out is how though is the test_qt_layerlist.py is still leaking widgets. Note this test doesn't create the full viewer, it only creates the QtLayerList for testing in isolation, so I thought I'd be able to just call close and it would be fine. Maybe @tlambert03 can take a look at that one.

I still have to work on stylings, I will give it a first pass in a couple hours and then maybe turn this over to @tlambert03 for further improvements.

I think it will be good to try and get this is in relatively as is, and then we can make further improvements when we go to tree views etc.

I'll post when this is ready for through review, for now though just help fixing the leaking widget would be great

sofroniewn commented 3 years ago

This PR is now ready for full review. The stylings have been fixed, though feedback, adjustments welcome. Play around with the drag/ drop too and let me know if you like it.

and

with lots of layers, which I think is quite nice.

A couple key points are:

all the custom qt layer list stuff is now gone, we use the qlistwidget instead, which is much cleaner.
we now use the event handler on our ListModel itself (and so the LayerList too).
- grid mode is still broken, see #1400, but I'd like to fix that in its own PR after this merges.
I made some improvements to active_layer but that is still not user settable. I'm thinking it belongs on the layer list in the future too, like how QListWidget has a current row property in addition to the selected properties, but that fix can come later. These changes don't make it worse.
I would like to drop the changed event from the ListModel in the upcoming world coords stuff, but as it was there before no need to drop it now.
I'm still leaking widgets in the test_qt_layerlist.py test and need help with that.

Looking forward to feedback from @jni @tlambert03 @GenevieveBuckley

tlambert03 commented 3 years ago

One thing I can't figure out is how though is the test_qt_layerlist.py is still leaking widgets. Note this test doesn't create the full viewer, it only creates the QtLayerList for testing in isolation, so I thought I'd be able to just call close and it would be fine. Maybe @tlambert03 can take a look at that one.

looks like the reference to the Widget in the EventHandler is causing this. comment out the register_listener bit and it's fine. One potential fix is to add weakrefs instead of direct references... for instance, this also works in the EventHandler

import weakref

...

    def register_listener(self, component):
        self.components.append(weakref.ref(component))

    ...

    def on_change(self, event=None):
        ...
        # Update based on event value
        for componentref in self.components:
            component = componentref()
            if component is not None:

EDIT: this of course has a bit of a bad smell... but I'm not immediately sure how to handle it. as is, the event handler is gradually going to maintain pointers to most objects in the program, so this will certainly pop up again

codecov[bot] commented 3 years ago

Codecov Report

Merging #1391 into master will increase coverage by 0.44%. The diff coverage is 91.18%.

@@            Coverage Diff             @@
##           master    #1391      +/-   ##
==========================================
+ Coverage   87.79%   88.23%   +0.44%     
==========================================
  Files         239      242       +3     
  Lines       22048    21923     -125     
==========================================
- Hits        19357    19344      -13     
+ Misses       2691     2579     -112

Impacted Files	Coverage Δ
napari/_qt/qt_viewer_buttons.py	`98.82% <ø> (+11.82%)`	:arrow_up:
napari/_tests/test_image_rendering.py	`100.00% <ø> (ø)`
napari/_viewer_key_bindings.py	`82.60% <ø> (-2.58%)`	:arrow_down:
napari/components/_tests/test_layers_list.py	`100.00% <ø> (ø)`
napari/utils/list/_base.py	`68.75% <ø> (ø)`
napari/utils/list/_multi.py	`66.66% <ø> (ø)`
napari/_qt/qt_controls.py	`82.85% <50.00%> (ø)`
napari/_qt/utils.py	`77.46% <50.00%> (-1.11%)`	:arrow_down:
napari/_qt/qt_viewer.py	`79.06% <60.00%> (-0.80%)`	:arrow_down:
napari/_qt/qt_layerlist.py	`75.32% <76.71%> (+15.76%)`	:arrow_up:
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e43f730...8791070. Read the comment docs.

jni commented 3 years ago

@sofroniewn before I review the code, here are some notes from our Zulip chat about potential usability improvements/issues:

[x] dragging a layer into the trashcan is allowed but crashes napari [1]
[ ] dragging a layer to the top or bottom of the list is finicky, as it's a small drop space and there's little visual indication of whether it's going to work. All of the bottom space should be available to drop. My guess is that the most used drag/drop will be to drag to the top, so that should be easy to do.
[ ] dropping in between two layers is tricky. There's a few issues, and it's hard to tell a priori which combination of the following suggested fixes will give the best user experience:
- the insertion marker is hard to see under the layer being dragged. It would be excellent to have it be more prominent, and extend all the way out under the thumbnail, maybe even a bit past it if there's room for that.
- the layer being dragged obstructs the insertion marker most of the time, so the layer marker opacity should be decreased while that layer is being dragged.
- the insertion location is determined by the location of the cursor rather than the location of the dragged glyph. This one I'm not sure about but it's worth experimenting.
[x] it's finicky to find a grab point, because click and drag over the text field results in click-through and text selection. Click and drag over the text should drag the layer unless the text box has been activated with a click (similar to the macOS Finder).

Ok time for code review now. =)

[1]:

napari crash when adding a points layer then dragging it to the trash icon

```pytb $ python examples/add_image.py WARNING: Traceback (most recent call last): File "/home/jni/projects/napari/napari/_qt/qt_viewer_buttons.py", line 188, in dropEvent layer = self.viewer.layers[layer_name] File "/home/jni/projects/napari/napari/utils/list/_base.py", line 28, in __getitem__ indices = self.__prsitem__(key) File "/home/jni/projects/napari/napari/utils/list/_multi.py", line 11, in __prsitem__ return super().__prsitem__(keys) File "/home/jni/projects/napari/napari/utils/list/_base.py", line 80, in __prsitem__ return self.__locitem__(key) File "/home/jni/projects/napari/napari/utils/list/_typed.py", line 36, in __locitem__ key = self.index(key) File "/home/jni/projects/napari/napari/utils/list/_typed.py", line 82, in index raise KeyError(f'could not find element {q} was referencing') KeyError: 'could not find element was referencing' Aborted (core dumped) ```

jni commented 3 years ago

(Also, can you make sure that all new code lines are covered? I did not check that.)

tlambert03 commented 3 years ago

some usage observations before looking at code:

styles:
- [x] corners of the widgets have lost their rounded, border radius
- [x] I like that the widgets now adjust their size when a scrollbar appears, but it's rather tight. would be nice to have some padding as before:
[x] selection behavior: it used to be the case that you could drag and drop a non-selected widget to reorder. now what happens is that it highlights the text, but does not move the widget. Furthermore that highlight persists until you select and edit the label to remove the highlight. old behavior:

new behavior:

sofroniewn commented 3 years ago

Fixed selection behaviour (i.e. problem with text) and the rounded corners. Still need to fix the space between list items and scrollbar and the three other drag/ drop related ones.

sofroniewn commented 3 years ago

Scroll bar padding now fixed too

sofroniewn commented 3 years ago

Ok I've gone through and addressed changes as best I can. I dropped the ability to drag and drop onto the trashcan (we can always add back in later if people feel strongly, but hover stylings weren't really working for it anyway).

I also couldn't figure out how to improve the stylings for the drop indicator or improve the drag/ drop experience at the top/ bottom of the list widget. Maybe @tlambert03 can give that a try? I did improve the text highlighting problem which I think was far worse. We can also continue to improve the stylings after merge too.

I've also left in the changed event and _on_changed_change even though it's a bit weird. I want to remove them in the next PR and we already had the changed event, so I feel like it's much simpler to just leave it rather than rename it only to take it out next PR.

Would be great @tlambert03 if you could go through the code, and also let me know if you can figure out how to improve the drop. A couple things I looked into were painting our own drop indicator (I couldn't figure out how to change it's thickness), and improving this line https://github.com/napari/napari/pull/1391/files#diff-6408ff7cd9e06a487f66df8d17583e6eR155-R156 which ignores the drop if the cursor isn't over one of the list items.

Feel free to take another pass too @jni!!

sofroniewn commented 3 years ago

I just added additional opacity scaling at a level of 0.7 for the drag pixmap. Easy to add more

sofroniewn commented 3 years ago

Upon suggestion from @jni I've lowered the opacity to 0.5 and I've further improved the interaction between the QLineEdit box and the dragging functionality - you only enter the edit mode (focus) of the QLineEdit after you've clicked on it once and then released a click. This change allows you to drag a QtLayerWidget around quite nicely.

The only things not implemented at this point on the stylings are better drag/ drop at the top/ bottom and a nicer looking dropIndicator.

sofroniewn commented 3 years ago

Ok, looks like there is another selection bug now where sometimes clicking/ dragging selects multiple items. Not sure how to deal with this one, but if @tlambert03 could look into that too that would be great! I think I've done all the Qt styling I can for one night

sofroniewn commented 3 years ago

Ok, looks like there is another selection bug now where sometimes clicking/ dragging selects multiple items.

I've determined the cause of this problem, it comes when clicking and dragging "between" the list elements, it's very easy to see if you increase the list spacing size. I havn't quite figured out the best solution yet, but at least I understand the problem!!

List with increased size, where click dragging in between elements lead to selection!

sofroniewn commented 3 years ago

Ok this is better, but I still can't quite get it, I still have problems with selections sometimes happening when they shouldn't, but I really don't know why. For me this works as expected though > 95% of the time. Curious what it is like for others!!

sofroniewn commented 3 years ago

Ok, I've fiddled with the stylings and interactivity one more time. I reverted the very last change around improving selectability - but I now think interactivity is close to identical with that on master right now (and improved since the last review from @tlambert03 which had deficits from master).

Neither the top/ bottom drag/ drop nor the highlighting of text after the layer has been selected are different from master, but I see this PR really as a refactor - NOT - a time to add new functionality.

I'd like to merge this tonight @jni @tlambert03 so I can press on with world coordinates. Can you give it one last try. I'm happy to make more changes to stylings if you find performance lagging master, but I don't want to spend more time trying to add functionality not present in master.

tlambert03 commented 3 years ago

About to take a look. Is there anything in here that world coordinates depends on? Or is it just the mental burden of having two things going on at once? I definitely don't think you should spend much more time on this, since we don't know how much is going to survive the change to layer groups. At the same time, this is a pretty key part of the interface, so I don't necessarily want to rush a merge... (but I agree, no reason to go above and beyond the current master, just don't want to go backwards in the UX in any way).

sofroniewn commented 3 years ago

Is there anything in here that world coordinates depends on? Or is it just the mental burden of having two things going on at once?

More so the mental burden/ refactor burden. Will be easier to not be making two big changes to layerlist at same time. I want to revisit and get #1360 merged after this.

I definitely don't think you should spend much more time on this, since we don't know how much is going to survive the change to layer groups.

Completely agree here.

At the same time, this is a pretty key part of the interface, so I don't necessarily want to rush a merge... (but I agree, no reason to go above and beyond the current master, just don't want to go backwards in the UX in any way).

100% yeah, I don't want to make things worse. Let me know how it feels now, I think it's back to parity, but can be hard to catch everything!!

tlambert03 commented 3 years ago

noticing one change from master WRT sorting: if you select multiple layers and move them, their order is reversed. I think it's fine to let this pass for now, but just wanted to note it somewhere, and when we refactor for Tree view, we should make sure that moving layers preserves relative order.

Untitled

sofroniewn commented 3 years ago

noticing one change from master WRT sorting: if you select multiple layers and move them, their order is reversed.

Nice catch! That was an easy fix, just had to reverse one list!! Let me know if that now works for you and if you have any other requests, otherwise I'll consider this good to merge.

sofroniewn commented 3 years ago

After more discussion with @jni I've gotten the green light for merge. We'll need to add some qt tests to cover our custom drag/ drop (see coverage report here https://codecov.io/gh/napari/napari/compare/e43f730170aab1e011c42060c65ba44133f6f18f...87910700153881903401c4f1d3f58285c94a674b/src/napari/_qt/qt_layerlist.py?before=napari/_qt/qt_layerlist.py) but i will make a dedicated issue for that as we havn't had test like that before (the code before the refactor in this place was also un-tested)

napari / napari

Use qlistwidget for QtLayerList #1391

Description

Type of change

Final checklist:

Codecov Report