-
Notifications
You must be signed in to change notification settings - Fork 145
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
New options dialog #173
New options dialog #173
Conversation
Sync with properties file Extract property keys
Add missing labels
Hide old options dialog
Fix bug for temporary load solution Improve save test function
Add support for storing/retrieving preferences
Fix labels
Fix label Show values for notice options
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.
Looks really good! Just a couple of really minor points.
Quelea/src/main/java/org/quelea/services/languages/LabelGrabber.java
Outdated
Show resolved
Hide resolved
@@ -67,13 +67,6 @@ public void run() { | |||
if (thisDeviceCount > lastDeviceCount && QueleaProperties.get().getUseAutoExtend()) { | |||
QueleaProperties.get().setProjectorModeScreen(); | |||
QueleaProperties.get().setProjectorScreen(thisDeviceCount - 1); | |||
Platform.runLater(new Runnable() { |
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.
Ah, I remember this fudge! Did you manage to solve the underlying cause somewhere else?
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.
Oh right, I forgot about double checking that out when I deleted the code that I had commented out. I added this code in when I made the advanced feature to automatically extend the screen to a connected device and when I looked at the code it seemed like all it did was to update the preference dialog and I figured that it is done automatically when it's re-opened now. However, now that you mention it, I realize that was made to trigger the update of which screen to use. I'll have a look at it later and see if I could find a better workaround or if we should do something similar again.
Quelea/src/main/java/org/quelea/windows/options/DisplayGroup.java
Outdated
Show resolved
Hide resolved
If you could take a look at the Codacy report as well and address those issues, that'd be fab. Looks like most of them shouldn't take long to sort out at all. |
I've look through the output from Codacy now. It seems IntelliJ and PMD have different views on explicit scoping based on their suggestions, but I've changed those to be explicit now. As for converting variables to local variables, I decided that it would be better for future additions/changes to have all the setting variables setup the same way in all the classes even if they're not all accessed from outside of the class/method at the moment. If you feel it's better to go for local variables where it's possible, we could of course do that as well. Regarding the long method "saveObject" in PreferenceStorageHandler, I'm not sure if I feel spitting it up improves the readability. Of course, if you have a better suggestion than the switch approach, do let me know. |
Thanks - yup, agree with that. Now merged in - thanks! 👍 |
I try the new options window, good job. |
FYI, the bug in the library that made the preference dialog open up an empty category by default is now resolved. The first view now should be the first subcategory in the General tab. (See this PR for more information about the approach.) |
@ArvidNy Fantastic, thanks! |
Fixes #112.
Note that the library has a bug that makes the default view empty now that the main category has been split into subcategories. I've made a fix for it and will make a PR for it that hopefully will be merged.