napari / napari-animation

A napari plugin for making animations
https://napari.github.io/napari-animation/
Other
76 stars 27 forks source link

Added a save dialog with options #102

Closed Fifourche closed 3 years ago

Fifourche commented 3 years ago

This PR aims at improving the save procedure, and brings a draft of save dialog with options, as raised by #76 . Currently filters are not properly handled. The rest seems ok !

save_na

codecov[bot] commented 3 years ago

Codecov Report

Merging #102 (1d50f41) into main (b885fe5) will decrease coverage by 5.47%. The diff coverage is 26.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
- Coverage   90.38%   84.91%   -5.48%     
==========================================
  Files          20       21       +1     
  Lines         853      928      +75     
==========================================
+ Hits          771      788      +17     
- Misses         82      140      +58     
Impacted Files Coverage Δ
napari_animation/_qt/savedialog_widget.py 12.50% <12.50%> (ø)
napari_animation/_qt/animation_widget.py 84.37% <28.57%> (-1.19%) :arrow_down:
napari_animation/animation.py 86.86% <100.00%> (+0.69%) :arrow_up:
napari_animation/frame_sequence.py 98.83% <100.00%> (ø)

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 b885fe5...1d50f41. Read the comment docs.

alisterburt commented 3 years ago

hey @Fifourche, sorry for not following up on this sooner! I love the look of this from your screenshot but unfortunately can't play with it properly here, I keep running into the same issues as #39 (not your fault, I think this is a weird mac specific bug

alisterburt commented 3 years ago

Following up here, what do you mean exactly by 'filters are not handled properly'? I've been meaning to fix this save dialog issue properly on my machine and getting this in is great motivation 🙂

Fifourche commented 3 years ago

Hi ! :) I just wanted to say that the drop down menu is not displaying indivdually all the format options, just folder or else. Otherwise everything seems to work fine, even though tests would be needed !

I don't have access to a Mac, so I'm not sure I can help much... but you can try this to test the interface : https://mybinder.org/v2/gh/Fifourche/omero-analysis-desktop.git/HEAD?filepath=napari.ipynb

This would run a napari viewer on a mybinder server, and let you play with it. Of course not optimized for real visualization, but might be helpful to check things in case !

Fifourche commented 3 years ago

Thanks for the suggestions, I took them all into account ! :) There's one thing I can't seem to work out for now ; it's the display style of the QLabeledSlider... I'm not sure how to handle the spacing properly, if you have some knowledge about this !

Fifourche commented 3 years ago

Hey ! I'm still having problems with the looks of the QLabeledSlider for the quality ; the size of the label - the QDoubleSpinBox, seems to be set at a width way too large, as if it was taking into account up and down buttons even though they're not displayed...! Any advice for this, @tlambert03 ? :) save_dialog

tlambert03 commented 3 years ago

hmm... mine actually looks good, what version of napari are you on?

Untitled
Fifourche commented 3 years ago

Was still on 0.4.8, didn't realize... ^^ I just tried 0.4.10 and it's works, thanks !

Fifourche commented 3 years ago

Oh and for the failing test, only on mac, I don't have a clue what might have triggered this error... Is this is something you already encountered @alisterburt ?

TEARDOWN ERROR: Exceptions caught in Qt event loop:
  ________________________________________________________________________________
  Traceback (most recent call last):
    File "/Users/runner/work/napari-animation/napari-animation/.tox/py39-macos-pyqt/lib/python3.9/site-packages/napari/_qt/utils.py", line 304, in remove_flash_animation
      widget.setGraphicsEffect(None)
  RuntimeError: wrapped C/C++ object of type QtWidgetOverlay has been deleted
alisterburt commented 3 years ago

oh that is looking nice!!

For the failing test, I've seen these errors before... usually they occur if I try to modify an attribute of a layer after closing the viewer... I'll relaunch the tests here to see if it passes on a second go!

Fifourche commented 3 years ago

Was still on 0.4.8, didn't realize... ^^ I just tried 0.4.10 and it's works, thanks !

retried on the same computer and an other one, the problem is still there it seems... I'm not sure why ! I'll give a closer look at the superqt QSliderLabeland I might get back to you @tlambert03 !

Here are some informations about the packages' versions if needed :

napari: 0.4.10
Platform: Windows-10-10.0.19043-SP0
Python: 3.8.10 | packaged by conda-forge | (default, May 11 2021, 06:25:23) [MSC v.1916 64 bit (AMD64)]
Qt: 5.12.9
PyQt5: 5.12.3
NumPy: 1.21.1
SciPy: 1.7.0
Dask: 2021.07.0
VisPy: 0.7.1

OpenGL:
- GL version: 4.6.0 NVIDIA 462.31
- MAX_TEXTURE_SIZE: 32768

Screens:
- screen 1: resolution 1920x1080, scale 1.0

Plugins:
- animation: 0.0.3.dev89+g9d7b210
- console: 0.0.3
- scikit-image: 0.4.10
- svg: 0.1.5
tlambert03 commented 3 years ago

retried on the same computer and an other one, the problem is still there it seems...

when you previously tried it and thought it was working, was that a different OS?

tlambert03 commented 3 years ago

a closer look at the superqt QSliderLabel

I actually don't think this is a superqt issue, it's more of a stylesheet issue, and the styles of widgets inserted into a napari viewer come from napari itself... superqt just "obeys" the parent styles.

The basic idea with layouts is that they get allotted an area according to their size policy. So, if they all have the default size policy, they will all get equal space (Which could give the appearance of a big gap like you had there). The ways to fix it include:

Fifourche commented 3 years ago

So from what I understood, the buttons of the QDoubleSpinBox (which is used to display the slider's value) are not displayed thanks to setButtonSymbols(QSpinBox.NoButtons), but the buttons still occupy some space... I used a stylesheet to set their sizes to 0, and that seems to be ok !

Would there be some tests to write for this part of the interface then ? I see that codecov is failing, but at the same time there are no tests for rest of the plugin's interface either if I'm not mistaken !

tlambert03 commented 3 years ago

not totally following what does what there, but i pulled this and it looks good :)

One comment on saving: currently, the entire napari window will block until saving is done. This can go in another PR, but this would be a good time to use napari's thread_worker to put the saving into another thread, and instead of tqdm, you can use the progress parameter to have napari give you a progress bar in the activity window (rather than the console that launched napari, like tqdm).

nice work!

alisterburt commented 3 years ago

apologies for taking so long about this @Fifourche, really let this get away from me! Congrats, awesome work 🚀