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 parent when creating layer context menu to inherit application theme and add style entry for disabled widgets and menus #5381

Merged
merged 10 commits into from Dec 22, 2022

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Nov 30, 2022

Description

Hi, this adds a missing parent kwarg so the layer context menu inherits the application theme (possible bug found while working on PR #5362). Also, add a style rule for disabled widgets (to for example show with a different style disabled entries on menus):

Preview

Layer context menu

Windows:

Before After
ctx_before ctx_menu_after_suggestion
ctx_light_before ctx_menu_after_light_suggestion

MacOS:

Before After
imagen imagen
imagen imagen

Ubuntu:

Before After
imagen imagen
imagen imagen

Application menus

Windows:

Before After
app_menu_before app_menu_afterapp_menu_qmodelmenu
app_menu_light_before app_menu_after_lightapp_menu_ligth_qmodelmenu

MacOS:

On macOS the application menus are integrated with the OS so the qss style doesn't change the way they look.

Ubuntu:

Before After
imagen imagenimagen
imagen imagenimagen

Type of change

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

References

How has this been tested?

  • all tests pass with my change
  • 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

@dalthviz dalthviz self-assigned this Nov 30, 2022
@dalthviz dalthviz marked this pull request as ready for review November 30, 2022 17:29
@github-actions github-actions bot added design Design discussion qt Relates to qt labels Nov 30, 2022
@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #5381 (522035e) into main (8656715) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #5381      +/-   ##
==========================================
+ Coverage   89.09%   89.11%   +0.01%     
==========================================
  Files         597      597              
  Lines       50614    50614              
==========================================
+ Hits        45095    45103       +8     
+ Misses       5519     5511       -8     
Impacted Files Coverage Δ
napari/_qt/containers/_layer_delegate.py 60.60% <0.00%> (ø)
napari/_qt/widgets/qt_viewer_status_bar.py 81.57% <0.00%> (-1.32%) ⬇️
napari/utils/theme.py 92.85% <0.00%> (+0.59%) ⬆️
napari/utils/info.py 81.44% <0.00%> (+1.03%) ⬆️
napari/_qt/__init__.py 56.66% <0.00%> (+6.66%) ⬆️
napari/components/experimental/chunk/_pool.py 93.65% <0.00%> (+7.93%) ⬆️

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

Czaki
Czaki previously approved these changes Nov 30, 2022
goanpeca
goanpeca previously approved these changes Nov 30, 2022
@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Nov 30, 2022

Ironically, because I use system for my napari I've never noticed the underlying issue.
However, for me this isn't working so well on macOS.
My OS is currently in dark mode, while napari has Light theme:
with this PR:
image
Without:
image
I think the old behavior doesn't look good, but is functional. The new behavior is hard to read.
Menus seem fine with this PR (I see no change in behavior vs main):
image

@brisvag
Copy link
Contributor

brisvag commented Dec 1, 2022

I also never noticed... @psobolewskiPhD maybe you could block this with a negative review to avoid premature merging, since you have a mac and can check future changes!

@Czaki
Copy link
Collaborator

Czaki commented Dec 1, 2022

It looks like something was missing from qss, but it looks like macOS specific problem.

@dalthviz dalthviz marked this pull request as draft December 1, 2022 16:34
@dalthviz
Copy link
Member Author

dalthviz commented Dec 1, 2022

Thanks for the feedback @psobolewskiPhD ! Will check on macOS to see what I can do 👍

@dalthviz dalthviz marked this pull request as ready for review December 2, 2022 17:38
@dalthviz
Copy link
Member Author

dalthviz commented Dec 2, 2022

Updated the screenshots testing on Windows, macOS and Ubuntu. The current approach sets a style that works as far as I tested on all three. Thinking on alternatives, another approach could be to only set the parent kwarg for the context menu on Windows and Linux (so the system style context menu is shown on macOS). Any further feedback, suggestions and checks are appreciated!

@dalthviz dalthviz dismissed stale reviews from goanpeca and Czaki December 2, 2022 17:46

Further changes were done

@psobolewskiPhD
Copy link
Member

psobolewskiPhD commented Dec 2, 2022

this PR Live
image image
image N/A it's system dark

I have a small preference for the live thinner border and the grey rather than black dividers — it's more native that way; see the menu screenshot above (note the border on macOS dark is also grey and not black). Not a blocker from me.
Edit: I prefer the darker look of the live Dark contextual menu as well, because it's more native, but that's in the eye of the beholder, so also not a blocker.

@dalthviz
Copy link
Member Author

dalthviz commented Dec 2, 2022

Thanks for the further feedback! Could you check with the new commit?

With them the context menu should look something like the following on macOS:

Screen Shot 2022-12-02 at 1 44 35 PM

Screen Shot 2022-12-02 at 1 45 52 PM

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
@dalthviz
Copy link
Member Author

dalthviz commented Dec 5, 2022

Trying to update the OP preview for the application menu, seems like the suggested style rules for QModelMenu doesn't apply to some application menus (like File) while for other menus is getting applied (like View):

File menu

imagen

View menu

imagen

Maybe some application menus are created in a different way than using QModelMenu/build_qmodel_menu ?

@psobolewskiPhD
Copy link
Member

I think it may be related to the app-model menu roll out.
Some menus have been converted to use app-model, like View: #4826
and also Help: #4922
All menus will eventually be migrated, so if View and Help look correct, then eventually everything will look fine. (on my mac, all the menus look the same—native—so I can't check.)

Also my suggestion may have been a bit reckless, not sure that the menu code makes sense there? At the very least maybe a comment is worth adding—sorry I neglected this.

@dalthviz
Copy link
Member Author

dalthviz commented Dec 6, 2022

Thanks for the explanation! Will check adding the extra comment/changing the position for the QModelMenu styles. Probably moving them near the QMenuBar styles section or to the 02_custom.qss makes more sense?

Again thanks for all the help, reviews and suggestion done here! :)

@dalthviz dalthviz changed the title Add parent when creating layer context menu to inherit application theme and add style entry for disabled widgets Add parent when creating layer context menu to inherit application theme and add style entry for disabled widgets and menus Dec 7, 2022
Copy link
Contributor

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @dalthviz 🚀

Copy link
Collaborator

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

Thanks for doing this :)

