Hide or Destroy dock widgets

[ppwadhwa]

This PR adds the options to hide or destroy a dock widget from the title bar.

See #3101

Screen.Recording.2021-09-03.at.1.21.22.AM.mov

[tlambert03]

love it! will have a look soon

[ppwadhwa]

@tlambert03 I'm glad to hear this response. :) I still need to do a little more work on it. Got some direction from @goanpeca on some trouble I was having with it, so will make a few changes and request a review from you soon!

[codecov]

Codecov Report

Merging #3331 (50ce25b) into main (c4c987c) will increase coverage by 0.03%.
The diff coverage is 91.04%.

@@            Coverage Diff             @@
##             main    #3331      +/-   ##
==========================================
+ Coverage   82.65%   82.68%   +0.03%     
==========================================
  Files         549      549              
  Lines       45690    45691       +1     
==========================================
+ Hits        37763    37780      +17     
+ Misses       7927     7911      -16     

Impacted Files	Coverage Δ
napari/_qt/widgets/qt_viewer_dock_widget.py	94.73% <88.23%> (-1.80%)	⬇️
napari/_qt/menus/plugins_menu.py	92.30% <91.66%> (+23.95%)	⬆️
napari/_qt/qt_main_window.py	74.30% <92.85%> (-1.86%)	⬇️
napari/_qt/_tests/test_plugin_widgets.py	99.02% <100.00%> (+0.06%)	⬆️
napari/_qt/widgets/_tests/test_qt_dock_widget.py	100.00% <100.00%> (ø)
napari/_qt/perf/qt_performance.py	97.10% <0.00%> (+7.24%)	⬆️

---

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 c4c987c...50ce25b. Read the comment docs.

[ppwadhwa]

@tlambert03 I am ready for your review. One of the last things I did was to only include a "hide" dock widget option for every non-plugin dock widget. For the console, layer list, etc., it seemed a bit complicated if that widget was destroyed and then recreated if a user selected it from the menu. I decided to leave that for another time if that is wanted. This means, though, that those dock widgets have an "eye" symbol and not and 'x'. Let me know if this is OK, or if should use different symbols.

cc @goanpeca

[tlambert03]

the functionality is great. I really want us to be "killing" plugin dock widgets more readily, and this accomplishes that, so happy to have this go in (once tests have been added that assert that when you close a plugin dock widget, it does indeed get destroyed, and it gets removed from window._dock_widgets, and stuff)

I feel like the code itself is starting to get a little messy ... we're starting to spread and mix together the logic for various concerns (plugin widgets, builtin dock widgets, etc...), and methods are getting longer and harder to test. everywhere that comments "check to see if this is a plugin" should ideally be in a section that knows it it's a plugin widget. (remember, all of these widgets were added at one point with either add_dock_widget or add_plugin_dock_widget... so that information was known at one point in the function chain... )

I'm fine with this going in as is, but it would be a good target for a future refactor

[tlambert03]

can you comment on this? it feels weird to me that someone would ever call add_plugin_dock_widget(...) and the result would be to hide the widget (even if it already existed). i.e. add_plugin_dock_widget is not a "toggle dock widget" method

[ppwadhwa]

this was something I added when I was trying to get the behavior to work properly when a widget was undocked and then you toggled the widget from the menu. Right now, it will call add_plugin_dock_widget when an action is toggled. I suppose I should change that?

[tlambert03]

can't see where this is used?

[ppwadhwa]

it looks to be left over from an old idea. thanks for catching this.

[tlambert03]

really don't like creating new signals and involving other widgets (here the QMainWindow) in the business of "managing" other widgets if it's at all possible to have that other widget manage itself. In this case, it looks like you're creating new signals so that you can check and uncheck the menu action when this gets hidden and shown... but don't you already have a handle to that action below (around line 275)? do we need the main window to do this?

[tlambert03]

what causes the attribute error here?

[ppwadhwa]

This will fail on start up. I think QtCustomTitleBar is called for the layer list and layer controls before the plugins menu is created.

[ppwadhwa]

thanks @tlambert03 ! I will make some changes based on your comments. I want to get it right, so I'm happy to work on it some more. I may ask for help if I can't get something to work.

[ppwadhwa]

quick update---I've addressed some of the comments above, but still need to add some tests.

Also, I think I should re-work this part of code:

          def _add_widget(*args, key=key, hook_type=hook_type):
                if hook_type == 'dock':
                    self._win.add_plugin_dock_widget(*key)
                else:
                    self._win._add_plugin_function_widget(*key)

            action.setCheckable(True)
            # check that this wasn't added to the menu already
            actions = [a.text() for a in menu.actions()]
            if action.text() not in actions:
                menu.addAction(action)
            action.triggered.connect(_add_widget)

I am treating add_plugin_dock_widget and _add_plugin_function_widget as toggle functions. We don't want that, right @tlambert03 ?

[tlambert03]

I am treating add_plugin_dock_widget and _add_plugin_function_widget as toggle functions. We don't want that, right @tlambert03 ?

correct. i know we probably need the concept of "toggle" somewhere in there, but the actual add_plugin_dock_widget function itself should not be the "toggler".

[ppwadhwa]

@tlambert03 I'm ready for you to take a look again! thank you!

[goanpeca]

LGTM @ppwadhwa, thanks!

[tlambert03]

good stuff!

[sofroniewn]

LGTM! Nice work