Skip to content
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

Attempt to fix the issue #6039 Font size increase does not increase preferences font size #6429

Closed
wants to merge 12 commits into from

Conversation

LuckyOne09
Copy link
Contributor

I am attempting to fix #6039

Hi, I have fixed the bug that the font size of preference dialog can't not change according to the specified font size.

I think that the scene of dialog window is not installed css properly. Therefore, I found the codes that are responsible for displaying the window. Just after those codes, I add some codes to ensure the preference dialog window is installed css.

the result after correcting is like:
result

Since I am not so familiar with the codes, probably this is not an elegant implementation. I will appreciate it very much if you could give me some suggestions.

Thanks, ^_^

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

LuckyOne09 and others added 7 commits May 1, 2020 21:45
sync with jabref/jabref
merge from JabRef/jabref
Merge from JabRef/jabref
merge from Jabref/jabref
Docs: For developers: New architectural decision added to list (#6397)
Luckylys and others added 3 commits May 6, 2020 15:16
…ferencesAction constructor rather than using Globals
… ShowPreferencesAction constructor rather than using Globals"

This reverts commit 75198fb.
@LuckyOne09 LuckyOne09 marked this pull request as ready for review May 6, 2020 08:48
Copy link
Member

@tobiasdiez tobiasdiez left a 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. The fix in itself looks good to me, but there might be a better place to put the new code.

Please add a changelog entry, too!

new PreferencesDialogView(jabRefFrame).show();
PreferencesDialogView preferencesDialogView = new PreferencesDialogView(jabRefFrame);
preferencesDialogView.show();
for (PreferencesTab tab: preferencesDialogView.getPreferenceTabList().getItems()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The action shouldn't really care about how the dialog is displayed, that's the job of the dialog itself. Thus, I would prefer if the fix can be moved to the PreferencesDialogView class. Maybe to https://github.com/JabRef/jabref/pull/6429/files#diff-39a71e5858d655cd93a5ca0a21ff300aL81 ?

Copy link
Contributor Author

@LuckyOne09 LuckyOne09 May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @tobiasdiez
Thanks for your suggestions. In fact, I tried to fix it in the PreferencesDialogView, but the PreferencesTab.getBuilder().getScene() is not null only when it is initialized after preferencesDialogView.show(). Therefore, this is the only way I found to fix it. Maybe I missed something important, but I really have no idea to handle that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you tried to add it exactly? The scene should be non-null as soon as the control has been added to the dialog. (But to be honest I don't understand why the preference tab doesn't inherit the css from the preference window).

Copy link
Contributor Author

@LuckyOne09 LuckyOne09 May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I write some codes to make my point clear:

PreferencesDialogView preferencesDialogView = new PreferencesDialogView(jabRefFrame);
for (PreferencesTab tab: preferencesDialogView.getPreferenceTabList().getItems()) {
    System.out.println("result of getBuilder(): ");
    System.out.println(tab.getBuilder());
    System.out.println("result of getScene(): ");
    System.out.println(tab.getBuilder().getScene());
}
preferencesDialogView.show();
System.out.println("after show()");
for (PreferencesTab tab: preferencesDialogView.getPreferenceTabList().getItems()) {
    System.out.println("result of getBuilder(): ");
    System.out.println(tab.getBuilder());
    System.out.println("result of getScene(): ");
    System.out.println(tab.getBuilder().getScene());
}

The part of result is:

result of getBuilder(): 
	GeneralTabView@5448f579
result of getScene(): 
	null
result of getBuilder(): 
	FileTabView@28189b70
result of getScene(): 
	null

after show()

result of getBuilder(): 
	GeneralTabView@5448f579
result of getScene(): 
	javafx.scene.Scene@454c8139
result of getBuilder(): 
	FileTabView@28189b70
result of getScene(): 
	null

You can see that the result of getScene() is not null only after preferencesDialogView.show().

By the way, I found that preference tab is not the only one having this problem.
Nearly every pop window suffer this problem like this screenshot:
捕获

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
all those popup dialogs inherit from BaseDialog and there is this line for setting the css as well:
Globals.getThemeLoader().installCss(getDialogPane().getScene(), Globals.prefs);

This would explain why those dialogs still have the same size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankyou for your guidance.
I tried to use

EasyBind.subscribe(new ReadOnlyObjectWrapper(getDialogPane().getScene()), scene -> Globals.getThemeLoader().installCss((Scene) scene, Globals.prefs));

to replace

Globals.getThemeLoader().installCss(getDialogPane().getScene(), Globals.prefs);

in constructor of BaseDialog.
However, I found that this installCss() was still executed before DialogView.show() (this is pseudo code, if I am press "Customize key bindings", then it should be KeyBindingsDialogView().show())
I am confused now about how to solve this.
The only solution I can come up with is adding some codes after each DialogView.show().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, you could try to use the properties onShown or onShowing https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/Dialog.html

Copy link
Member

@tobiasdiez tobiasdiez May 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use getScene then you access the current value of the scene (which is null) and this is static, i.e. the ReadOnlyObjectWrapper will never update. To resolve this, you can use the sceneProperty to get notified about the new value.

Something like the following should work:
EasyBind.wrapNullable(getDialogPane().sceneProperty()).subscribeToValues(scene -> Globals.getThemeLoader().installCss(scene, Globals.prefs))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use getScene then you access the current value of the scene (which is null) and this is static, i.e. the ReadOnlyObjectWrapper will never update. To resolve this, you can use the sceneProperty to get notified about the new value.

Something like the following should work:
EasyBind.wrapNullable(getDialogPane().sceneProperty()).subscribeToValues(scene -> Globals.getThemeLoader().installCss(scene, Globals.prefs))

I found a interesting thing. That is I found that the KeyBindingsDialogView.getDialogPane().getScene() can be initiated properly before KeyBindingsDialogView.show()

To prove this, I add some codes in construtor of BaseDialog:

        System.out.println("before before");
        System.out.println(getDialogPane().getScene());
        Globals.getThemeLoader().installCss(getDialogPane().getScene(), Globals.prefs);

and see the behaviours in the debug mode.

following is results:

command line output is :

before before
javafx.scene.Scene@b1b581d
07:27:19.064 [JavaFX Application Thread] INFO  org.jabref.gui.util.ThemeLoader - Enabling live reloading of {$workspace}\build\resources\main\org\jabref\gui\Base.css

screenshot for debug mode (19 is the font size I set)
image

I think it means Themeloader.installCss() can apply to KeyBindingsDialogView properly but the font size does not change. I am comfused again about this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it also the same scene that is used after the dialog is shown? Maybe it's registering to the old scene that gets replaced?

@tobiasdiez tobiasdiez added status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 6, 2020
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LuckyOne09 ,
about the problem: This bug smells like it's about the font settings are not updated after changing. Can you please try if the changed font settings have an effect after restarting JabRef?

Also about injection of JabRefPreferences: As @Siedlerchr already expressed: we are currently in the process of refactoring the JabRefPreferences and try to avoid a direct call to Globals. You should be able to inject JabRefPreferences in the constructor of ShowPreferencesAction.

@calixtus
Copy link
Member

Hi @LuckyOne09, have you made any progress investigating the deeper issue about this bug or did you try @tobiasdiez approach on this?

@calixtus calixtus added the status: waiting-for-feedback The submitter or other users need to provide more information about the issue label May 20, 2020
@LuckyOne09
Copy link
Contributor Author

Hi @LuckyOne09, have you made any progress investigating the deeper issue about this bug or did you try @tobiasdiez approach on this?

Sorry, I am still trying @tobiasdiez approach. I am busy recently...... Sorry for that. If I have some progress, I will let you know.

@Siedlerchr Siedlerchr mentioned this pull request Jul 31, 2020
5 tasks
@calixtus
Copy link
Member

#6725 (comment)

@tobiasdiez
Copy link
Member

Thanks a lot @LuckyOne09 for your PR. I've continued working on it in #6828. This solution there doesn't work in all cases for some reason, so if you find the time feel free to come back to this issue. If you feel like attacking another issue, that's also ok of course.

@tobiasdiez tobiasdiez closed this Aug 31, 2020
koppor pushed a commit that referenced this pull request Mar 1, 2023
e9fd2027de Add Medicine Publishing Styles (#6434)
cae128f35f Create Bristol University Press (#6356)
74b4af3b82 Create internet-archaeology.csl (#6357)
ee7ece480b Add Bio-Protocol style (#6429)
9a455efcee Create archives-of-medical-research.csl (#6415)
e91aba46fc Remove some bursa-uludag styles (#6423)
03f3962657 Update offa.csl (#6428)
95dc9b9f5a Update journal-of-neolithic-archaeology.csl (#6427)
a4e6c7f477 Update the-university-of-winchester-harvard.csl (#6374)
c0bf10647a add manuscript formatting to ASA (#6387)
3a673a564a Update universite-de-sherbrooke-histoire.csl (#6392)
0c48c7289e Update chemistry-education-research-and-practice.csl (#6397)
51f718a7b9 Update journal-of-endodontics.csl (#6409)
51e419051f Update presses-universitaires-de-rennes.csl (#6413)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: e9fd2027de4e2355f3244ac662960467e225774d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers status: waiting-for-feedback The submitter or other users need to provide more information about the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Font size increase does not increase preferences font size
5 participants