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

Added a save dialog with options #102

Merged
merged 14 commits into from
Aug 28, 2021
Merged

Conversation

Fifourche
Copy link
Contributor

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
Copy link

codecov bot commented Jun 2, 2021

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%) ⬇️
napari_animation/animation.py 86.86% <100.00%> (+0.69%) ⬆️
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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

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 !

Copy link
Collaborator

@alisterburt alisterburt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fifourche you've really gone above and beyond providing that binder link! Thanks so much and sorry for not being able to review properly without it 🙂

I have a couple of small comments/suggestions but the functionality is awesome and works really nicely! Awesome job

What do you think about making the qualityComboBox a QLabeledSlider from the new superqt repo? A slider might be more obvious than a combo box for the quality

I'm approving now but will wait to hear what you think about these suggestions before merging, if you'd rather not implement these changes yourself I'm happy to do them myself in a followup PR!

Thanks again!!

setup.cfg Outdated Show resolved Hide resolved
napari_animation/_qt/savedialog_widget.py Outdated Show resolved Hide resolved
napari_animation/_qt/animation_widget.py Outdated Show resolved Hide resolved
napari_animation/_qt/animation_widget.py Outdated Show resolved Hide resolved
napari_animation/_qt/savedialog_widget.py Outdated Show resolved Hide resolved
Co-authored-by: alisterburt <alisterburt@gmail.com>
setup.cfg Outdated Show resolved Hide resolved
@Fifourche
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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

Untitled

@Fifourche
Copy link
Contributor Author

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

@Fifourche
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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:

  • modifying the size policy of one or more widgets in the layout
  • setting a fixed width either via the stylesheet or via setFixedWidth
  • adding stretch factors

@Fifourche
Copy link
Contributor Author

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
Copy link
Contributor

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 alisterburt merged commit 07f93c7 into napari:main Aug 28, 2021
@alisterburt
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants