-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[WIP] Make dark theme selectable in preferences #4313
Conversation
…ePrefsTAb, ThemeLoader, and added the css files PreferencesDialog.css, DarkPrefBox.css. Method in AppearancePrefTab basically swaps both the css files with each other and sets them up.Since I noticed that there was a function that actively reads the base css file, I set it up this way. Once you restart the application, the dark theme is a ctivated. Additionally, I noticed that there wasn't really a css for the preferences box so I created a simple css using the colors already implemented in the dark theme
…ePrefsTab, ThemeLoader, and added the css files PreferencesDialog.css, DarkPrefBox.css. Method in AppearancePrefTab basically swaps both the css files with each other and sets them up. Since I noticed that there was a function that actively reads the base css file, I set it up this way. Once you restart the application, the dark theme is activated. Additionally, I noticed that there wasn't really a css for the preferences box so I created a simple css using the colors already implemented in the dark theme
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.
Thanks for your contribution! 👍
I noted some issues which need to be improved. And I don't understand why you need to replace the css files with the copy operation. Maybe you can explain to me why it's necessary
|
||
} | ||
|
||
private void copy(File file1,File file2) throws IOException { |
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.
You can use the copy method from FileUtil
@@ -64,6 +64,7 @@ private static FXDialog createDialog(AlertType type, String title, String conten | |||
alert.setHeaderText(null); | |||
alert.setContentText(content); | |||
alert.getDialogPane().setMinHeight(Region.USE_PREF_SIZE); | |||
alert.showAndWait(); |
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 is not a good idea to add it here as is the internal method for creating a dialog. Normally you call one of the public method from the Dialog Service, get a dialog object back and then call show and wait on that object. Just look in other places how it's done
|
||
if(accessDarkTheme.isSelected()==true){ | ||
|
||
File basecaseFile= new File(DEFAULT_PATH_MAIN_CSS); |
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 use the nio Path methods here. Have a look at the exporter tests to see how to access a resource file
if(accessDarkTheme.isSelected()==true){ | ||
|
||
File basecaseFile= new File(DEFAULT_PATH_MAIN_CSS); | ||
File darkthemecssFile= new File(DARK_THEME_PATH_CSS); |
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 copy and replace is necessary. I thought it's just pointing the JavaFX loader to the new path 🤔
@@ -51,6 +55,9 @@ public ThemeLoader(FileUpdateMonitor fileUpdateMonitor) { | |||
*/ | |||
public void installBaseCss(Scene scene, JabRefPreferences preferences) { | |||
addAndWatchForChanges(scene, DEFAULT_PATH_MAIN_CSS, 0); | |||
addAndWatchForChanges(scene,DARK_THEME_PATH_CSS,0); |
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.
It should work without copying (and modifying) the css files if you add here something like:
if (preferences.useDarkTheme) {
addAndWatchForChanges(scene,DARK_THEME_PATH_CSS, 1);
}
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.
Do we really need to watch for changes? Maybe just have a second method which just adds/loads the css and doesn't watch for changes.
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.
It only watches for changes if the corresponding command line flag is passed, i.e. only for us developers.
I would rather habe a dropdown to select the two different styles than a checkbox for the dark theme. |
Cool I'll look at everyone's advice and suggestions and try to fix it. |
#4372 was a bit quicker to implement this. Sorry! |
This pull request allows the user to pick the dark theme option in the preferences box. After the user restarts the application the UI would have the dark theme in it. I also added the dark theme in the preferences box. An critique on my coding will be greatly appreciated.