@psobolewskiPhD psobolewskiPhD merged commit f1c4071 into napari:main Dec 22, 2022
melissawm pushed a commit to melissawm/napari that referenced this pull request Jan 11, 2023
…eme and add style entry for disabled widgets and menus (napari#5381)

* Add parent when creating layer context menu to inherit application theme

* Remove opacity for disabled widgets background color style

* Set border-color for disabled widgets to background

* Set selection-background-color to transparent for disabled widgets

* Swap border-color and background-color for disabled widgets style

* Apply suggestions from code review

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

* Move QModelMenu styles to 02_custom styles

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
@Czaki Czaki mentioned this pull request Jun 7, 2023
@Czaki Czaki added this to the 0.4.18 milestone Jun 16, 2023
Czaki pushed a commit that referenced this pull request Jun 16, 2023
…eme and add style entry for disabled widgets and menus (#5381)

* Add parent when creating layer context menu to inherit application theme

* Remove opacity for disabled widgets background color style

* Set border-color for disabled widgets to background

* Set selection-background-color to transparent for disabled widgets

* Swap border-color and background-color for disabled widgets style

* Apply suggestions from code review

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

* Move QModelMenu styles to 02_custom styles

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 17, 2023
…eme and add style entry for disabled widgets and menus (#5381)

* Add parent when creating layer context menu to inherit application theme

* Remove opacity for disabled widgets background color style

* Set border-color for disabled widgets to background

* Set selection-background-color to transparent for disabled widgets

* Swap border-color and background-color for disabled widgets style

* Apply suggestions from code review

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

* Move QModelMenu styles to 02_custom styles

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 18, 2023
…eme and add style entry for disabled widgets and menus (#5381)

* Add parent when creating layer context menu to inherit application theme

* Remove opacity for disabled widgets background color style

* Set border-color for disabled widgets to background

* Set selection-background-color to transparent for disabled widgets

* Swap border-color and background-color for disabled widgets style

* Apply suggestions from code review

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

* Move QModelMenu styles to 02_custom styles

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 19, 2023
…eme and add style entry for disabled widgets and menus (#5381)

* Add parent when creating layer context menu to inherit application theme

* Remove opacity for disabled widgets background color style

* Set border-color for disabled widgets to background

* Set selection-background-color to transparent for disabled widgets

* Swap border-color and background-color for disabled widgets style

* Apply suggestions from code review

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

* Move QModelMenu styles to 02_custom styles

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 21, 2023
…eme and add style entry for disabled widgets and menus (#5381)

* Add parent when creating layer context menu to inherit application theme

* Remove opacity for disabled widgets background color style

* Set border-color for disabled widgets to background

* Set selection-background-color to transparent for disabled widgets

* Swap border-color and background-color for disabled widgets style

* Apply suggestions from code review

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

* Move QModelMenu styles to 02_custom styles

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 21, 2023
…eme and add style entry for disabled widgets and menus (#5381)

* Add parent when creating layer context menu to inherit application theme

* Remove opacity for disabled widgets background color style

* Set border-color for disabled widgets to background

* Set selection-background-color to transparent for disabled widgets

* Swap border-color and background-color for disabled widgets style

* Apply suggestions from code review

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

* Move QModelMenu styles to 02_custom styles

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>
Czaki pushed a commit that referenced this pull request Jun 21, 2023
…eme and add style entry for disabled widgets and menus (#5381)

* Add parent when creating layer context menu to inherit application theme

* Remove opacity for disabled widgets background color style

* Set border-color for disabled widgets to background

* Set selection-background-color to transparent for disabled widgets

* Swap border-color and background-color for disabled widgets style

* Apply suggestions from code review

Style for menu separators and padding

Co-authored-by: Peter Sobolewski <76622105+psobolewskiPhD@users.noreply.github.com>

* Move QModelMenu styles to 02_custom styles

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
design Design discussion enhancement qt Relates to qt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants