Add restoration functionality for shared database #2014

obraliar · 2016-09-20T11:02:47Z

Related to #1703:

Session restore: JabRef should open all [shared] tabs after startup.

-> Decision: Open from last connection.

Internal quality assurance
Change in CHANGELOG.md described
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Siedlerchr · 2016-09-20T11:40:59Z

src/main/java/net/sf/jabref/JabRefGUI.java

+            try {
+                new SharedDatabaseUIManager(mainFrame, keywordSeparator).openLastSharedDatabaseTab(isFocused);
+            } catch (ClassNotFoundException | SQLException | DatabaseNotSupportedException e) {
+                LOGGER.error("Failed to restore shared database. Use connection dialog to connect.");


Please add the exception e as second parameter to the logger.error (there is already an overload for that=

I intentionally didn't add it, cause those exceptions basically inform the user about the failure. It's not a real problem. But I think it's better to use LOGGER.info(...). Done.

Ah okay. It's just about that in case of a real exception, e.g. soemthing is wrong it helps the developers to debug the root cause ;)

- Restoration of the last shared database tab which was sucessfully connected - Popup of the connection dialog for the case that the new connection wasn't sucessfull - Reorganisation of DBMSConnectionProperties & OpenSharedDatabaseDialog (simplification)

tschechlovdev

Made some minor comments

tschechlovdev · 2016-09-20T15:13:31Z

src/main/java/net/sf/jabref/shared/DBMSConnectionProperties.java

+            }
+        }
+
+        if (prefs.getHost().isPresent()) {


you could use prefs.getHost().ifPresent(prefsHost -> this.host = prefsHost).
This avoids the if statements

tschechlovdev · 2016-09-20T15:15:35Z

src/main/java/net/sf/jabref/shared/DBMSConnectionProperties.java

+            try {
+                this.password = new Password(prefs.getPassword().get().toCharArray(), prefs.getUser().get()).decrypt();
+            } catch (UnsupportedEncodingException | GeneralSecurityException e) {
+                LOGGER.error("Could not decrypt pasword" + e);


replace + with ,

tschechlovdev · 2016-09-20T15:18:12Z

src/main/java/net/sf/jabref/shared/DBMSConnectionProperties.java

+        }
+
+        if (prefs.getPort().isPresent()) {
+            this.port = Integer.parseInt(prefs.getPort().get());


Maybe you should catch here the NumberFormatException and log, if the port contains some values that cannot converted to an int. Or is this somewhere else done?

A NumberFormatException can never occur, except hard changes in prefs (on the file system or registry) were made.

tschechlovdev · 2016-09-20T15:22:54Z

src/main/java/net/sf/jabref/shared/DBMSConnectionProperties.java

+        }
+
+        if (prefs.getUser().isPresent()) {
+            this.user = prefs.getUser().get();


Why don't add here if(prefs.getPassword.isPresent())? Then you don't have to check twice if user is present.

I think you mean ifPresent...

As well the user as the password is required for the class Password. It takes the Username as secret key. Therefore I have to check both. Or am I understanding sth. wrong?

No, here I meant isPresent. You check here for user.isPresent and below you check whether password.isPresent and user.isPresent. So why don't combine both:

if(prefs.getUser().isPresent() { this.user = ... if(prefs.getPassword().isPresent() { this.password = .... } }

So then you have only one check if prefs.getUser.isPresent and not twice.

When both is required you could just combine the two with and &&

Ok nevermind. That's ok how it is.

Ah you mean sth. like:

if (A) { x = ... } if (A && B) { y = ... }

=>

if (A) { x = ... if (B) { y = ... } }

Here you can save one check (A). 👍

Done.

obraliar · 2016-09-20T16:19:20Z

@tschechlovdev Thanks for suggestions 👍. Appropriate changes have been applied.

tschechlovdev · 2016-09-20T20:47:42Z

src/main/java/net/sf/jabref/shared/DBMSConnectionProperties.java

+            }
+        }
+
+        if (!prefs.getPassword().isPresent()) {


you could combine this if and the if statement above with an if / else if like the following:

if(!prefs.getPassword().isPresent()) { ... } else if(prefs.getPassword().isPresent() && prefs.getUser().isPresent()){ .... }

Oh, now I choosed the option from above.

Yeah that's ok 👍
But then please delete
prefs.getUser().ifPresent(theUser -> this.user = theUser); ;)

Oh, yeah. Done.

- Fix Conflicts in: CHANGELOG.md src/main/java/net/sf/jabref/gui/JabRefFrame.java src/main/java/net/sf/jabref/gui/shared/OpenSharedDatabaseDialog.java src/main/java/net/sf/jabref/gui/shared/SharedDatabaseUIManager.java

koppor · 2016-09-22T19:32:24Z

Went through with Admir. LGTM.

- Restoration of the last shared database tab which was sucessfully connected - Popup of the connection dialog for the case that the new connection wasn't sucessfull - Reorganisation of DBMSConnectionProperties & OpenSharedDatabaseDialog (simplification)

obraliar force-pushed the RestoreSharedDatabaseTab branch from 0bccbc6 to 412a412 Compare September 20, 2016 11:13

Siedlerchr reviewed Sep 20, 2016

View reviewed changes

obraliar force-pushed the RestoreSharedDatabaseTab branch from 412a412 to 2be310d Compare September 20, 2016 11:41

obraliar force-pushed the RestoreSharedDatabaseTab branch from 2be310d to d8257e7 Compare September 20, 2016 11:46

boceckts added stupro labels Sep 20, 2016

tschechlovdev reviewed Sep 20, 2016

View reviewed changes

obraliar force-pushed the RestoreSharedDatabaseTab branch 2 times, most recently from 94fe423 to d8257e7 Compare September 20, 2016 19:43

tschechlovdev reviewed Sep 20, 2016

View reviewed changes

obraliar force-pushed the RestoreSharedDatabaseTab branch 2 times, most recently from 5c0f46a to eea0629 Compare September 20, 2016 21:13

Fix QA issues.

5d30b42

obraliar force-pushed the RestoreSharedDatabaseTab branch from eea0629 to 5d30b42 Compare September 20, 2016 22:23

tschechlovdev added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Sep 20, 2016

tschechlovdev changed the title ~~[WIP] Add restoration functionality for shared database~~ Add restoration functionality for shared database Sep 20, 2016

obraliar added 2 commits September 21, 2016 21:16

Change changelog description.

5d3e8c1

Merge branch 'master' into RestoreSharedDatabaseTab

b917634

- Fix Conflicts in: CHANGELOG.md src/main/java/net/sf/jabref/gui/JabRefFrame.java src/main/java/net/sf/jabref/gui/shared/OpenSharedDatabaseDialog.java src/main/java/net/sf/jabref/gui/shared/SharedDatabaseUIManager.java

obraliar force-pushed the RestoreSharedDatabaseTab branch 2 times, most recently from b917634 to 5d3e8c1 Compare September 22, 2016 15:33

koppor merged commit e1b2aa3 into JabRef:master Sep 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add restoration functionality for shared database #2014

Add restoration functionality for shared database #2014

obraliar commented Sep 20, 2016 •

edited by tschechlovdev

Loading

Siedlerchr Sep 20, 2016

obraliar Sep 20, 2016 •

edited

Loading

Siedlerchr Sep 20, 2016

tschechlovdev left a comment

tschechlovdev Sep 20, 2016

obraliar Sep 20, 2016

tschechlovdev Sep 20, 2016

obraliar Sep 20, 2016

tschechlovdev Sep 20, 2016 •

edited

Loading

obraliar Sep 20, 2016

tschechlovdev Sep 20, 2016

Siedlerchr Sep 20, 2016

obraliar Sep 20, 2016

tschechlovdev Sep 20, 2016 •

edited

Loading

Siedlerchr Sep 20, 2016

tschechlovdev Sep 20, 2016

obraliar Sep 20, 2016 •

edited

Loading

obraliar commented Sep 20, 2016

tschechlovdev Sep 20, 2016

obraliar Sep 20, 2016

tschechlovdev Sep 20, 2016 •

edited

Loading

obraliar Sep 20, 2016

koppor commented Sep 22, 2016

Add restoration functionality for shared database #2014

Add restoration functionality for shared database #2014

Conversation

obraliar commented Sep 20, 2016 • edited by tschechlovdev Loading

Choose a reason for hiding this comment

obraliar Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschechlovdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschechlovdev Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschechlovdev Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obraliar Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

obraliar commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschechlovdev Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Sep 22, 2016

obraliar commented Sep 20, 2016 •

edited by tschechlovdev

Loading

obraliar Sep 20, 2016 •

edited

Loading

tschechlovdev Sep 20, 2016 •

edited

Loading

tschechlovdev Sep 20, 2016 •

edited

Loading

obraliar Sep 20, 2016 •

edited

Loading

tschechlovdev Sep 20, 2016 •

edited

Loading