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

FlatLaf: Enable Window decoration option for Linux #6391

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

demitriusbelai
Copy link
Contributor

@demitriusbelai demitriusbelai commented Aug 30, 2023

Hi! This enable window decoration option for system that not support Native Window Decorations (Linux). This can be enable in Tools > Options > Appearance > FlatLaf > Window decorations.

See: https://www.formdev.com/flatlaf/window-decorations/#enable_disable_linux

@mbien mbien added UI User Interface ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Aug 30, 2023
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

My first reaction: nice. Thank you for working on this. The System.out calls have to go though.

A first quick test was successful and even switching at runtime seems to be working. Review from the right people is already requested and I hope that they agree, that this the right direction.

@demitriusbelai demitriusbelai force-pushed the flatlaf-window-decoration branch 2 times, most recently from 2ea6891 to cacb91c Compare September 2, 2023 01:29
@mbien mbien added the Platform [ci] enable platform tests (platform/*) label Sep 2, 2023
@mbien
Copy link
Member

mbien commented Sep 2, 2023

looks nice!
image

When testing fullscreen mode (shift+alt+enter), I sometimes didn't have any window controls on the title bar and the main window was not movable/resizable. Toggling it a few times more fixed it again. Might be a race condition? Can't reproduce this with NB 19.
image

Copy link
Member

@DevCharly DevCharly left a comment

Choose a reason for hiding this comment

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

Tested this on Windows (with some modifications) because I don't have a NB dev env on Linux. Works fine 👍

Anyway there are two questions:

  • Option "FlatLaf Window decorations" is selected by default. Also on Linux. This means after merging this PR, NetBeans (and NB platform apps) use this by default on Linux. Is this wanted? Or should it be disabled on Linux by default?
  • Should this also enabled for JDialogs on Linux? Currently it is only enabled for JFrames.

We also should take care on NB platform apps that already use FlatLaf window decorations on Linux. I know that @rkeen-siemens has one in his company. @rkeen-siemens would this PR cause problems for your app?

@demitriusbelai
Copy link
Contributor Author

@DevCharly It should be disabled on Linux by default. Visual Studio Code disable it too: microsoft/vscode#65608. How can I change this?

@DevCharly
Copy link
Member

How can I change this?

In class FlatLafPrefs the parameter true here:

return prefs.getBoolean(USE_WINDOW_DECORATIONS, true);

and here

I would suggest to add a field to that class and use it instead of passing true to get/putBoolean(). E.g.:

private static final boolean DEF_USE_WINDOW_DECORATIONS = SystemInfo.isWindows_10_orLater;

static boolean isUseWindowDecorations() {
    return prefs.getBoolean(USE_WINDOW_DECORATIONS, DEF_USE_WINDOW_DECORATIONS);
}

static void setUseWindowDecorations(boolean value) {
    putBoolean(USE_WINDOW_DECORATIONS, value, DEF_USE_WINDOW_DECORATIONS);
}

Copy link
Member

@DevCharly DevCharly left a comment

Choose a reason for hiding this comment

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

👍

@mbien mbien added this to the NB20 milestone Sep 6, 2023
@mbien
Copy link
Member

mbien commented Sep 6, 2023

great! @demitriusbelai can you squash everything and force push into the PR branch? The resulting commit will be merged as-is.

Since PRs are linked from the release notes, I think it would be nice here to describe in the PR text where to find the option (tools->options->appearance->flatlaf->window decorations).

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Sep 7, 2023

@DevCharly the other thing to check with @rkeen-siemens is that the quick patch (#6294) I had to make to his dialog fix hasn't caused the original issue to reappear.

See also the original issue report at #5987 to see why it might be of concern here.

Copy link
Contributor

@eirikbakke eirikbakke left a comment

Choose a reason for hiding this comment

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

Do I understand it correctly that the code in this PR will only run if the new setting is explicitly turned on? If so, it seems low-risk.

@demitriusbelai
Copy link
Contributor Author

@mbien Sorry for the delay. I did squash and rebase.

@demitriusbelai
Copy link
Contributor Author

Guys! I remove JDialog.setDefaultLookAndFeelDecorated(true) from MR because Generate code popup was showing with decoration:

Screenshot_20230917_021729

I chaged PopupUtil and It worked:

--- a/ide/editor/src/org/netbeans/modules/editor/codegen/PopupUtil.java
+++ b/ide/editor/src/org/netbeans/modules/editor/codegen/PopupUtil.java
@@@ -38,6 -38,6 +38,7 @@@ import javax.swing.AbstractAction
  import javax.swing.Action;
  import javax.swing.JComponent;
  import javax.swing.JDialog;
++import javax.swing.JRootPane;
  import javax.swing.KeyStroke;
  import javax.swing.SwingUtilities;
  import org.openide.windows.WindowManager;
@@@ -81,6 -81,6 +82,7 @@@ public final class PopupUtil  
          popupWindow.setUndecorated(undecorated);
          popupWindow.getRootPane().getInputMap( JComponent.WHEN_ANCESTOR_OF_FOCUSED_COMPONENT ).put( ESC_KEY_STROKE, CLOSE_KEY );
          popupWindow.getRootPane().getActionMap().put( CLOSE_KEY, CLOSE_ACTION );
++        popupWindow.getRootPane().setWindowDecorationStyle(JRootPane.NONE);
        
        //set a11y
        String a11yName = content.getAccessibleContext().getAccessibleName();

However, I looked for setUndecorated in source code and found a lot of. So it is better keep JDialog untouched.

@mbien
Copy link
Member

mbien commented Sep 22, 2023

how does this behave on other systems, do the window decoration changes take immediate effect when the option is applied?

we should probably trigger the restart notification bubble (this might need a isLinux() check if the restart is unique to linux):

diff --git a/platform/o.n.swing.laf.flatlaf/src/org/netbeans/swing/laf/flatlaf/FlatLafOptionsPanel.java b/platform/o.n.swing.laf.flatlaf/src/org/netbeans/swing/laf/flatlaf/FlatLafOptionsPanel.java
index f95768a..436c6d8 100644
--- a/platform/o.n.swing.laf.flatlaf/src/org/netbeans/swing/laf/flatlaf/FlatLafOptionsPanel.java
+++ b/platform/o.n.swing.laf.flatlaf/src/org/netbeans/swing/laf/flatlaf/FlatLafOptionsPanel.java
@@ -53,6 +53,7 @@
 
     private static final Color DEFAULT = new Color(0, true);
     private static final Color currentAccentColor = FlatLafPrefs.getAccentColor();
+    private static final boolean currentUseWindowDecorations = FlatLafPrefs.isUseWindowDecorations();
 
     private static final RequestProcessor RP = new RequestProcessor(FlatLafOptionsPanel.class);
 
@@ -341,7 +342,7 @@
         FlatLafPrefs.setUnderlineMenuSelection(underlineMenuSelectionCheckBox.isSelected());
         FlatLafPrefs.setAlwaysShowMnemonics(alwaysShowMnemonicsCheckBox.isSelected());
 
-        if (!Objects.equals(accentColor, currentAccentColor)) {
+        if (!Objects.equals(accentColor, currentAccentColor) || useWindowDecorationsCheckBox.isSelected() != currentUseWindowDecorations) {
             askForRestart();
         }
         return false;

@demitriusbelai
Copy link
Contributor Author

how does this behave on other systems, do the window decoration changes take immediate effect when the option is applied?

On Windows it do not need restart.

we should probably trigger the restart notification bubble (this might need a isLinux() check if the restart is unique to linux):

My fault. NetBeans was already asking for restart but now I know it is a bug when AccentColor is set DEFAULT. I will try to explain.

When AccentColor is set DEFAULT, it is saved as 'null'. So NetBeans launch FlatLatOptionsPanel:

  • the static variable currentAccentColor is NULL
  • getPrefsAccentColorOrDefault() returns DEFAULT
  • accentColorField.setSelectedColor(getPrefsAccentColorOrDefault()) => DEFAULT

When store():

  • accentColor = accentColorField.getSelectedColor() => DEFAULT
  • accentColor is always not equals currentAccentColor (DEFAULT != null), then it always trigger askForRestart() when FlatLafPanel is changed.

It can be fixed:

diff --git a/platform/o.n.swing.laf.flatlaf/src/org/netbeans/swing/laf/flatlaf/FlatLafOptionsPanel.java b/platform/o.n.swing.laf.flatlaf/src/org/netbeans/swing/laf/flatlaf/FlatLafOptionsPanel.java
index 9c47c6c60e..da5eaca8d8 100644
--- a/platform/o.n.swing.laf.flatlaf/src/org/netbeans/swing/laf/flatlaf/FlatLafOptionsPanel.java
+++ b/platform/o.n.swing.laf.flatlaf/src/org/netbeans/swing/laf/flatlaf/FlatLafOptionsPanel.java
@@ -53,7 +53,7 @@ import org.openide.util.RequestProcessor;
 public class FlatLafOptionsPanel extends javax.swing.JPanel {
 
     private static final Color DEFAULT = new Color(0, true);
-    private static final Color currentAccentColor = FlatLafPrefs.getAccentColor();
+    private static final Color currentAccentColor = FlatLafPrefs.getAccentColor() != null ? FlatLafPrefs.getAccentColor() : DEFAULT;
 
     private static final RequestProcessor RP = new RequestProcessor(FlatLafOptionsPanel.class);

As soon as possible I will fix the PR.

@demitriusbelai
Copy link
Contributor Author

Done! I put the fix for currentAccentColor together. Let me know if I need to make a new PR for that.

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 to me + tested on linux

thanks for your contribution - looking forward to using it in the next release

@mbien mbien added the os:linux label Sep 30, 2023
@mbien
Copy link
Member

mbien commented Oct 5, 2023

if everyone is ok with this I will press merge this weekend. Lets get some PRs in before feature freeze mid-October.

@mbien
Copy link
Member

mbien commented Oct 6, 2023

no complaints -> merging

@mbien mbien merged commit 0a618c9 into apache:master Oct 6, 2023
34 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) os:linux Platform [ci] enable platform tests (platform/*) UI User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants