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

Restructure startup #4054

Merged
merged 5 commits into from
May 25, 2018
Merged

Restructure startup #4054

merged 5 commits into from
May 25, 2018

Conversation

stefan-kolb
Copy link
Member

Considered as necessary when reviewing #4047

@stefan-kolb stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 24, 2018
/**
* Perform checks and changes for users with a preference set from an older JabRef version.
*/
private static void migratePreferences() {
Copy link
Member

Choose a reason for hiding this comment

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

Move to PreferencesMigrations ?

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.

The changes look good to me.

I'm pretty sure that this gets a lot of merge conflicts with the maintable-beta branch...

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Wow, the new code looks really great! I have a micro comment on two lines of duplicated code 🤷‍♂️


// See if we should shut down now
if (argumentProcessor.shouldShutDown()) {
Globals.shutdownThreadPools();
Copy link
Member

Choose a reason for hiding this comment

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

This code is appearing twice, too. Can it be moved to shutdownCurrentInstance()?

Alternatively, change line this line to

if (!allowMultipleAppInstances(args) || argumentProcessor.shouldShutDown()) {

and remove the shutdown code from allowMultipleAppInstances.

.invokeLater(() -> new JabRefGUI(argumentProcessor.getParserResults(), argumentProcessor.isBlank()));
}

private static boolean allowMultipleAppInstances(String[] args) {
Copy link
Member

Choose a reason for hiding this comment

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

Rename that to handleMultipleInstances, because some "black magic" is happening here. Nut just a check whether multiple instances are allowed.

@stefan-kolb stefan-kolb merged commit cab2692 into master May 25, 2018
@stefan-kolb stefan-kolb deleted the startup branch May 25, 2018 07:48
@tobiasdiez
Copy link
Member

@stefan-kolb may I ask you to merge the master now also into the maintable-beta branch. You are more familiar with the changes to the startup code than me.

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.

4 participants