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

Fix opening file dialogs in PySide #5492

Merged
merged 5 commits into from Feb 1, 2023
Merged

Conversation

Czaki
Copy link
Collaborator

@Czaki Czaki commented Jan 18, 2023

Description

Even if official documentation of qt says that the argument of getOpenFileNames is named dir, the dir is a builtin function in python, so PySide2, PyQt5, and PyQt6 use directory as the argument name. But PySide6 breaks this.

The PyQt6 does not support QFileDialog.Options()

Type of change

  • 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

References

How has this been tested?

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change
  • example: I check if my changes works with both PySide and PyQt backends
    as there are small differences between the two Qt bindings.

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
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #5492 (95ce540) into main (180992f) will increase coverage by 0.00%.
The diff coverage is 83.33%.

@@           Coverage Diff           @@
##             main    #5492   +/-   ##
=======================================
  Coverage   89.31%   89.32%           
=======================================
  Files         600      600           
  Lines       51060    51066    +6     
=======================================
+ Hits        45605    45614    +9     
+ Misses       5455     5452    -3     
Impacted Files Coverage Δ
napari/_qt/qt_viewer.py 78.78% <83.33%> (-0.15%) ⬇️
napari/_qt/dialogs/qt_package_installer.py 81.67% <0.00%> (+0.39%) ⬆️
napari/utils/theme.py 92.89% <0.00%> (+0.59%) ⬆️
napari/utils/info.py 81.44% <0.00%> (+1.03%) ⬆️
napari/_qt/__init__.py 56.66% <0.00%> (+6.66%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

I merged this with the other pyside6 PR and it works nice.
Makes sense to me.

Out of curiosity, it's >10 lines duplicated—does it make sense from a maintenance PoV to make this a function?

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 19, 2023

Out of curiosity, it's >10 lines duplicated—does it make sense from a maintenance PoV to make this a function?

done

@psobolewskiPhD
Copy link
Member

Out of curiosity, it's >10 lines duplicated—does it make sense from a maintenance PoV to make this a function?

done

Bah, you didn't have to! I was just asking! 😬

BTW I get this message in console:

2023-01-19 21:08:39.248 napari[35804:10079388] +[CATransaction synchronize] called within transaction

Seems like it may be macOS 13 specific:
https://forum.qt.io/topic/141868/macos-catransaction-synchronize-called-within-transaction/7
Just putting this somewhere for future reference.

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 19, 2023

Bah, you didn't have to! I was just asking! grimacing

but it is improvement

napari/_qt/qt_viewer.py Outdated Show resolved Hide resolved
@Czaki Czaki linked an issue Jan 25, 2023 that may be closed by this pull request
@Czaki Czaki changed the title Fix opening file dialogs in PySide6 Fix opening file dialogs in PySide Jan 25, 2023
@psobolewskiPhD
Copy link
Member

Kinda amazing this was missed this long, but I have to admit when I was new I used File > Sample and drag-n-drop and now I largely use those two plus programatic methods. So I'm never doing File > Open.

@Czaki
Copy link
Collaborator Author

Czaki commented Jan 25, 2023

An badly it is not possible to test this in normal way.

Maybe I should bring issue to provide both PySide and PyQt bundles?

@Czaki Czaki merged commit 3d926e5 into napari:main Feb 1, 2023
@Czaki Czaki deleted the fix_PySide6_open_dialog branch February 1, 2023 11:27
@Czaki Czaki added this to the 0.5.0 milestone Feb 5, 2023
@Czaki Czaki added the bugfix PR with bugfix label Feb 5, 2023
kne42 added a commit to kne42/napari that referenced this pull request Feb 28, 2023
* main: (38 commits)
  Fix `test_worker_with_progress` by wait on worker end (napari#5548)
  Un-set unified title and tool bar on mac (Qt property) (napari#5533)
  Set PYTHONEXECUTABLE as part of macos fixes on (re)startup (napari#5531)
  Fix key error issue of action manager (napari#5539)
  Clean dangling widget in test (napari#5544)
  Use pytest-pretty for better log readability (napari#5525)
  Update vendoring tool to check on matplotlib colormap (napari#5181)
  MAINT: add time limit for CI. (napari#5495)
  Add show_debug notification (napari#5101)
  Overlays 2.0 (napari#4894)
  Clarify layer's editable property and separate interaction with visible property (napari#5413)
  ci(dependabot): bump docker/build-push-action from 3 to 4 (napari#5523)
  Fix opening file dialogs in PySide (napari#5492)
  [pre-commit.ci] pre-commit autoupdate (napari#5518)
  Replace flake8, isort and pyupgrade by ruff, enable additional usefull rules (napari#5513)
  MAINT: Don't format logs in log call (napari#5504)
  Fix conda avaliability check (napari#5496)
  Handle case when QtDims play thread is partially deleted (napari#5499)
  Bugfix: Add missing Enums and Flags required by PySide6 > 6.4 (napari#5480)
  Refactor Main Window status bar to improve information presentation (napari#5451)
  ...
@Czaki Czaki mentioned this pull request Jun 7, 2023
@Czaki Czaki modified the milestones: 0.5.0, 0.4.18 Jun 16, 2023
@Czaki Czaki added the triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process label Jun 16, 2023
Czaki added a commit that referenced this pull request Jun 16, 2023
Czaki added a commit that referenced this pull request Jun 17, 2023
Czaki added a commit that referenced this pull request Jun 18, 2023
Czaki added a commit that referenced this pull request Jun 19, 2023
Czaki added a commit that referenced this pull request Jun 21, 2023
Czaki added a commit that referenced this pull request Jun 21, 2023
Czaki added a commit that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix PR with bugfix qt Relates to qt triaged-0.4.18 To mark PR that is triaged in 0.4.18 release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File > Open dialog doesn't work in pyside2
3 participants