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

WIP Default External Application Push Function #4368

Closed
wants to merge 8 commits into from
Closed

WIP Default External Application Push Function #4368

wants to merge 8 commits into from

Conversation

slyr91
Copy link

@slyr91 slyr91 commented Oct 11, 2018

This is a new pull request to replace #3942 which address #674 in part. It doesn't have keybindings for the applications but it does allow for changing the default application.


  • 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?)

There is a button in the preferences -> external tab -> individual push application settings labeled "Set Default" that, once pressed, will change the Push to application button in the main frame.
Added an entry to reflect the added functionality.
@@ -213,6 +208,14 @@ private void init() {
Pane containerPane = DndTabPaneFactory.createDefaultDnDPane(DndTabPaneFactory.FeedbackType.MARKER, null);
tabbedPane = (DndTabPane) containerPane.getChildren().get(0);

uiUpdate = new Observable() {
Copy link
Member

Choose a reason for hiding this comment

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

this looks a bit odd. Why don't you use a ChangeListener or sth like that?

Copy link
Author

Choose a reason for hiding this comment

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

I decided to go with the simple Observable object because of the way everything is laid out. I could create an Observable Map and apply a change listener to that so when the button changes it can update the HBox.

@slyr91
Copy link
Author

slyr91 commented Oct 11, 2018

I will review the Codacy/PR Quality Review and CI checks and resolve them as soon as I get the chance.

The Codacy issues regarding checkstyle should be resolved with this commit and the Travis-CI issue regarding Localization should also be resolved.
The import ordering was not correct. They were not grouped correctly. Now they should be.
Selected Entries was left in the localization file.
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. I have a few remarks.

final JDialog diag = new JDialog(parent, Localization.lang("Settings"), true);
JPanel options = toApp.getSettingsPanel(n);
options.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));
diag.getContentPane().add(options, BorderLayout.CENTER);
ButtonBarBuilder bb = new ButtonBarBuilder();
JButton defaultApp = new JButton(Localization.lang("Set Default"));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe only Default or Default application ?

uiUpdate.addObserver((o, arg) -> {
DefaultTaskExecutor.runInJavaFXThread(() -> {
rightSide.getChildren().clear();
PushToApplicationButton pushExternal = new PushToApplicationButton(this, pushApplications.getApplications());
Copy link
Member

Choose a reason for hiding this comment

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

I like that you try to update the ui when the preferences is changed. However, this seems to be a bit too complicated here. We have to probably think about how to make the preference values observable on a global scale (not specific to the pull to app button). But this is a huge project and not in the scope of this PR. Hence, I'd favor the solution that the push to app button is only updated when JabRef is started and adding a short notification that JabRef needs to be restarted for the changes in the prefs to come into effect (have a look at the other prefs panes for such a text).

Copy link
Author

Choose a reason for hiding this comment

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

Ok, that sounds reasonable. I will tackle this soon. should be simple to implement.

@calixtus calixtus mentioned this pull request May 22, 2019
6 tasks
@calixtus calixtus mentioned this pull request Jun 2, 2019
6 tasks
@tobiasdiez
Copy link
Member

Surpassed by #5024.

Siedlerchr added a commit that referenced this pull request Dec 11, 2019
49a1841 Update queen-margaret-university-harvard.csl (#4443)
f841560 Create uni-bern-institut-fur-musikwissenschaft-note.csl (#4401)
6ee503c Create harvard-cite-them-right-all-authors.csl (#4416)
3eceec4 Create harvard-queen-margaret-university.csl (#4441)
3c2299d JEMS.csl (#4436)
bb2f60f Create open-window.csl (#4430)
a908ab4 Nutrition Research Reviews (#4434)
208b693 Bump nokogiri from 1.10.3 to 1.10.4 (#4439)
122a3bb Create nmfs-west-coast-region-national-environmental-policy-act.csl (#4426)
3565fc2 Update tissue-engineering.csl (#4435)
981044e Create transposition.csl (#4375)
73ef6f3 Update tissue-engineering.csl (#4431)
7c4a4cf Update r-and-d-management.csl (#4432)
8628afb Create china-national-standard-gb-t-7714-2015-numeric.csl (#4065)
6ce148c removed spaces in harvard-gesellschaft-fur-bildung-und-forschung-in-europa.csl (#4422)
2af1c6f Create japanese-journal-of-applied-physics.csl (#4425)
fc16579 Add Presses universitaires de Strasbourg style (#4411)
01e58fb Correct spacing issues and documentation link (#4423)
d97cf3f Update geistes-und-kulturwissenschaften-heilmann.csl (#4421)
017947d Update ens-de-lyon-centre-d-ingenierie-documentaire.csl (#4413)
bffe57e Create instituto-de-pesquisas-energeticas-e-nucleares-sp.csl (#4368)
18686d1 Update and rename zeitschrift-fuer-digitale-geisteswissenschaften.csl… (#4410)
bd3a5f6 Final fix for journal article cit plus other updates - uniwersytet-im-adama-mickiewicza-w-poznaniu-wydzial-anglistyki (#4417)

git-subtree-dir: src/main/resources/csl-styles
git-subtree-split: 49a1841
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants