Skip to content

[WIP] Add External Application Selection to Preferences#3942

Closed
slyr91 wants to merge 1 commit into
JabRef:masterfrom
slyr91:maintable-beta
Closed

[WIP] Add External Application Selection to Preferences#3942
slyr91 wants to merge 1 commit into
JabRef:masterfrom
slyr91:maintable-beta

Conversation

@slyr91

@slyr91 slyr91 commented Apr 10, 2018

Copy link
Copy Markdown

WIP - Add External Application Selection to Preferences

This pull request is in regards to #674.
This initial commit adds a button to the settings panel for each external application which allows the user to select an application to make default. There is currently a problem with this build where the application will not update the pushToApplicationButton. I believe the issue has something to do with the interaction between javafx and swing components. Also I plan on adding keybinding options as well in the future, it's just taking me a bit of time to familiarize myself with the program between life events. Let me know what you think so far and on the direction I am taking.

Addresses #674
This commit adds a button to the settings panel of each external application to set the currently selected application to be the default application. It is known that pressing the button does not properly update the applications pushToApplicationButton properly.
@slyr91 slyr91 changed the title Add option to set default external application WIP - Add External Application Selection to Preferences Apr 10, 2018
@tobiasdiez

Copy link
Copy Markdown
Member

This looks like a good start. Nice work!

The button is not updated because such an update mechanism is not yet implemented. The icon is generated from an action (which itself is created in PushToApplicationButton.getMenuAction). However, the Action class only has static return values. I would propose to follow the example of controlsfx Action and replace the getters in the action interface by javafx properties. Then you can use bindings in the adapter class JabRefAction and in this way update the UI if the underlying property is changed.

@slyr91 do you want to implement these changes or prefer if somebody from the team does it? (I would volunteer but it may take some time).

@slyr91

slyr91 commented Apr 11, 2018

Copy link
Copy Markdown
Author

I will definitely give this a solid try. @tobiasdiez I am still learning about the proper procedures on GitHub but if I can implement those changes should I add them to this pull request or to a separate one?

@tobiasdiez

Copy link
Copy Markdown
Member

Good question. In general, it is better to create a few smaller PRs because that makes reviewing and understanding the code changes easier. Having said this, in this particular case, I think however that the changes are small and localized enough so that you can simply push them to this PR.

@koppor koppor changed the title WIP - Add External Application Selection to Preferences [WIP] Add External Application Selection to Preferences May 11, 2018
@LinusDietz LinusDietz changed the base branch from maintable-beta to master June 29, 2018 14:01
@stefan-kolb

Copy link
Copy Markdown
Member

@slyr91 Will you work on this in the future?

@slyr91

slyr91 commented Jul 12, 2018

Copy link
Copy Markdown
Author

@stefan-kolb I definitely will give it a shot soon. I have just been too busy to dive into this. I had finals to deal with and now I am trying to study for a certification so all my free time goes to that. As soon as I am done with that I will be returning to this.

@slyr91

slyr91 commented Aug 30, 2018

Copy link
Copy Markdown
Author

I'm getting back into this now and I noticed that the branch this was originally built on, maintainable-beta, is closed. Which branch should I apply these changes and proposed improvements to?

@stefan-kolb

Copy link
Copy Markdown
Member

Hi, on the master branch please 😄

@Siedlerchr

Copy link
Copy Markdown
Member

You probably have to rework your code a bit as recently the preferences dialog was transformed to javaFX as well.

@tobiasdiez

Copy link
Copy Markdown
Member

Surpassed by #5024.

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.

4 participants