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

Add delete button for layers #65

Closed
wants to merge 16 commits into from

Conversation

sofroniewn
Copy link
Contributor

What does this PR do? Why are doing this? References?

This PR adds a delete button above the layer list. Clicking the button will deleted the selected layers if there are any. Alternatively dragging and dropping onto the delete button will delete the dragged layers along with any others that are selected if the dragged layer is also selected.

Right now the deleting happens automatically. In the future we could add dialog box seeking confirmation before a delete that could be disabled in preferences.

One thing to note also is that if you delete all the layers and then try and add a new one you get an error. This behavior is true even if deleting layers directly from the notebook and should be fixed in some future PR.

One final note is that this PR also contains the drag and drop changes in PR #64 which have not been merged yet. The first 8 commits are in that PR and it should probably be merged first and then this one can be merged with only the changes relevant to the delete button.

Type of change

[Please delete options that are not relevant.]

  • 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?

  • TestCase A Run examples.ipynb and manually interacted with the gui
    image

Final Checklist:

  • My PR is the possible minimum 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

@sofroniewn sofroniewn added the feature New feature or request label Nov 18, 2018
@sofroniewn sofroniewn requested a review from a team November 18, 2018 21:25
@pep8speaks
Copy link

pep8speaks commented Nov 18, 2018

Hello @sofroniewn! Thanks for updating the PR.

Line 67:5: E301 expected 1 blank line, found 0
Line 68:5: E301 expected 1 blank line, found 0
Line 69:5: E301 expected 1 blank line, found 0
Line 70:5: E301 expected 1 blank line, found 0
Line 71:5: E301 expected 1 blank line, found 0

Line 1:80: E501 line too long (114 > 79 characters)
Line 8:24: E231 missing whitespace after ','
Line 8:32: E231 missing whitespace after ','
Line 10:1: E302 expected 2 blank lines, found 1
Line 14:80: E501 line too long (123 > 79 characters)
Line 15:80: E501 line too long (127 > 79 characters)
Line 21:80: E501 line too long (87 > 79 characters)
Line 38:80: E501 line too long (82 > 79 characters)
Line 47:80: E501 line too long (83 > 79 characters)
Line 97:24: E225 missing whitespace around operator
Line 97:80: E501 line too long (102 > 79 characters)
Line 122:5: E303 too many blank lines (2)
Line 130:9: E265 block comment should start with '# '
Line 131:9: E265 block comment should start with '# '
Line 132:9: E265 block comment should start with '# '

Line 14:9: E265 block comment should start with '# '
Line 57:63: E231 missing whitespace after ','
Line 58:61: E231 missing whitespace after ','
Line 77:80: E501 line too long (86 > 79 characters)
Line 81:80: E501 line too long (100 > 79 characters)
Line 101:80: E501 line too long (100 > 79 characters)
Line 117:24: E225 missing whitespace around operator
Line 120:29: E231 missing whitespace after ','

Line 3:80: E501 line too long (82 > 79 characters)
Line 9:28: E231 missing whitespace after ','
Line 9:36: E231 missing whitespace after ','
Line 11:1: E302 expected 2 blank lines, found 1
Line 22:1: E302 expected 2 blank lines, found 1
Line 34:80: E501 line too long (85 > 79 characters)
Line 35:80: E501 line too long (87 > 79 characters)
Line 36:80: E501 line too long (88 > 79 characters)
Line 56:1: E302 expected 2 blank lines, found 1

Comment last updated on November 20, 2018 at 05:45 Hours UTC

@sofroniewn
Copy link
Contributor Author

One extra thing to note, is that occasionally (though reproducibly) I get a warning message when deleting layers. One way to see it is to run the first 6 cells of the examples/layers.ipynb then in the gui unselect all the layers, select just the first one and click delete. Here is a copy of the error trace

WARNING: Traceback (most recent call last):
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/ipykernel_launcher.py", line 16, in <module>
    app.launch_new_instance()
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/traitlets/config/application.py", line 658, in launch_instance
    app.start()
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/ipykernel/kernelapp.py", line 499, in start
    self.io_loop.start()
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/tornado/platform/asyncio.py", line 132, in start
    self.asyncio_loop.run_forever()
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/asyncio/base_events.py", line 422, in run_forever
    self._run_once()
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/asyncio/base_events.py", line 1432, in _run_once
    handle._run()
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/asyncio/events.py", line 145, in _run
    self._callback(*self._args)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/tornado/ioloop.py", line 758, in _run_callback
    ret = callback()
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/tornado/stack_context.py", line 300, in null_wrapper
    return fn(*args, **kwargs)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/ipykernel/kernelbase.py", line 295, in advance_eventloop
    eventloop(self)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/ipykernel/eventloops.py", line 137, in loop_qt5
    return loop_qt4(kernel)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/ipykernel/eventloops.py", line 130, in loop_qt4
    _loop_qt(kernel.app)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/ipykernel/eventloops.py", line 114, in _loop_qt
    app.exec_()
  File "/Users/nicholassofroniew/Github/napari-gui/napari_gui/elements/qt/_layerPanel.py", line 40, in on_click
    self.layers.remove_selected()
  File "/Users/nicholassofroniew/Github/napari-gui/napari_gui/elements/_layer_list.py", line 333, in remove_selected
    self.pop(i)
  File "/Users/nicholassofroniew/Github/napari-gui/napari_gui/elements/_layer_list.py", line 199, in pop
    self.events.remove_item(item=item)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/util/event.py", line 455, in __call__
    self._invoke_callback(cb, event)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/util/event.py", line 471, in _invoke_callback
    cb(event)
  File "/Users/nicholassofroniew/Github/napari-gui/napari_gui/elements/_layer_list.py", line 310, in _remove
    layer.viewer = None
  File "/Users/nicholassofroniew/Github/napari-gui/napari_gui/layers/_base_layer.py", line 96, in viewer
    self._parent = parent
  File "/Users/nicholassofroniew/Github/napari-gui/napari_gui/layers/_visual_wrapper.py", line 44, in _parent
    self._node.parent = parent
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/util/frozen.py", line 17, in __setattr__
    object.__setattr__(self, key, value)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/scene/node.py", line 209, in parent
    self._update_trsys(None)
  File "/Users/nicholassofroniew/Github/napari-gui/napari_gui/_vispy/scene/visuals.py", line 82, in _update_trsys
    self.transforms.scene_transform = scene.node_transform(doc)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/visuals/transforms/transform_system.py", line 276, in scene_transform
    self._scene_transform.transforms = tr
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/visuals/transforms/chain.py", line 96, in transforms
    self.update()
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/visuals/transforms/base_transform.py", line 153, in update
    self.changed(*args)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/util/event.py", line 455, in __call__
    self._invoke_callback(cb, event)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/util/event.py", line 471, in _invoke_callback
    cb(event)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/visuals/transforms/chain.py", line 212, in _subtr_changed
    self.update(ev)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/visuals/transforms/base_transform.py", line 153, in update
    self.changed(*args)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/util/event.py", line 455, in __call__
    self._invoke_callback(cb, event)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/util/event.py", line 475, in _invoke_callback
    self, cb_event=(cb, event))
  << caught exception here: >>
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/util/event.py", line 471, in _invoke_callback
    cb(event)
  File "/Users/nicholassofroniew/anaconda3/lib/python3.6/site-packages/vispy/visuals/transforms/chain.py", line 281, in source_changed
    new_tr = [tr[0]]
IndexError: list index out of range
ERROR: Invoking <bound method SimplifiedChainTransform.source_changed of <ChainTransform [<STTransform scale=[1. 1. 1. 1.] translate=[0. 0. 0. 0.] at 0x4961768952>,
                 <STTransform scale=[1.0227273e+00 1.0227273e+00 1.0000000e-06 1.0000000e+00] translate=[138.18182   26.181818   0.         0.      ] at 0x4941059856>] at 0x126ca7da0>> for Event

gui/elements/qt/_layer.py Outdated Show resolved Hide resolved
gui/elements/qt/_layerDivider.py Outdated Show resolved Hide resolved
gui/elements/qt/_layerList.py Show resolved Hide resolved
gui/elements/qt/_layerPanel.py Outdated Show resolved Hide resolved
gui/elements/qt/_layerPanel.py Show resolved Hide resolved
@jni
Copy link
Member

jni commented Nov 20, 2018

@sofroniewn that traceback looks pretty nasty, and I imagine it's a bug in vispy since you are not doing anything too fancy here... Although maybe this is weakref biting us in the butt? ;)

What do people think about having a trashcan icon on the layers themselves?

@sofroniewn
Copy link
Contributor Author

My initial impulse was to have the trashicon on every button, but then I moved away from it as similar applications with layers have a single trash button, but I remain open to the possibility.

I can try deleting the weakref call and seeing if I get the same error

@kne42
Copy link
Member

kne42 commented Nov 20, 2018

The independent trashcan icon seems most intuitive to me, but I would like to note that while other applications typically modify their layers in-place, we will typically be creating a new layer every time. In that scenario, perhaps making the deletion of individual layers as easy as possible should be a priority.

@sofroniewn
Copy link
Contributor Author

I have just created a new PR #68 that adds more properties to layers, and any discussion of including a trashIcon on a layer or not should probably look at what changes are being proposed there

@sofroniewn
Copy link
Contributor Author

The changes in this PR are also included in #68 along with some additional ones. In order to simplify things I will close this PR in favor of the other one

@sofroniewn sofroniewn closed this Nov 22, 2018
@sofroniewn sofroniewn mentioned this pull request Nov 22, 2018
9 tasks
Czaki pushed a commit that referenced this pull request Jun 23, 2023
# Description
Adding right-click controls, main menu options and other copy edits to
napari viewer tutorial.

## Type of change
- [x] Fixes or improves existing content

# References
None

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality
- [x] I have added [alt text](https://webaim.org/techniques/alttext/) to
new images included in this PR

---------

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants