-
-
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
Changes from 7 commits
da017b6
b442fc8
40479ff
fbc6908
9e7bbb0
1f6e3e1
19d2c13
21dc53f
1f0b87a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,19 +13,25 @@ | |
|
||
import javafx.application.ColorScheme; | ||
import javafx.application.Platform; | ||
import javafx.collections.ListChangeListener; | ||
import javafx.scene.Node; | ||
import javafx.scene.Scene; | ||
import javafx.scene.web.WebEngine; | ||
import javafx.stage.Stage; | ||
import javafx.stage.Window; | ||
|
||
import org.jabref.gui.WorkspacePreferences; | ||
import org.jabref.gui.icon.IconTheme; | ||
import org.jabref.gui.util.BindingsHelper; | ||
import org.jabref.gui.util.UiTaskExecutor; | ||
import org.jabref.logic.l10n.Localization; | ||
import org.jabref.logic.os.OS; | ||
import org.jabref.model.util.FileUpdateListener; | ||
import org.jabref.model.util.FileUpdateMonitor; | ||
|
||
import com.google.common.annotations.VisibleForTesting; | ||
import com.pixelduke.window.ThemeWindowManager; | ||
import com.pixelduke.window.ThemeWindowManagerFactory; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
|
@@ -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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I think so. Maybe, there are some Window managers we never thought of supported? Icewm, dwm, ... |
||
|
||
private final WorkspacePreferences workspacePreferences; | ||
private final FileUpdateMonitor fileUpdateMonitor; | ||
private final Consumer<Runnable> updateRunner; | ||
private final ThemeWindowManager themeWindowManager; | ||
|
||
private final StyleSheet baseStyleSheet; | ||
private Theme theme; | ||
private boolean isDarkMode; | ||
|
||
private Scene mainWindowScene; | ||
private final Set<WebEngine> webEngines = Collections.newSetFromMap(new WeakHashMap<>()); | ||
|
@@ -66,9 +75,13 @@ public ThemeManager(WorkspacePreferences workspacePreferences, | |
this.workspacePreferences = Objects.requireNonNull(workspacePreferences); | ||
this.fileUpdateMonitor = Objects.requireNonNull(fileUpdateMonitor); | ||
this.updateRunner = Objects.requireNonNull(updateRunner); | ||
this.themeWindowManager = ThemeWindowManagerFactory.create(); | ||
|
||
this.baseStyleSheet = StyleSheet.create(Theme.BASE_CSS).get(); | ||
this.theme = workspacePreferences.getTheme(); | ||
this.isDarkMode = Theme.EMBEDDED_DARK_CSS.equals(this.theme.getName()); | ||
|
||
initializeWindowThemeUpdater(this.isDarkMode); | ||
|
||
// Watching base CSS only works in development and test scenarios, where the build system exposes the CSS as a | ||
// file (e.g. for Gradle run task it will be in build/resources/main/org/jabref/gui/Base.css) | ||
|
@@ -83,6 +96,58 @@ public ThemeManager(WorkspacePreferences workspacePreferences, | |
updateThemeSettings(); | ||
} | ||
|
||
private void initializeWindowThemeUpdater(boolean darkMode) { | ||
if (!SUPPORTS_DARK_MODE) { | ||
return; | ||
} | ||
|
||
this.isDarkMode = darkMode; | ||
|
||
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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 |
||
}); | ||
} | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 |
||
|
||
Window.getWindows().addListener(windowsListener); | ||
applyDarkModeToAllWindows(darkMode); | ||
|
||
LOGGER.debug("Window theme monitoring initialized"); | ||
} | ||
|
||
private void applyDarkModeToWindow(Stage stage, boolean darkMode) { | ||
if (!SUPPORTS_DARK_MODE || stage == null || !stage.isShowing()) { | ||
return; | ||
} | ||
calixtus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
themeWindowManager.setDarkModeForWindowFrame(stage, darkMode); | ||
LOGGER.debug("Applied {} mode to window: {}", darkMode ? "dark" : "light", stage); | ||
} | ||
|
||
private void applyDarkModeToAllWindows(boolean darkMode) { | ||
this.isDarkMode = darkMode; | ||
Window.getWindows().stream() | ||
.filter(Window::isShowing) | ||
.filter(window -> window instanceof Stage) | ||
.map(window -> (Stage) window) | ||
.forEach(stage -> applyDarkModeToWindow(stage, darkMode)); | ||
} | ||
|
||
private void updateThemeSettings() { | ||
Theme newTheme = Objects.requireNonNull(workspacePreferences.getTheme()); | ||
|
||
|
@@ -103,6 +168,12 @@ private void updateThemeSettings() { | |
this.theme = newTheme; | ||
LOGGER.info("Theme set to {} with base css {}", newTheme, baseStyleSheet); | ||
|
||
boolean isDarkTheme = Theme.EMBEDDED_DARK_CSS.equals(newTheme.getName()); | ||
if (this.isDarkMode != isDarkTheme) { | ||
this.isDarkMode = isDarkTheme; | ||
applyDarkModeToAllWindows(isDarkTheme); | ||
} | ||
|
||
this.theme.getAdditionalStylesheet().ifPresent( | ||
styleSheet -> addStylesheetToWatchlist(styleSheet, this::additionalCssLiveUpdate)); | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.