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

Filter out unsupported Look and Feels from UI #7832

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

neilcsmith-net
Copy link
Member

We've previously talked in other issues about the various JDK Look and Feels no longer really being supported. All cause varying degrees of issue - see eg. discussion at #7799 (comment)

This PR filters these Look and Feels out from the Appearance options UI, while still allowing them to be set using --laf option.

The first commit allows filtering by a brandable list. This is empty by default to allow platform applications to make their own choices.

The second commit filters the options in the IDE branding. I've started with filtering all (my preference), but perhaps could filter just the most problematic (esp. GTK)??

@neilcsmith-net neilcsmith-net added Look and Feel UI User Interface ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Oct 4, 2024
@neilcsmith-net neilcsmith-net added this to the NB24 milestone Oct 4, 2024
@eirikbakke
Copy link
Contributor

I think we should keep at least the default Swing LAFs for each OS in the list, per UIManager.getSystemLookAndFeelClassName(). On Windows and MacOS those are WindowsLookAndFeel and AquaLookAndFeel, respectively. Both of those work quite well with NetBeans.

As for GTKLookAndFeel, does it not work quite well when the "-J-Djdk.gtk.version=2.2" setting is enabled? I see this setting was modified by https://issues.apache.org/jira/browse/NETBEANS-5862

@neilcsmith-net
Copy link
Member Author

neilcsmith-net commented Oct 4, 2024

I think we should keep at least the default Swing LAFs for each OS in the list, per UIManager.getSystemLookAndFeelClassName(). On Windows and MacOS those are WindowsLookAndFeel and AquaLookAndFeel, respectively. Both of those work quite well with NetBeans.

Sure, we could remove both of those from the IDE branded filter string. They only show up on the right OS anyway. Aqua has certainly been a cause of issues (eg. #5861 ), and possibly the Windows one too (eg. #5697 ?). I'm personally in favour of removing them from the UI, but can remove from the excludes if other people think we should keep them too?

Thanks! Will handle the immediate changes.

@eirikbakke
Copy link
Contributor

I'm personally in favour of removing them from the UI, but can remove from the excludes if other people think we should keep them too?

What's the rationale for removing them? Maybe a topic for the mailing list.

@neilcsmith-net
Copy link
Member Author

What's the rationale for removing them? Maybe a topic for the mailing list.

The comment linked in the description about filtering out some LaFs on #7799 was one of a few about filtering some or all of the legacy LaFs. We have already stated they're unsupported. I'm not personally in favour of having unsupported or broken things in the UI. I'm happy for people to argue to keep them visible, as long as they're also going to engage with things like the two issues in my previous comment.

I worked on this now, prompted by @mbien comment, because I could do with the filtering functionality in the platform anyway. This doesn't affect platform applications unless they decide to use it though.

Have added some protection for a NPE with non-installed current LaF. (eg. tested setting config/Preferences/laf.properties to laf=com.formdev.flatlaf.FlatDarculaLaf) This is somewhat broken in current use, more so with the filtering in place.

@eirikbakke
Copy link
Contributor

We have already stated they're unsupported.

Have we? I have been updating both of these LAFs in the past years, adding HiDPI support etc...

I also mentioned some reasons to keep them, in that other discussion thread.

@mbien
Copy link
Member

mbien commented Oct 4, 2024

i think it would be good to keep one cross platform LAF which is not FlatLaf (Metal?). This is often useful to debug issues, e.g like the recent popup issue 2c99e0f. E.g I remember telling @cstamas to quickly switch to metal (which fixed the issue for him).

I agree GTK has to go, it has problems no matter what impl is used.

Having a branded filter is great, however I am wondering if we could add a UI tweak:

  • UI shows only FlatLaf based LAFs since this is the only LAF we test/support for NB
  • there is a "Show unsupported Look and Feels" checkbox which would unlock the unfiltered list

this would communicate to the user what is supported and what is not, while still giving all options when safeties are off.

edit: impl via inverted filter? only list supported LAF in the filter? Empty means all LAFs?

@neilcsmith-net
Copy link
Member Author

neilcsmith-net commented Oct 4, 2024

I considered that UI tweak but it would require a bit more recoding of the UI as it's not well set up for the list to be dynamic - was beyond the 30min I had for it this morning anyway! It would also need the switch to be hidden when no filters are set, or switched off by another branding switch. The IDE might want to allow access to unsupported LaFs, while platform apps might not.

For debugging purposes people can also run netbeans --laf Metal or netbeans --laf javax.swing.plaf.nimbus.NimbusLookAndFeel (straight Nimbus should work but it's the wrong class name at https://github.com/apache/netbeans/blob/master/platform/core.startup/src/org/netbeans/core/startup/CLIOptions.java#L152 ) This can also be done in the conf file.

It's also possible to manually edit <userdir>/config/Preferences/laf.properties. eg. laf=javax.swing.plaf.metal.MetalLookAndFeel.

That's not an argument for or against. Happy to change the list as required, or just make it GTK for now.

@mbien
Copy link
Member

mbien commented Oct 4, 2024

@neilcsmith-net tried to implement it via 844a4aa (on top of this PR) (feel free to squash into your PR if you like)

@eirikbakke
Copy link
Contributor

i think it would be good to keep one cross platform LAF which is not FlatLaf (Metal?). This is often useful to debug issues

Good idea!

I'd also prefer that the Windows and Aqua LAFs remain available, since (1) they are the only LAFs that use the widget style of their respective native platforms (through actual rendering API calls to Win32 and Aqua, respectively), (2) they are common in Swing apps, and (3) they still work well with NetBeans.

I do like the "Show unsupported LAFs" checkbox idea. But--as a general rule I think we should alert/consult the mailing list before declaring existing features as unsupported or deprecated, in case people have objections.

Alternatively the checkbox could be inverted and be called "Show recommended LAFs only."

@mbien
Copy link
Member

mbien commented Oct 4, 2024

I'd also prefer that the Windows and Aqua LAFs remain available, since (1) they are the only LAFs that use the widget style of their respective native platforms (through actual rendering API calls to Win32 and Aqua,

NB on win laf doesn't look horrible, but it isn't quite production ready. It uses some of the native widgets and when it doesn't you can spot it right away. The interesting aspect about using native windows components is also that they don't look native. (thats also why eclipse always looked like eclipse since SWT never looked native). (I had some larger eclipse RCP projects in early days of SWT). I have NB using win laf open on win 10 and 11 in vbox and context menus etc all look different from edge or system setting windows.

The windows file chooser "shell folder" integration is also still broken even on latest JDKs #3919 (comment). FlatLaf file chooser not only works, we also get NB favorites integration for free.

I can't comment on aqua, thought it doesn't exist anymore - I have no mac available atm.

However, some might want to design forms in the target LAF and set NB to that LAF + it might be useful for debugging issues as previously mentioned. So I don't have anything against allowing (+making it easy to switch to) unsupported LAFs if that is properly communicated. (-> checkbox proposal)

Alternatively the checkbox could be inverted and be called "Show recommended LAFs only."

I thought about using "recommended" but this sounds too much like an endorsement, what is meant is that NB is only tested on FlatLaf. But since the other checkbox is already super vague (you basically have to read the tooltip to figure out what it might do), I suppose we could simply use "Show other look and feels" and the tooltip could say that "some may not be supported".

But if others do like "recommended" I would be fine with it, esp when the tooltip clarifies what it actually means.

But--as a general rule I think we should alert/consult the mailing list

I don't have another dev list thread in me right now, but feel free to open one if you want.

@neilcsmith-net
Copy link
Member Author

neilcsmith-net commented Oct 5, 2024

Thanks @mbien I've pulled your change in, fixed a couple of issues with current index and forced laf (eg. --laf Metal).

I've updated the label to use the word "all" rather than "unsupported", and moved mention of not recommended or supported in to the tooltip. Hopefully this addresses @eirikbakke concerns? I'm definitely not inverting the toggle, as it completely removes the reason for adding this in the first place - to discourage people from using these Look and Feels (amongst other things, to cut down on issue reports where we're telling people to go back to the recommended LAF because no-one is going to fix the problem!).

I've added a branding toggle for the all-lafs checkbox. Without that I'm back to square one with writing my own LAF panel implementation for a platform application, which I was trying to get away from by working on this for the IDE too.

However, some might want to design forms in the target LAF

Incidentally, right-click the component (eg. JPanel here) in the designer or Navigator. Preview Design allows you to at least choose a LAF for preview, if not for the design tab. Not everything works correctly mind you.

@eirikbakke
Copy link
Contributor

I've updated the label to use the word "all" rather than "unsupported", and moved mention of not recommended or supported in to the tooltip. Hopefully this addresses @eirikbakke concerns?

Yes, that's fine! Thanks and sorry for jumping to the defense of old features. :-)

I'm definitely not inverting the toggle, as it completely removes the reason for adding this in the first place - to discourage people from using these Look and Feels

Oh, I meant invert the default as well. It was just in case it made it easier to come up with a good wording. In any case, thanks for the checkbox work!

NB on win laf doesn't look horrible, but it isn't quite production ready.

I think it was the default NetBeans LAF on Windows in the years 2012-2021.

@mbien
Copy link
Member

mbien commented Oct 5, 2024

I think it was the default NetBeans LAF on Windows in the years 2012-2021.

right, until we agreed to switch to a cross platform LAF to simplify maintenance and due to various problems of system specific LAFs (some of them I already mentioned), including the win laf.

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

code looks good + tested it too and it worked great.

Feel free to squash my commit into the rest if you want - no need to pollute the history ;)

@neilcsmith-net
Copy link
Member Author

Thanks! I'll tidy up the commits as soon as I get a chance, and look to merge early next week unless there are any further comments.

neilcsmith-net and others added 2 commits October 6, 2024 10:56
- add a branding option LafPanel.excludes to hide installed LAFs in combobox
- add optional checkbox to show all installed LAFs
- switched LafPanel layout to group layout and tweaked UI

Co-authored-by: Michael Bien <mbien42@gmail.com>
- filter out the JDK and dark NB LAFs by default in the LAF panel.
- enable the checbox toggle to show all LAFs, including not recommended or unsupported.
@neilcsmith-net neilcsmith-net merged commit 02e44d8 into apache:master Oct 7, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Look and Feel UI User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants