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

Switch to org.postgresql #4031

Merged
merged 4 commits into from
May 20, 2018
Merged

Switch to org.postgresql #4031

merged 4 commits into from
May 20, 2018

Conversation

tobiasdiez
Copy link
Member

In the forum, there were some reports that the postgre SQL integration sometimes fails due to a bug in the sql driver we use. It appears that the driver pgjdbc-ng is no longer maintained and all the features we need are present in the "official driver". Thus I switched to the later in the hope that it fixes the issues mentioned in the forum.


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

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 14, 2018
}

// Wait a while before checking again for new notifications
Thread.sleep(500);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the driver support push notifications?

Copy link
Member

Choose a reason for hiding this comment

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

// Do not use `new PostgresSQLNotificationListener(...)` as the object has to exist continuously!
// Otherwise the listener is going to be deleted by GC.
pgConnection.addNotificationListener(listener);
PGConnection pgConnection = connection.unwrap(PGConnection.class);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the above comment, which contradicts the new code.

Has someone tested the new implementation?

Copy link
Member

Choose a reason for hiding this comment

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

I can try to test it this evening. I think I have a pg sql db somewhere lying around

@Siedlerchr
Copy link
Member

Database test seems to be green. I will try to test this manually.
There is still one checkstyle error left

@koppor
Copy link
Member

koppor commented May 14, 2018

We do not have any test for the remote database feature (refs #3282). There are free postgres hostings (see http://help.jabref.org/en/SQLDatabase#try-it-out) to try it out. It is important that at least two different JabRef instances are connected.

@tobiasdiez
Copy link
Member Author

@Siedlerchr It would be nice if you could try it out. In the forum it was said that this version does not work since postgre does not show up as an option. As I have absolutely no experience with the database-related code, I highly welcome help on this PR.

@Siedlerchr
Copy link
Member

17:44:13.731 [JavaFX Application Thread] INFO org.jabref.logic.shared.DBMSConnection - PostgreSQL driver not available.

@Siedlerchr
Copy link
Member

So I managed to get the connection working. However, it's currently unusable due to the same error the importer has: (Just import a new bib file)

The problem is the "addTab" Method which creates a new basePanel and fails as the PreviewPanel is not on FX thread. However, I tried wrapping the call to it in the FXThread, but that did just lead to a complete freeze. I don't know how to solve this

18:11:22.467 [AWT-EventQueue-0] ERROR org.jabref.FallbackExceptionHandler - Uncaught exception occurred in Thread[AWT-EventQueue-0,6,main]
java.lang.IllegalStateException: Not on FX application thread; currentThread = AWT-EventQueue-0
	at com.sun.javafx.tk.Toolkit.checkFxUserThread(Toolkit.java:279) ~[jfxrt.jar:?]
	at com.sun.javafx.tk.quantum.QuantumToolkit.checkFxUserThread(QuantumToolkit.java:423) ~[jfxrt.jar:?]
	at javafx.scene.web.WebEngine.checkThread(WebEngine.java:1243) ~[jfxrt.jar:?]
	at javafx.scene.web.WebEngine.<init>(WebEngine.java:879) ~[jfxrt.jar:?]
	at javafx.scene.web.WebEngine.<init>(WebEngine.java:868) ~[jfxrt.jar:?]
	at javafx.scene.web.WebView.<init>(WebView.java:273) ~[jfxrt.jar:?]
	at org.jabref.gui.PreviewPanel.<init>(PreviewPanel.java:83) ~[main/:?]
	at org.jabref.gui.BasePanel.<init>(BasePanel.java:234) ~[main/:?]
	at org.jabref.gui.JabRefFrame.addTab(JabRefFrame.java:1206) ~[main/:?]
	at org.jabref.gui.shared.SharedDatabaseUIManager.openNewSharedDatabaseTab(SharedDatabaseUIManager.java:117) ~[main/:?]
	at org.jabref.gui.shared.ConnectToSharedDatabaseDialog.openSharedDatabase(ConnectToSharedDatabaseDialog.java:145) ~[main/:?]

…date

* upstream/maintable-beta:
  Fix freeze when importing (#4037)
  Use JavaFX-native SVGPath for logo in About dialog (#4035)
  Remove reflection hack to set WM_CLASS (#4034)
@Siedlerchr
Copy link
Member

I would propose to merge this changes. And I am currently reworking the shared db dialog to FX, so that the problem with the exception of the import should be solved

@Siedlerchr Siedlerchr merged commit 4f1299e into maintable-beta May 20, 2018
@Siedlerchr Siedlerchr deleted the postgreUpdate branch May 20, 2018 09:21
Siedlerchr added a commit that referenced this pull request May 20, 2018
* upstream/maintable-beta:
  Switch to org.postgresql (#4031)
Siedlerchr added a commit that referenced this pull request May 22, 2018
* upstream/maintable-beta: (46 commits)
  update gradle to 4.7 (#4049)
  Add missing translation for "HTML to Unicode" (#4046)
  New Crowdin translations (#4042)
  Use all-text-fields magic also in BibTeX cleanup. (#4039)
  Switch to org.postgresql (#4031)
  Fix freeze when importing (#4037)
  Use JavaFX-native SVGPath for logo in About dialog (#4035)
  Remove reflection hack to set WM_CLASS (#4034)
  Fix initial freeze when downloading files
  Inline comment
  Move color related method
  Extract dialog service to field
  Extract dialog service to field
  Remove unused class
  Show tooltip for icon-based columns
  Remove commented-out code
  Convert static methods to instance members (and fix spelling)
  Fix formatting
  Fix formatting
  Remove unused argument
  ...

# Conflicts:
#	build.gradle
#	gradle/wrapper/gradle-wrapper.properties
#	src/main/java/org/jabref/gui/GUIGlobals.java
#	src/main/java/org/jabref/gui/SidePaneComponent.java
#	src/main/java/org/jabref/gui/groups/GroupModeViewModel.java
#	src/main/java/org/jabref/gui/groups/GroupSidePane.java
#	src/main/java/org/jabref/gui/help/AboutDialogView.java
#	src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java
#	src/main/java/org/jabref/logic/shared/PostgreSQLProcessor.java
#	src/main/resources/l10n/JabRef_en.properties
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