Convert SharedDatabaseConnect Dialog to Javafx by Siedlerchr · Pull Request #4040 · JabRef/jabref

Siedlerchr · 2018-05-20T11:56:20Z

Connection works so far. No more freezes regarding addTabs. in basePanel
Somehow the dialog looks odd, @tobiasdiez Any idea what is missing? The checkboxes are not visible correct. I just see that it looks really odd in he maintable-beta branch, too. e..g. int he Event Log

Edit// The conection and the synchronization seems to be working.

Fixes: #3419 PostgresSQL database connection now supports SSL

TODO for documentaition:
Create and configure Server certs
https://www.postgresql.org/docs/current/static/ssl-tcp.html

Convert to keystore format
https://jdbc.postgresql.org/documentation/head/ssl-client.html

Currently I only tested it with PostgresSQL, but I see that mySQL should also work
https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-using-ssl.html

TODO:

check conflict merge dialog
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?)

add stub

* upstream/maintable-beta: Switch to org.postgresql (#4031)

tobiasdiez · 2018-05-22T07:42:19Z

The display problem probably results from us playing around with different styles and changing defaults. Since there is no checkbox in the main view, it was not noticed that some of the input controls look strange. I'll have a look at it later...

* upstream/maintable-beta: 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) Shortittle bibtexkeypattern now also discards small words (#4030)

add migration help message remove old dialogs

Siedlerchr · 2018-05-25T08:34:49Z

The database connection works so far. Tested with PostgresSQL. I reworked the dialog and fixed an error in the listener which led to freezes.
The MergeEntryDialog is not yet converted, this will be done in a future PR.

The only thing open is the GUI styling. The dialog looks a bit odd. But that's due to some missing CSS stuff I think.

tobiasdiez

In general the rework looks good. Thanks for the work!

It would be nice if you could move all the logic to the view model so that we have a nice separation of concerns. The other remarks are mostly minor.

tobiasdiez · 2018-05-25T10:18:11Z

+                        <Label text="%Database Type:" />
+                        <ComboBox fx:id="cmbDbType" prefHeight="25.0" prefWidth="606.0" GridPane.columnIndex="1" GridPane.columnSpan="2" />
+                        <Label text="%Host/Port:" GridPane.rowIndex="1" />
+                        <TextField fx:id="tbHost" GridPane.columnIndex="1" GridPane.rowIndex="1" />


I don't have a strong opinion on this, but this Hungarian notation is against the usual naming convention. Probably, host or hostTextField is preferable. @koppor what's your opinion about this?

tobiasdiez · 2018-05-25T10:26:56Z

+        this.frame = frame;
+        this.setTitle(Localization.lang("Connect to shared database"));
+
+        this.getDialogPane().getButtonTypes().addAll(connect, ButtonType.CANCEL);


It is preferable to set the buttons in the fxml

jabref/src/main/java/org/jabref/gui/help/AboutDialog.fxml

Lines 99 to 100 in 390c387

<ButtonType fx:id="copyVersionButton" text="%Copy Version" />

<ButtonType fx:constant="CLOSE" />

and then add the action in the code-behind

jabref/src/main/java/org/jabref/gui/help/AboutDialogView.java

Line 36 in 390c387

ControlHelper.setAction(copyVersionButton, getDialogPane(), event -> copyVersionToClipboard());

Unfortunately this results in an NPE here for the connect butotn which I don't understand. The lookUpButton returns null.

Did you also tried it without the surrounding buttontypes tag?

tobiasdiez · 2018-05-25T10:30:19Z

+
+    }
+
+    private void openDatabase(ActionEvent e) {


This method and essentially everything below should be moved to the ViewModel. The view should contain no logic whatsoever and just binds the fxml file to the properties in the view model.

tobiasdiez · 2018-05-25T10:31:45Z

+        cmbDbType.getSelectionModel().select(0);
+        viewModel.selectedDbmstypeProperty().bind(cmbDbType.getSelectionModel().selectedItemProperty());
+
+        viewModel.selectedDbmstypeProperty().addListener((observable, oldValue, newValue) -> {


EasyBind.subscribe is usually easier to read. Moreover, this logic should also be moved to the view model.

tobiasdiez · 2018-05-25T10:33:19Z

+        FileDialogConfiguration fileDialogConfiguration = new FileDialogConfiguration.Builder()
+                                                                                               .addExtensionFilter(FileType.BIBTEX_DB)
+                                                                                               .withDefaultExtension(FileType.BIBTEX_DB)
+                                                                                               .withInitialDirectory(Globals.prefs.get(JabRefPreferences.WORKING_DIRECTORY))


Please extract the Globals.prefs dependency. The new JavaFX stuff should no longer depend on Globals.

tobiasdiez · 2018-05-25T10:38:34Z

+    }
+
+    private void setLoadingConnectButtonText(boolean isLoading) {
+        btnConnect.setDisable(isLoading);


The viewModel should have a boolean property connectionInProgress so that the functionality of this method can be realized by binding.

tobiasdiez · 2018-05-25T10:39:50Z

+    private DBMSConnectionProperties connectionProperties;
+    private SharedDatabaseLoginDialogViewModel viewModel;
+
+    public SharedDatabaseLoginDialogView(JabRefFrame frame) {


I really don't like that we need a frame here. Can you please investigate if it can be replaced by the StateManager (maybe after extending the latter).

tobiasdiez · 2018-05-25T10:43:15Z

+    }
+
+    private void checkFields() throws JabRefException {
+        if (isEmptyField(tbHost)) {


Validation should be done using the validation framework so that the user get immediate feedback (have a look at the usage of ControlsFxVisualizer).

tobiasdiez · 2018-05-25T10:44:54Z

+                jabRefFrame.closeCurrentTab();
+                new SharedDatabaseLoginDialogView(jabRefFrame).showAndWait();
+
+            }


else if (and remove empty line above)

tobiasdiez · 2018-05-25T10:46:28Z


-            SwingUtilities.invokeLater(() -> panel.closeBottomPane());
+            dialogService.showInformationDialogAndWait(Localization.lang("Shared entry is no longer present"),
+                                                       Localization.lang("The BibEntry you currently work on has been deleted on the shared side.")


BibEntry is for our internal usage and entry should be used for dialogs.

Siedlerchr · 2018-05-25T13:04:24Z

+   </header>
+   <buttonTypes>
+      <ButtonType fx:constant="CLOSE" />
+      <ButtonType fx:id="connectButton" text="Connect" />


Here I add the button

Siedlerchr · 2018-05-25T13:04:53Z

+    @FXML private TextField tbFolder;
+    @FXML private Button btnBrowse;
+    @FXML private CheckBox chkAutosave;
+    @FXML private ButtonType connectButton;


The buttonType is initalized here

Siedlerchr · 2018-05-25T13:05:18Z

+                  .load()
+                  .setAsContent(this.getDialogPane());
+
+        ControlHelper.setAction(connectButton, this.getDialogPane(), event -> openDatabase());


In this method the lookup method from the dialogPane returns null. I don't understand why.

Somehow the buttons are in the content property of the dialog pane. Does not make any sense to me

Siedlerchr · 2018-05-25T13:49:15Z

Yes I tried both with and without the tags. Same behavior. I don't understand this. Tobias Diez <notifications@github.com> schrieb am Fr., 25. Mai 2018, 15:39:

…

***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/jabref/gui/shared/SharedDatabaseLoginDialogView.java <#4040 (comment)>: > + @Inject private DialogService dialogService; + + private final SharedDatabasePreferences prefs = new SharedDatabasePreferences(); + + private final ButtonType connect = new ButtonType("Connect", ButtonData.YES); + private final Button btnConnect; + private final JabRefFrame frame; + + private DBMSConnectionProperties connectionProperties; + private SharedDatabaseLoginDialogViewModel viewModel; + + public SharedDatabaseLoginDialogView(JabRefFrame frame) { + this.frame = frame; + this.setTitle(Localization.lang("Connect to shared database")); + + this.getDialogPane().getButtonTypes().addAll(connect, ButtonType.CANCEL); Did you also tried it without the surrounding buttontypes tag? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4040 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATi5GBJ--hYtjBd1unf_oiZTC9_5v_Zks5t2AmsgaJpZM4UGDLC> .

tobiasdiez · 2018-05-25T14:19:17Z

+
+        ViewLoader.view(this)
+                  .load()
+                  .setAsContent(this.getDialogPane());


Should be .setAsDialogPane(this);

add bindings

TODO: Validation throws error

…ing duplicate root pane stuff

* upstream/master: (89 commits) Fix that sometimes an empty side pane is shown (#4105) Update gradle from 4.7 to 4.8 (#4102) Add gradle task downloadDependencies and use it at CircleCI (#4091) Set minimum java to 171 (#4095) Also use https in the CHANGELOG itself Add missing CHANGELOG entry Fix localiazation test Use official TempFolder extension for JUnit 5 tests Remove menu* from crowdin.yml (#4094) Merge recent changes to exporter/importer Add missing CHANGELOG entry New translations JabRef_en.properties (Vietnamese) New translations JabRef_en.properties (Dutch) New translations JabRef_en.properties (French) New translations JabRef_en.properties (German) New translations JabRef_en.properties (Greek) New translations JabRef_en.properties (Indonesian) New translations JabRef_en.properties (Italian) New translations JabRef_en.properties (Japanese) New translations JabRef_en.properties (Danish) ... # Conflicts: # src/main/java/org/jabref/gui/shared/ConnectToSharedDatabaseDialog.java

renamings and other refactorings

* upstream/master: (129 commits) Fix Export to clipbaord action (#4286) Add minimal height for entry editor (#4283) Fix exceptions when trying to change settings (#4284) Added a comment for the purpose of the class (#4282) Fix NPE when importing from CLI (#4281) Fix display of key patterns convert bibtexkeypatterndlg to javafx as well (#4277) Style preference sections using css Fix a few style issues Changed the behavior of the field formatting dialog (#4275) Fix checkbox display Remove small font size Style side pane in preferences Wrap preference tabs in scrollpane Fix NPE with bibdatabasecontext in preview style dialog update xmlunit and log4j jul and checkstyle Update mysql driver to 8.0.12 Rework preference dialog main class Move preferences stuff to gui.preferences Implement drag and drop for maintable (#3765) ... # Conflicts: # src/main/java/org/jabref/gui/shared/ConnectToSharedDatabaseDialog.java

* upstream/master: Fix error prone warnings (#4290) Fix freeze on import into current library (#4299) Upgrade gradle to 4.10 update xmlunit and postgres update xmlunit-matchers from 2.6.0 -> 2.6.1 update gradle build-scan from 1.15.2 -> 1.16 checkstyle execute changes only if disk db present fix npe in Merge entries dialog

fix layoout enable ssl stuff only when postgres is enabeld

shutdown listener stops the listener thread safely

tobiasdiez

Since this already looks pretty good, I took the opportunity and reviewed the code again. Feel free to merge directly after fixing my comments.

tobiasdiez · 2018-08-30T19:49:42Z

+            });
+        });
+
+        viewModel.applyPreferences();


call this in the constructor of the view model

tobiasdiez · 2018-08-30T19:50:12Z

+        databaseType.valueProperty().bindBidirectional(viewModel.selectedDbmstypeProperty());
+
+        folder.textProperty().bindBidirectional(viewModel.folderProperty());
+        browseButton.disableProperty().bind(autosave.selectedProperty().not());


Bind to viewModel.autosaveProperty() (the view model controls the view).

tobiasdiez · 2018-08-30T19:50:59Z

+
+        fileKeystore.textProperty().bindBidirectional(viewModel.keyStoreProperty());
+
+        browseKeystore.disableProperty().bind(useSSL.selectedProperty().not());


tobiasdiez · 2018-08-30T19:55:09Z

+        hostValidator = new FunctionBasedValidator<>(host, predicate, ValidationMessage.error(Localization.lang("Required field \"%0\" is empty.", Localization.lang("Port"))));
+        portValidator = new FunctionBasedValidator<>(port, predicate, ValidationMessage.error(Localization.lang("Required field \"%0\" is empty.", Localization.lang("Host"))));
+        userValidator = new FunctionBasedValidator<>(user, predicate, ValidationMessage.error(Localization.lang("Required field \"%0\" is empty.", Localization.lang("User"))));
+        folderValidator = new FunctionBasedValidator<>(folder, predicate, ValidationMessage.error(Localization.lang("Please enter a valid file path.")));


Really test that the files exists.

tobiasdiez · 2018-08-30T19:58:29Z

+                                                                                      ButtonType.OK, openHelp);
+
+            result.filter(btn -> btn.equals(openHelp)).ifPresent(btn -> HelpAction.openHelpPage(HelpFile.SQL_DATABASE_MIGRATION));
+            result.filter(btn -> btn == ButtonType.OK).ifPresent(btn -> openSharedDatabase());


I'm not sure but equals might be necessary here.

tobiasdiez · 2018-08-30T20:05:35Z

-            this.connection = DriverManager.getConnection(
-                    properties.getType().getUrl(properties.getHost(), properties.getPort(), properties.getDatabase()),
-                    properties.getUser(), properties.getPassword());
+            Properties props = new Properties();


Can you please move these additions to the DBMSConnectionProperties class as two methods Properties asJavaProperties() and String getUrl.

tobiasdiez · 2018-08-30T20:06:31Z

+               && this.host.equalsIgnoreCase(properties.getHost())
+               && (this.port == properties.getPort())
+               && this.database.equals(properties.getDatabase())
+               && this.user.equals(properties.getUser());


add useSSL in equals and hash

tobiasdiez · 2018-08-30T20:07:43Z

            PGConnection pgConnection = connection.unwrap(PGConnection.class);
            listener = new PostgresSQLNotificationListener(dbmsSynchronizer, pgConnection);
-            listener.start();
+            new Thread(listener, "PostgresSQLNotificationListener").start();


Better reuse our threading infrastructure so that we are sure that the thread is shutdown on exit etc.

tobiasdiez · 2018-08-30T20:08:26Z


+    public void start() {
+        stop = false;
+        new Thread(this).start();


tobiasdiez · 2018-08-30T20:08:46Z

    }

+    public boolean isUseSSL()
+    {


use executor store keystore in prefs(todo)

Siedlerchr added 3 commits May 18, 2018 23:02

Convert SharedBD Dialog to javafx

a2f99df

add stub

Merge remote-tracking branch 'upstream/maintable-beta' into sharedblogin

3db0452

* upstream/maintable-beta: Switch to org.postgresql (#4031)

convert shared database dialog to javafx

8e79ac9

Siedlerchr added component: shared-database status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 20, 2018

Siedlerchr added 3 commits May 20, 2018 17:24

fix checkstyle

cfa6384

convert some more dialogs to fx

bd73449

checkstyle

f4f0bc4

Siedlerchr added 2 commits May 25, 2018 09:16

remove duplicate listener which results in loop

66ecd83

Siedlerchr changed the title ~~[WIP] Convert SharedDatabaseConnect Dialog to Javafx~~ Convert SharedDatabaseConnect Dialog to Javafx May 25, 2018

add localization

08836a5

add migration help message remove old dialogs

Siedlerchr changed the base branch from master to maintable-beta May 25, 2018 08:29

add cancel button

e228e65

Siedlerchr requested review from koppor, lenhard and obraliar and removed request for lenhard May 25, 2018 08:35

tobiasdiez requested changes May 25, 2018

View reviewed changes

add button types, but results in npe

9348086

Siedlerchr commented May 25, 2018

View reviewed changes

tobiasdiez reviewed May 25, 2018

View reviewed changes

Siedlerchr added 3 commits May 25, 2018 16:51

move gui logic to viewmodel

e7fd700

add bindings

add validation support, move rest of logic to view model

f960808

TODO: Validation throws error

wrap initVisualization in platform.runlater to avoid exception regard…

e0cf01d

…ing duplicate root pane stuff

Siedlerchr changed the base branch from maintable-beta to master June 6, 2018 08:37

Siedlerchr added 2 commits June 6, 2018 10:39

remove old gui class

2fcc7a3

renamings and other refactorings

Siedlerchr mentioned this pull request Jun 12, 2018

Feature Request: OpenSSL for SQL #3419

Closed

Siedlerchr mentioned this pull request Jul 23, 2018

Fix Part of Issue#3861: Edit->Replace String #4227

Merged

6 tasks

Siedlerchr added 13 commits August 24, 2018 16:07

add ssl options

7c1605c

l10n cleanup

0da63fa

add bindings for keystore

b01ca3c

fix layoout enable ssl stuff only when postgres is enabeld

checkstyle

d3ac48f

fix l10n

bbce005

add support for ssl

a38d03a

fix checkstyle

0dc04a3

fix keystore file path not displayed

46d90f6

renamings

e1d45f9

shutdown listener stops the listener thread safely

enable ssl for all

b4190f0

checkstyle

dbc7f03

tobiasdiez requested changes Aug 30, 2018

View reviewed changes

Siedlerchr added 7 commits August 31, 2018 12:56

cleanups

9e68ea5

use executor store keystore in prefs(todo)

store keystore file in prefs

c6a3b1a

set useSSL and keystore correctly

18f8344

fix checkstyle

1814069

resize and convert one binding to view model

c19bb18

address comments, pass connectionProperties as parameter

83898ff

fix one codacy suggestion

4102644

Siedlerchr mentioned this pull request Aug 31, 2018

Update shared SQL information JabRef/user-documentation#193

Merged

1 task

Siedlerchr merged commit ef088e2 into master Aug 31, 2018

Siedlerchr deleted the sharedblogin branch August 31, 2018 15:58

luizphilipe2 mentioned this pull request Jun 26, 2021

Convert AutoLinkFilesAction to JavafX luizphilipe2/jabref_experiment#32

Open

	<ButtonType fx:id="copyVersionButton" text="%Copy Version" />
	<ButtonType fx:constant="CLOSE" />


		fileKeystore.textProperty().bindBidirectional(viewModel.keyStoreProperty());

		browseKeystore.disableProperty().bind(useSSL.selectedProperty().not());

Uh oh!

Conversation

Siedlerchr commented May 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez commented May 22, 2018

Uh oh!

Siedlerchr commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented May 25, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Siedlerchr commented May 20, 2018 •

edited

Loading

Siedlerchr commented May 25, 2018 •

edited

Loading

Siedlerchr May 25, 2018 •

edited

Loading