-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix a few theme-related issues #13877
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?
Conversation
… runtime - IndexOutOfBoundsException keeps being thrown although the theme is applied correctly. By catching the exception, we avoid spamming the exception to user.
Please don't squash before merging, as each commit addresses a separate issue. |
When I go to prefs -> Change from light to dark theme
|
I also experience the IndexOutOfBoundsException when I switch the theme first from light to dark then dark to light:
|
installCssImmediately(scene); | ||
} catch (IndexOutOfBoundsException e) { | ||
// IndexOutOfBoundsException keeps being thrown although the theme is applied correctly. | ||
// By catching the exception, we avoid spamming the exception to user. |
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 think you can actually catch the IndexOfOutBoundsException
... For some magic that I cannot fully understood, even with that, I can get an error similar to this:
java.lang.IndexOutOfBoundsException: toIndex = 3
at java.base/java.util.AbstractList.subListRangeCheck(AbstractList.java:509)
at java.base/java.util.AbstractList.subList(AbstractList.java:499)
at javafx.base@24.0.2/javafx.collections.ModifiableObservableListBase.subList(ModifiableObservableListBase.java:234)
at javafx.base@24.0.2/com.sun.javafx.binding.ContentBinding$ListContentBinding.onChanged(ContentBinding.java:123)
at javafx.base@24.0.2/com.sun.javafx.collections.ListListenerHelper$Generic.fireValueChangedEvent(ListListenerHelper.java:327)
at javafx.base@24.0.2/com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(ListListenerHelper.java:71)
at javafx.base@24.0.2/javafx.collections.ObservableListBase.fireChange(ObservableListBase.java:246)
at javafx.base@24.0.2/javafx.collections.ListChangeBuilder.commit(ListChangeBuilder.java:482)
at javafx.base@24.0.2/javafx.collections.ListChangeBuilder.endChange(ListChangeBuilder.java:541)
at javafx.base@24.0.2/javafx.collections.ObservableListBase.endChange(ObservableListBase.java:210)
at javafx.base@24.0.2/javafx.collections.ModifiableObservableListBase.setAll(ModifiableObservableListBase.java:102)
at org.jabref/org.jabref.gui.theme.ThemeManager.installCssImmediately(ThemeManager.java:108)
at org.jabref/org.jabref.gui.theme.ThemeManager.lambda$installCss$0(ThemeManager.java:117)
at javafx.graphics@24.0.2/com.sun.javafx.application.PlatformImpl.lambda$runLater$4(PlatformImpl.java:419)
at javafx.graphics@24.0.2/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
at javafx.graphics@24.0.2/com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
at javafx.graphics@24.0.2/com.sun.glass.ui.win.WinApplication.lambda$runLoop$0(WinApplication.java:168)
at java.base/java.lang.Thread.run(Thread.java:1447)
I wonder if DelayedExecution
utility, or first clear all the theme instead of directly setAll would perform the trick. Alternatively, the other method that I haven't tried before (when I encountered this), is to always keep the base.css and only remove the css not needed (so switch light to dark = add(dark.css); switch dark to light = remove(dark.css), instead of just performing an array of css replacement).
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 could fix this by calling clear first and then adding the new stylesheets
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 exception is less frequent now but still reproducible unfortunately.
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 think you can actually catch the IndexOfOutBoundsException... For some magic that I cannot fully understood, even with that, I can get an error similar to this:
I'm really confused too :), no idea how the exception is not caught. We have this setUncaughtExceptionHandler
method call to catch uncaught exceptions and show them in error dialog but that should be for uncaught 🥲.
I would keep it throwing since this is not a new bug and the theme changes correctly even with the exception, and open an issue to fix it later.
-fx-padding: 0.5em 1em 0.5em 1em; | ||
} | ||
|
||
.dialog-pane .button-bar .button { |
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.
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.
JabRef should be able to resize buttons to show full texts when font size changes but it is more important to have it work using the default font size, we can think of a better solution for that case in parallel.
The issue doesn't happen with the merge dialog only but with any dialog that has long text (more than two words) in button bar .i.e About dialog.
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 tried - worked on Gnome.
Exception no longer occurs after the latest changes. I think the only issue left was that with the icon size updating on font size change not always beeng correct |
…dated at runtime" This reverts commit 7361b5a.
Actually it still does for me. I didn't push any attempts to fix the exception, it is more involved. I reverted the commit that said it fixed this problem. I think we can merge as is, good enough for alpha release. |
@HoussemNasri @Yubo-Cao I merged the javafx25 now in, would be nice if could play around wtih this as well |
} | ||
|
||
/// Installs the base and additional css files as stylesheets in the given scene. | ||
/// Installs the base and additional CSS files as stylesheets in the given scene. |
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 comment is trivial and restates the method name. Comments should provide additional context or reasoning not obvious from the code itself.
stylesheets.addAll(baseOrThemeStylesheet); | ||
} | ||
|
||
/// Registers a runnable on JavaFX thread to install the base and additional css files as stylesheets in the given scene. |
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 comment is trivial and restates the method name. Comments should provide additional context or reasoning not obvious from the code itself.
JavaFX default theme brief flash
Screen.Recording.2025-09-13.at.14.53.57.mov
Button-bar button ellipsis truncate issue
Mandatory checks
CHANGELOG.md
in a way that is understandable for the average user (if change is visible to the user)