Dialogstojavafx by Siedlerchr · Pull Request #3801 · JabRef/jabref

Siedlerchr · 2018-03-02T22:54:18Z

I have converted a lot of dialogs to javafx.
I did not test each one.

Some of them are going to be replaced in the near future (e.g. the Strings dialog and the import of files/drag and drop stuff), so I did not convert them.

Dialogs which need special treatment are marked with TODO statements.
Some of the candidates are the ones that integrate html or the preview pane

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

start with preferences

tobiasdiez

Thanks for having a look at this! This is a huge step to make the maintable-branch usable without dialogs hiding behind the main window. I have a few comments, but these are generally only minor ones.

tobiasdiez · 2018-03-02T23:06:00Z

        actions.put(Actions.OPEN_URL, new OpenURLAction());

-        actions.put(Actions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(this));
+        actions.put(Actions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(this, frame));


could you please pass the DialogService directly, instead of the frame (makes the dependency clearer).

tobiasdiez · 2018-03-02T23:08:46Z

            try {
-                SavePreferences prefs = SavePreferences.loadForSaveFromPreferences(Globals.prefs).withMakeBackup(false)
-                        .withEncoding(panel.getBibDatabaseContext().getMetaData().getEncoding()
+                SavePreferences prefs = SavePreferences.loadForSaveFromPreferences(Globals.prefs)


The code style settings still seem to be off. The dots are not aligned (if this is not an artifact of the way github displays the code)

tobiasdiez · 2018-03-02T23:09:09Z

        actions.put(Actions.MOVE_TO_GROUP, new GroupAddRemoveDialog(this, true, true));

-        actions.put(Actions.DOWNLOAD_FULL_TEXT, new FindFullTextAction(this));
+        actions.put(Actions.DOWNLOAD_FULL_TEXT, new FindFullTextAction(frame, this));


tobiasdiez · 2018-03-02T23:09:22Z

        entryTypes.addAll(EntryTypes.getAllTypes(bibDatabaseMode));

-        typeComp = new EntryTypeList(entryTypes, bibDatabaseMode);
+        typeComp = new EntryTypeList(frame, entryTypes, bibDatabaseMode);


tobiasdiez · 2018-03-02T23:15:40Z

-            JOptionPane.showMessageDialog(null, Localization.lang("Invalid URL") + ": " + ex.getMessage(),
-                    Localization.lang("Download file"), JOptionPane.ERROR_MESSAGE);
            LOGGER.info("Error while downloading " + "'" + res + "'", ex);
+            frame.getDialogService().showErrorDialogAndWait(Localization.lang("Download file"), Localization.lang("Invalid URL") + ": " + ex.getMessage(), ex);


This code should display the error message twice if I'm not mistaken.

tobiasdiez · 2018-03-02T23:23:07Z

                    Localization.lang("You must enter an integer value in the interval 1025-65535 in the text field for")
                            + " '" + Localization.lang("Remote server port") + '\'',
-                    Localization.lang("Remote server port"), JOptionPane.ERROR_MESSAGE);
+                    ex);


the exception as an argument is unneeded here, since the stack trace is not helpful at all in this situation: the user entered data in the wrong format and this is said already in the error message.

tobiasdiez · 2018-03-02T23:24:03Z


-
-    public GeneralTab(JabRefPreferences prefs) {
+    public GeneralTab(JabRefFrame frame, JabRefPreferences prefs) {


directly pass the dialogservice pls

tobiasdiez · 2018-03-02T23:24:42Z

-                    (null, Localization.lang("The chosen date format for new entries is not valid"),
-                            Localization.lang("Invalid date format"),
-                            JOptionPane.ERROR_MESSAGE);
+            JOptionPane.showMessageDialog(null, Localization.lang("The chosen date format for new entries is not valid"),


you missed this one ;-)

tobiasdiez · 2018-03-02T23:25:14Z

-        tabs.add(new NetworkTab(prefs));
-        tabs.add(new AdvancedTab(prefs));
-        tabs.add(new AppearancePrefsTab(prefs));
+        tabs.add(new NetworkTab(frame, prefs));


dialogservice everywhere

tobiasdiez · 2018-03-02T23:28:27Z

                JFXPanel container = CustomJFXPanel.wrap(new Scene(testPane));
                container.setPreferredSize(new Dimension(800, 350));
+
+                //TODO: Add dialog for showing pane


This should be easy using

jabref/src/main/java/org/jabref/gui/DialogService.java

Lines 114 to 120 in f7d9678

/**

* This will create and display a new dialog showing a custom {@link DialogPane}

* and using custom {@link ButtonType}s.

*

* @return Optional with the pressed Button as ButtonType

*/

Optional<ButtonType> showCustomDialogAndWait(String title, DialogPane contentPane, ButtonType... buttonTypes);

…javafx * upstream/maintable-beta: Change open last edited dialgo to javafx (#3800)

convert some more dialogs Fix index out of bounds in Entry Table columns Pref Tab

Display charsets in Choices dialog

Extend choice dialog with cancel button and ok button label

tobiasdiez · 2018-03-03T22:49:07Z

-                        }
-                        if (answer == JOptionPane.NO_OPTION) {
+
+                        //  return dialogService.showConfirmationDialogWithOptOutAndWait(title, message,


please remove these comments

tobiasdiez · 2018-03-03T22:52:10Z

+                    Localization.lang("Are you sure you want to remove the style?"),
+                    Localization.lang("Remove style"),
+                    Localization.lang("Cancel"));
+            if (!style.isFromResource() && removeStylePressed) {


previously the dialog was only shown if style.isFromResource returned false. Is this change of behavior desirable?

Ah shit, I oversaw that condiation

tobiasdiez · 2018-03-03T22:53:08Z

        removeAction = actionEvent -> getSelectedTermsList().ifPresent(list -> {

-            if (!list.isInternalList() && (JOptionPane.showConfirmDialog(diag,
+            boolean removeProctedTermsFilePressed = frame.getDialogService().showConfirmationDialogAndWait(Localization.lang("Remove protected terms file"),


tobiasdiez · 2018-03-03T22:56:07Z


    private static FXDialog createDialogWithOptOut(AlertType type, String title, String content,
-                                                   String optOutMessage, Consumer<Boolean> optOutAction) {
+            String optOutMessage, Consumer<Boolean> optOutAction) {


This seems to be another inconsistency between eclipse and intellj style

I will investigate that

tobiasdiez · 2018-03-03T22:58:15Z

-                    Localization.lang("You are already connected to a database using entered connection details."),
-                    Localization.lang("Warning"), JOptionPane.WARNING_MESSAGE);
+
+            frame.getDialogService().showWarningDialogAndWait(Localization.lang("Warning"),


Use a better title?

…javafx * upstream/maintable-beta: Enable travis build for maintable-beta (#3804)

Remove comment form import insepction dlg

tobiasdiez · 2018-03-04T14:16:02Z


    @Override
-    public void pushEntries(BibDatabase database, List<BibEntry> entries, String keys, MetaData metaData) {
+    public void pushEntries(BibDatabase database, List<BibEntry> entries, String keys, MetaData metaData, DialogService dialogService) {


The dialogService makes more sense as a constructor argument, I think. Probably push it even to AbstractPushToApplication

I thought so, too, but the concrete subclass of PushToEntries (operation) is just passed as constructur argument as a dependency, so it is not possible

Sorry, I don't understand what you mean. The push to application action of course only gets the PushToApplicaiton instance without knowing which concrete subclass it is. But this is not a problem as far as I can see since upon initialization each subclass would get a DialogService as a constructor argument.

The problem is: PushToApplicationAction already gets an instance of the concrete push operation and only calls pushEntries.
And these push-Operations are defined and instantiated here. I can't just pass the DialogService as Constructor arg.
The only thing I could do is to create an additional setter for the Dialog Service

jabref/src/main/java/org/jabref/gui/push/PushToApplications.java

Lines 16 to 20 in fd98056

applications = new ArrayList<>();

applications.add(new PushToEmacs());

applications.add(new PushToLyx());

applications.add(new PushToTexmaker());

Why can't you pass the dialog service as a constructor argument to PushToApplications? It seems the only place where this class is initiated is

jabref/src/main/java/org/jabref/gui/JabRefFrame.java

Line 580 in fd98056

pushApplications = new PushToApplications();

where you have a dialog service at your disposal.

and convert some open office dlgs, too

Fix l10n keys

tobiasdiez · 2018-03-04T15:59:13Z

-                                + "</html>",
-                        "", JOptionPane.ERROR_MESSAGE);
+
+        frame.getDialogService().showErrorDialogAndWait(Localization.lang("Undefined paragraph format"),


remove todo?

You were a second too early ;)

…javafx * upstream/maintable-beta: Remove unused import Add group coloring in maintable as replacement for marked entries (#3769) # Conflicts: # src/main/resources/l10n/JabRef_en.properties

tobiasdiez · 2018-03-05T15:11:40Z

Let's merge this before the size escalates ;-). Thanks a lot for your time and good work @Siedlerchr!

…drop * upstream/maintable-beta: (24 commits) Fixes #3796: Pretend that we have every translation for every key (#3820) adjust line wrapping on column replace open office install selection dialog with choice dialog Wrap error messages in fx executor (because abstract worker is swing thread) Convert swing error dialog in save db action to custom fx dialog Replace swing export dialog in ExportToClipboardAction with fx choices dlg run find fulltext dialog in fx thread fix cut, copy & paste action from toolbar Set resizable for custom dialog with dialog pane convert merge entries dlg to javafx and make action working again checkstyle argh! Update test order -> Execute checkstyle first (#3807) embed cleanup dialog in javafx Fix some non FX Thread issues Main drawback: Cleanup panel is in backgroung due to swing thread) Allow search field to be smaller Fix Codacy Unused Params, Fields (#3753) Dialogstojavafx (#3801) Remove unused import Add group coloring in maintable as replacement for marked entries (#3769) Enable travis build for maintable-beta (#3804) Remove setting of tooltip manually Change open last edited dialgo to javafx (#3800) ... # Conflicts: # src/main/java/org/jabref/gui/maintable/MainTable.java

…tomjfx * upstream/maintable-beta: (28 commits) Fixes #3796: Pretend that we have every translation for every key (#3820) adjust line wrapping on column replace open office install selection dialog with choice dialog Wrap error messages in fx executor (because abstract worker is swing thread) Convert swing error dialog in save db action to custom fx dialog Replace swing export dialog in ExportToClipboardAction with fx choices dlg run find fulltext dialog in fx thread fix cut, copy & paste action from toolbar Set resizable for custom dialog with dialog pane convert merge entries dlg to javafx and make action working again checkstyle argh! Update test order -> Execute checkstyle first (#3807) embed cleanup dialog in javafx Fix some non FX Thread issues Main drawback: Cleanup panel is in backgroung due to swing thread) Allow search field to be smaller Fix Codacy Unused Params, Fields (#3753) Dialogstojavafx (#3801) Remove unused import Add group coloring in maintable as replacement for marked entries (#3769) Enable travis build for maintable-beta (#3804) Remove setting of tooltip manually Change open last edited dialgo to javafx (#3800) ...

Siedlerchr added 4 commits March 2, 2018 22:58

convert dialogs to javafx

67c489e

start with preferences

convert open office and other preferences

ad502c4

Convert entryTypeList and entry customization and shared database

55a985f

And convert some more

bb66b22

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 2, 2018

tobiasdiez requested changes Mar 2, 2018

View reviewed changes

Siedlerchr and others added 9 commits March 3, 2018 19:10

Merge remote-tracking branch 'upstream/maintable-beta' into dialogsto…

ce6837f

…javafx * upstream/maintable-beta: Change open last edited dialgo to javafx (#3800)

Pass dialogService instead of frame

86dfdc3

convert some more dialogs Fix index out of bounds in Entry Table columns Pref Tab

entry types list

13500fe

create choice dialog

d05d253

ADd method for returning list in Encodings

f4b84d6

Display charsets in Choices dialog

Convert some more confirmation dialogs

71b17c2

convert some more dialogs

469a834

Extend choice dialog with cancel button and ok button label

Merge branch 'maintable-beta' into dialogstojavafx

6b1ef6f

fix checkstyle and add missing lang keys

cbb1df3

tobiasdiez requested changes Mar 3, 2018

View reviewed changes

Siedlerchr added 6 commits March 4, 2018 12:14

fix dialog conditions

9e7ff57

Merge remote-tracking branch 'upstream/maintable-beta' into dialogsto…

5b8b745

…javafx * upstream/maintable-beta: Enable travis build for maintable-beta (#3804)

add better title for share db dialog

9a04aa6

Add dialogservice to push to apps

4d1b6e7

Remove comment form import insepction dlg

add new keys

74391d4

fix dialog service in push

44bceed

tobiasdiez reviewed Mar 4, 2018

View reviewed changes

Siedlerchr added 2 commits March 4, 2018 16:39

checkstyle

cf82ad3

and convert some open office dlgs, too

use dialogservice as local variable

a43a7de

Fix l10n keys

tobiasdiez reviewed Mar 4, 2018

View reviewed changes

Siedlerchr added 3 commits March 5, 2018 15:11

pass Dialog service to push dialogs as ctor arg

7d4afe0

Merge remote-tracking branch 'upstream/maintable-beta' into dialogsto…

3dc6068

…javafx * upstream/maintable-beta: Remove unused import Add group coloring in maintable as replacement for marked entries (#3769) # Conflicts: # src/main/resources/l10n/JabRef_en.properties

checkstyle

43d38db

tobiasdiez merged commit 21355ab into maintable-beta Mar 5, 2018

tobiasdiez deleted the dialogstojavafx branch March 5, 2018 15:11



		public GeneralTab(JabRefPreferences prefs) {
		public GeneralTab(JabRefFrame frame, JabRefPreferences prefs) {

	/**
	* This will create and display a new dialog showing a custom {@link DialogPane}
	* and using custom {@link ButtonType}s.
	*
	* @return Optional with the pressed Button as ButtonType
	*/
	Optional<ButtonType> showCustomDialogAndWait(String title, DialogPane contentPane, ButtonType... buttonTypes);

	applications = new ArrayList<>();

	applications.add(new PushToEmacs());
	applications.add(new PushToLyx());
	applications.add(new PushToTexmaker());

Uh oh!

Conversation

Siedlerchr commented Mar 2, 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!

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!

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!

tobiasdiez Mar 4, 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!

Siedlerchr Mar 4, 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!

tobiasdiez commented Mar 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Siedlerchr commented Mar 2, 2018 •

edited

Loading

tobiasdiez Mar 4, 2018 •

edited

Loading

Siedlerchr Mar 4, 2018 •

edited

Loading