-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Dark titlebar #13386
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
base: main
Are you sure you want to change the base?
Dark titlebar #13386
Conversation
This seems to be too much micromanagement here. as this needs to be maintained in future. |
Do not introduce custom monitoring, use proper bindings in javafx' platfrom api. |
FxTheme does the same and we would just introduce another dependency. |
Yes, but fxtheme is not maintained by us. Introducing a new library is in that case not necessary, if another library already supports what we need. But in this case, I have no interest in fixing changing apis and keeping it compatible to other OSs. That is what literally using an external library is meant for. |
jabgui/src/main/java/org/jabref/gui/desktop/os/ThemeListener.java
Outdated
Show resolved
Hide resolved
ListChangeListener<Window> windowsListener = change -> { | ||
while (change.next()) { | ||
if (!change.wasAdded()) { | ||
continue; | ||
} | ||
change.getAddedSubList().stream() | ||
.filter(Stage.class::isInstance) | ||
.map(Stage.class::cast) | ||
.forEach(stage -> { | ||
BindingsHelper.subscribeFuture(stage.showingProperty(), showing -> { | ||
if (showing) { | ||
applyDarkModeToWindow(stage, isDarkMode); | ||
} | ||
}); | ||
if (stage.isShowing()) { | ||
applyDarkModeToWindow(stage, isDarkMode); | ||
} | ||
}); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can maybe simplified with platform preferences api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the platform preferences API would work? The dark title bar must be applied to each new window, nor does the platform preferences API understand "use system theme" settings in the JabRef application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The platform preferences provide the binding colorSchemeProperty, which can be subscribed to, which notifies you every time the color scheme is changed in the system settings. Only thing left is if the user manually changes the theme in JabRef itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah, wait... Maybe there is no something I overlooked.... Right now I'm on my cellphone, I will take a deeper look in a few hours
@@ -49,13 +55,16 @@ public class ThemeManager { | |||
); | |||
|
|||
private static final Logger LOGGER = LoggerFactory.getLogger(ThemeManager.class); | |||
private static final boolean SUPPORTS_DARK_MODE = OS.WINDOWS || OS.OS_X; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please comment why it is not supported on Linux. Add a link. I think, KDE supports dark mode, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminded me that there is a thing they are working on https://bugs.openjdk.org/browse/JDK-8313424
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linux would automatically enforce a dark/light titlebar based on the system settings. The actual implementation of FXThemes on a Linux environment is a method that is empty. https://github.com/dukke/FXThemes/blob/main/FXThemes/src/main/java/com/pixelduke/window/LinuxThemeWindowManager.java
Would it be better for me to just have called the stub methods anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better for me to just have called the stub methods anyway
Yes, I think so. Maybe, there are some Window managers we never thought of supported? Icewm, dwm, ...
BindingsHelper.subscribeFuture(stage.showingProperty(), showing -> { | ||
if (showing) { | ||
applyDarkModeToWindow(stage, isDarkMode); | ||
} | ||
}); | ||
if (stage.isShowing()) { | ||
applyDarkModeToWindow(stage, isDarkMode); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplication can be solved with EasyBind::subscribe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure but i think this may cause a memory leak. if a window is closed, the reference is not removed and the garbage collector wont remove this. or am i wrong here? @Siedlerchr @koppor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yubo-Cao do not close converstations with questions raised that have not been answered. Maybe you already know, but I want to know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought the code was updated such that this duplication is not present, so this conversation is no longer relevant. But I think it's unlikely that we will have a memory leak, since this listener is anonymous, so the only reference to the window object would be inside the list of listeners that the window object creates for itself, which won't prevent the window from being recycled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For resolving just write a "done" or something like this if you addressed the comment
Didn't really make any difference so far, I also don't know if that should work on newer macs or not. No idea |
Co-authored-by: Subhramit Basu <subhramit.bb@live.in>
@trag-bot didn't find any issues in the code! ✅✨ |
JUnit tests of You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide. |
Closes #11457
Added a dark mode title bar for Windows.
Steps to test
A screenshot can be found here: https://imgur.com/a/aLDeZqg
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)