Skip to content

Options panel use GTK default colors - #2744

Merged
lkishalmi merged 1 commit into
apache:masterfrom
hectorespert:options_panel_gtk
Dec 18, 2021
Merged

Options panel use GTK default colors#2744
lkishalmi merged 1 commit into
apache:masterfrom
hectorespert:options_panel_gtk

Conversation

@hectorespert

@hectorespert hectorespert commented Feb 7, 2021

Copy link
Copy Markdown
Member

Use default colors in options panel when GTK laf is used.

Tested in Ubuntu 20.04 with default theme.

Before:
Screenshot from 2021-02-07 22-39-09

After:
Screenshot from 2021-02-07 22-40-04

@hectorespert
hectorespert marked this pull request as ready for review February 8, 2021 06:37
@hectorespert hectorespert added this to the 12.4 milestone Feb 9, 2021
@neilcsmith-net

Copy link
Copy Markdown
Member

Sorry, -1 from me on this. Although +1 to fixing it. We can't keep adding exceptions for look and feels over the code base. Better to fix this so that a single key works for all LAFs. If separator.foreground doesn't work for all, maybe a specific key for this panel that does?

@hectorespert
hectorespert marked this pull request as draft February 23, 2021 17:41
@hectorespert
hectorespert force-pushed the options_panel_gtk branch 2 times, most recently from 8339568 to f57210f Compare February 23, 2021 21:28
@hectorespert
hectorespert marked this pull request as ready for review February 23, 2021 21:30
@hectorespert

Copy link
Copy Markdown
Member Author

Sorry, -1 from me on this. Although +1 to fixing it. We can't keep adding exceptions for look and feels over the code base. Better to fix this so that a single key works for all LAFs. If separator.foreground doesn't work for all, maybe a specific key for this panel that does?

I added a keys to modify this panel to allow remove LAFs exceptions but this PR only removes exceptions for the GTK laf.
I can't test other LAFs and they could be modified later.

@lkishalmi

Copy link
Copy Markdown
Contributor

A step in the good direction, though if we are adding new LAF keys, probably shall add constants for it in LFCustoms.
And by introducing that it would be good to check if all supported LAF-s are providing defaults for the new key.

A sidenote: It would be really great if you could detect somehow when the GTK skin is set to dark, if it does, it would be great at least add the "nb.dark.theme" to true.

@hectorespert

Copy link
Copy Markdown
Member Author

A step in the good direction, though if we are adding new LAF keys, probably shall add constants for it in LFCustoms.

I added the new constants in LFCustoms.

And by introducing that it would be good to check if all supported LAF-s are providing defaults for the new key.

New keys are only used by GTK theme, I would like to fix the problem with that theme before start to modify the other themes.

A sidenote: It would be really great if you could detect somehow when the GTK skin is set to dark, if it does, it would be great at least add the "nb.dark.theme" to true.

A GTK configuration property to detect that is available: https://developer.gnome.org/gtk3/3.8/GtkSettings.html#GtkSettings--gtk-application-prefer-dark-theme

@geertjanw

Copy link
Copy Markdown
Member

Changing milestone to 12.5.

@geertjanw geertjanw modified the milestones: 12.4, 12.5 Apr 16, 2021
@hectorespert
hectorespert force-pushed the options_panel_gtk branch 2 times, most recently from 995895b to d4278b2 Compare April 17, 2021 09:10
@ebarboni

Copy link
Copy Markdown
Contributor

@neilcsmith-net @lkishalmi are the changes better now ? Can we consider merging ?

@lkishalmi lkishalmi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Generally I'm Ok with the implementation, just those constants...

Comment thread platform/o.n.swing.plaf/src/org/netbeans/swing/plaf/LFCustoms.java Outdated
Comment thread platform/options.api/src/org/netbeans/modules/options/OptionsPanel.java Outdated
@ebarboni

Copy link
Copy Markdown
Contributor

change resquested moving to 12.6 for now

@ebarboni ebarboni modified the milestones: 12.5, 12.6 Jul 23, 2021
@hectorespert
hectorespert force-pushed the options_panel_gtk branch 2 times, most recently from 7c590fb to 75f91b5 Compare October 16, 2021 10:10
@neilcsmith-net

Copy link
Copy Markdown
Member

@lkishalmi please review and merge if you're happy.

@neilcsmith-net neilcsmith-net modified the milestones: 12.6, NB13 Oct 22, 2021
@matthiasblaesing

Copy link
Copy Markdown
Contributor

This seems to be a working implementation, which even looks like a cleanup. Any reason not to merge now? (the unittests that fail are known to be flaky).

@hectorespert

Copy link
Copy Markdown
Member Author

Any reason not to merge now?

It's only waiting for a positive review.

@lkishalmi lkishalmi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems to be fine now.

@lkishalmi
lkishalmi merged commit 418b01c into apache:master Dec 18, 2021
@hectorespert
hectorespert deleted the options_panel_gtk branch December 18, 2021 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants