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

Show Citation style also in entry preview in preferences #4121

Merged
merged 4 commits into from
Jun 12, 2018

Conversation

eso31
Copy link
Contributor

@eso31 eso31 commented Jun 10, 2018

Fixes #3849

It works, now you can have a preview of a selected citation style. I am not sure if it is the correct way of programming it though. Let me now if I should do it in some other way or try a different approach.


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Manually tested changed features in running JabRef
  • Screenshots added in PR description (for bigger UI changes)
  • Ensured that the git commit message is a good one
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@Siedlerchr Siedlerchr changed the title Fixes #3849 Show Citation style also in entry preview in preferences Jun 11, 2018
@@ -126,8 +131,19 @@ private void setupLogic() {
DefaultTaskExecutor.runInJavaFXThread(() -> {

PreviewPanel testPane = new PreviewPanel(null, null, Globals.getKeyPrefs(), Globals.prefs.getPreviewPreferences(), dialogService);
Copy link
Member

Choose a reason for hiding this comment

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

Here null is passed for BasePanel and bibdatabasecontext,

PreviewPreferences p = Globals.prefs.getPreviewPreferences();
p = new PreviewPreferences(p.getPreviewCycle(),indexStyle,p.getPreviewPanelDividerPosition(),p.isPreviewPanelEnabled(), p.getPreviewStyle(),p.getPreviewStyleDefault());

testPane = new PreviewPanel(Globals.basePanel, new BibDatabaseContext(), Globals.getKeyPrefs(), p, dialogService);
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need the base panel here? If you look at the line 133 where the PreviewPanel is initialized for the first time, there is null passed for the basePanel and the bibdatabasecontex. So I assume you don't need the Globals.basepanel hack here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I need the basepanel because in the PreviewPanel.java file checks if basePanel is present in order to get the citation otherwise it doesn't work.

At first I initialized it with null because if no citation style is selected generates a default citation. So that's why I did it that way, the other thing I could try is to write the same logic there without calling the function updateLayout in PreviewPanel but it'll end up with duplicated code. What do you think is the best approach?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, I now did take a closer look at the code here. Having a dependency on the BasePanel is not nice. Well, but looking at the code and trying out some things I see no other way as the CitationStyleCache is configured on the basePanel.
However, you can use the method here JabRefGUI.getMainFrame().getCurrentBasePanel() . so you don't need that dependency on base panel in globals.

Copy link
Contributor Author

@eso31 eso31 Jun 11, 2018

Choose a reason for hiding this comment

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

Thank you, I removed it from Globals and used getCurrentBasePanel instead and it worked.

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jun 11, 2018
@Siedlerchr
Copy link
Member

Thank you very much for your contribution! 👍 If it works without the BasePanel hack, it looks good to me. Our internal standards require a second dev to review your code until it will be merged.

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

See my comments about the BasePanel hack

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.

A big thank you also from me. The code looks good, aside from a very minor remark. Please add a changelog entry and then we can merge!

}
else {
int indexStyle = chosen.getSelectedIndex();
PreviewPreferences p = Globals.prefs.getPreviewPreferences();
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use abbreviations as this makes the code harder to understand: p -> preferences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, I changed it to preferences, and added a changelog entry as well. Thank you.

@tobiasdiez tobiasdiez merged commit df98c31 into JabRef:master Jun 12, 2018
Siedlerchr added a commit that referenced this pull request Jun 12, 2018
* upstream/master:
  Show Citation style also in entry preview in preferences (#4121)

# Conflicts:
#	src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java
Siedlerchr added a commit that referenced this pull request Jun 13, 2018
* upstream/master:
  Extract v4.x changelog (#4125)
  Saves and reloads window state on close and on open (#4124)
  Fix convert to bibtex moves contents of the file field (#4123)
  Opens the directory of the currently edited file when opening a new file (#4106)
  Show Citation style also in entry preview in preferences (#4121)
  Mac graphics (#4074)
  The textarea in the PersonsEditor is focused when the field is focused (#4112)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants