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

Un-set unified title and tool bar on mac (Qt property) #5533

Merged
merged 2 commits into from Feb 9, 2023

Conversation

aganders3
Copy link
Contributor

Description

This one is a bit weird, but here goes. This fixes the background "behind" the QtViewerDockWidgets being the wrong color (something from the system theme). I think this was actually the root cause of the issues seen in #5529 and addressed separately in #5526. I could go either way then on whether to include the changes in #5526 as well.

Anyway this removes a property that was set to fix an intermittent problem quite a while ago. It seems the behavior of this property actually changed in Qt5, and it is supposedly not compatible with windows that contain Open GL content.

Before this change

Screenshot 2023-02-06 at 12 12 18 PM

After this change

Screenshot 2023-02-06 at 12 24 58 PM

Type of change

  • Bug-fix (non-breaking change which fixes an issue)

References

#245
#239
#5529
#5526

How has this been tested?

  • I checked my changes work with both PySide and PyQt backends on Qt6, and PyQt on Qt5

Final checklist:

  • My PR is the minimum possible work for the desired functionality

@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Merging #5533 (b30dd7a) into main (ef76a46) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5533   +/-   ##
=======================================
  Coverage   89.36%   89.37%           
=======================================
  Files         608      608           
  Lines       51097    51096    -1     
=======================================
+ Hits        45664    45667    +3     
+ Misses       5433     5429    -4     
Impacted Files Coverage Δ
napari/_qt/qt_main_window.py 74.34% <ø> (-0.22%) ⬇️
napari/_qt/dialogs/qt_package_installer.py 81.81% <0.00%> (+0.39%) ⬆️
napari/utils/theme.py 94.04% <0.00%> (+0.59%) ⬆️
napari/utils/info.py 85.10% <0.00%> (+1.06%) ⬆️
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.

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Feb 6, 2023

For me this works perfectly and it's quite simple.
I wonder if this will mess something up on some sort of minimum requirements install?
But from reading the old threads, it's clear that the original issue wasn't easy to trigger, because none of the other mac users at the time had the issue.

One thing that makes this better is that the background when undocking is set by the napari theme and not OS theme. Now, I prefer to have napari and OS synced, but if you don't and you try to undock a widget it will look horrible. The other PR does not fix that on my end with pyside6.

Maybe @dalthviz has some insight, as original qss 🧙‍♂️ ?

@aganders3
Copy link
Contributor Author

One thing that makes this better is that the background when undocking is set by the napari theme and not OS theme.

Yeah this is what I set out to fix with this one.

I can go either way about including the changes from the other PR in addition. They still make sense to me, but I think with this change they're not strictly necessary. I'll happily defer to the resident Qt experts!

@brisvag
Copy link
Contributor

brisvag commented Feb 7, 2023

I wonder if this will mess something up on some sort of minimum requirements install?

We do have minreq in our tox matrix, so we should be good on that front :)

@dalthviz
Copy link
Member

dalthviz commented Feb 7, 2023

Maybe for an older Qt5 version like 5.6 or 5.9 or older MacOS versions was necessary to set that property for some reason? I think this solution makes sense 👍 (it's always nice to find a one liner solution for multiple problems and even better if it is done by removing code :) )

@psobolewskiPhD
Copy link
Member

Man these tests failing pyqt5/python 3.8 are a bummer :(
At least the dims ones would pass if rerun!
When do we drop python 3.8? 👀

@Czaki
Copy link
Collaborator

Czaki commented Feb 7, 2023

Man these tests failing pyqt5/python 3.8 are a bummer :(

this is not a problem with python 3.8 I observed it also on other python version.

@psobolewskiPhD psobolewskiPhD added Qt6 bugfix PR with bugfix labels Feb 8, 2023
@psobolewskiPhD
Copy link
Member

Based on the above approvals and the comment from @dalthviz I think this is ready to merge.
Starting the clock—I use pyside6 in my dev env, so this is a QoL issue for me!

@psobolewskiPhD psobolewskiPhD added the ready to merge Last chance for comments! Will be merged in ~24h label Feb 8, 2023
@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone Feb 8, 2023
@psobolewskiPhD psobolewskiPhD merged commit 6f7881f into napari:main Feb 9, 2023
@aganders3 aganders3 deleted the more-qt6-style branch February 9, 2023 21:25
@Czaki Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label Feb 18, 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 pushed a commit that referenced this pull request Jun 18, 2023
# Description
This one is a bit weird, but here goes. This fixes the background
"behind" the QtViewerDockWidgets being the wrong color (something from
the system theme). I think this was actually the root cause of the
issues seen in #5529 and addressed separately in #5526. I could go
either way then on whether to include the changes in #5526 as well.

Anyway this removes a property that was set to fix an intermittent
problem quite a while ago. It seems the behavior of this property
actually changed in Qt5, and it is supposedly not compatible with
windows that contain Open GL content.

### Before this change
![Screenshot 2023-02-06 at 12 12 18
PM](https://user-images.githubusercontent.com/1231828/217045566-eb330839-4496-4cb4-883e-2f58788ef68a.png)

### After this change
![Screenshot 2023-02-06 at 12 24 58
PM](https://user-images.githubusercontent.com/1231828/217045560-0d593d26-9cea-48ee-8838-864b8ef265fb.png)

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# References
#245
#239
#5529
#5526

# How has this been tested?
- [x] I checked my changes work with both PySide and PyQt backends on
Qt6, and PyQt on Qt5

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 19, 2023
# Description
This one is a bit weird, but here goes. This fixes the background
"behind" the QtViewerDockWidgets being the wrong color (something from
the system theme). I think this was actually the root cause of the
issues seen in #5529 and addressed separately in #5526. I could go
either way then on whether to include the changes in #5526 as well.

Anyway this removes a property that was set to fix an intermittent
problem quite a while ago. It seems the behavior of this property
actually changed in Qt5, and it is supposedly not compatible with
windows that contain Open GL content.

### Before this change
![Screenshot 2023-02-06 at 12 12 18
PM](https://user-images.githubusercontent.com/1231828/217045566-eb330839-4496-4cb4-883e-2f58788ef68a.png)

### After this change
![Screenshot 2023-02-06 at 12 24 58
PM](https://user-images.githubusercontent.com/1231828/217045560-0d593d26-9cea-48ee-8838-864b8ef265fb.png)

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# References
#245
#239
#5529
#5526

# How has this been tested?
- [x] I checked my changes work with both PySide and PyQt backends on
Qt6, and PyQt on Qt5

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 21, 2023
# Description
This one is a bit weird, but here goes. This fixes the background
"behind" the QtViewerDockWidgets being the wrong color (something from
the system theme). I think this was actually the root cause of the
issues seen in #5529 and addressed separately in #5526. I could go
either way then on whether to include the changes in #5526 as well.

Anyway this removes a property that was set to fix an intermittent
problem quite a while ago. It seems the behavior of this property
actually changed in Qt5, and it is supposedly not compatible with
windows that contain Open GL content.

### Before this change
![Screenshot 2023-02-06 at 12 12 18
PM](https://user-images.githubusercontent.com/1231828/217045566-eb330839-4496-4cb4-883e-2f58788ef68a.png)

### After this change
![Screenshot 2023-02-06 at 12 24 58
PM](https://user-images.githubusercontent.com/1231828/217045560-0d593d26-9cea-48ee-8838-864b8ef265fb.png)

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# References
#245
#239
#5529
#5526

# How has this been tested?
- [x] I checked my changes work with both PySide and PyQt backends on
Qt6, and PyQt on Qt5

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 21, 2023
# Description
This one is a bit weird, but here goes. This fixes the background
"behind" the QtViewerDockWidgets being the wrong color (something from
the system theme). I think this was actually the root cause of the
issues seen in #5529 and addressed separately in #5526. I could go
either way then on whether to include the changes in #5526 as well.

Anyway this removes a property that was set to fix an intermittent
problem quite a while ago. It seems the behavior of this property
actually changed in Qt5, and it is supposedly not compatible with
windows that contain Open GL content.

### Before this change
![Screenshot 2023-02-06 at 12 12 18
PM](https://user-images.githubusercontent.com/1231828/217045566-eb330839-4496-4cb4-883e-2f58788ef68a.png)

### After this change
![Screenshot 2023-02-06 at 12 24 58
PM](https://user-images.githubusercontent.com/1231828/217045560-0d593d26-9cea-48ee-8838-864b8ef265fb.png)

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# References
#245
#239
#5529
#5526

# How has this been tested?
- [x] I checked my changes work with both PySide and PyQt backends on
Qt6, and PyQt on Qt5

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 21, 2023
# Description
This one is a bit weird, but here goes. This fixes the background
"behind" the QtViewerDockWidgets being the wrong color (something from
the system theme). I think this was actually the root cause of the
issues seen in #5529 and addressed separately in #5526. I could go
either way then on whether to include the changes in #5526 as well.

Anyway this removes a property that was set to fix an intermittent
problem quite a while ago. It seems the behavior of this property
actually changed in Qt5, and it is supposedly not compatible with
windows that contain Open GL content.

### Before this change
![Screenshot 2023-02-06 at 12 12 18
PM](https://user-images.githubusercontent.com/1231828/217045566-eb330839-4496-4cb4-883e-2f58788ef68a.png)

### After this change
![Screenshot 2023-02-06 at 12 24 58
PM](https://user-images.githubusercontent.com/1231828/217045560-0d593d26-9cea-48ee-8838-864b8ef265fb.png)

## Type of change
- [x] Bug-fix (non-breaking change which fixes an issue)

# References
#245
#239
#5529
#5526

# How has this been tested?
- [x] I checked my changes work with both PySide and PyQt backends on
Qt6, and PyQt on Qt5

## Final checklist:
- [x] My PR is the minimum possible work for the desired functionality

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
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 Qt6 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.

Qt6: visual error: non-theme border around LayerControlsContainer and Console (maybe macOS only?)
5 participants