[WIP] Convert Exporter Customization Dialog to javafx #4394

abepolk · 2018-10-21T09:27:09Z

This is my work so far. I am just beginning the dialog conversion and wanted to share my reasoning so far. What I am doing is listed in the comments in class ExportCustomizationDialogViewModel.

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

Siedlerchr · 2018-10-21T10:16:01Z

src/main/java/org/jabref/preferences/PreferencesService.java

@@ -35,4 +35,8 @@

    public void updateEntryEditorTabList();

+    List<String> getStringList(String key);


Please no generic methods here, create specific ones for the cases you need, e.g. get CustomExporters or getExporterormat... Or you create CustomExporter preferences which has all the needed information inside.

The Swing version has a class CustomExportList which holds a list of custom exporters and associated data. I was going to move some of the code here into the ExporterViewModel and other code into the ExporterCustomizationDialogViewModel. Do you recommend keeping the CustomExportList as it is?

The PreferencesService should only contain usecase-specific preference methods and the caller shouldn't care about how these information are stored in the preferences. So, here, the method would look like List<Exporter> getCustomExportFormats() and then in the implementation there would be calls like preferences.getStringList(JabRefPreferences.CUSTOM_EXPORT_FORMAT + i)).

Thus, most of the code in CustomExportList should go into JabrefPreferences.

Shouldn't the methods in preferencesService match those invoked in ExportCustomizationDialogViewModel as exporters.add(preferences.X) where X is the method, rather than using preferences.getStringList?

Sorry, I don't really understand what you mean. The PreferencesService should provide an interface to the preferences that hide the exact semantics of how the object is serialized. So abstractly, X getX() and putX(X newValue), where X is the (model) object. The implementation in JabRefPreferences than has to worry about (de)serialization.

Quick question: Do you recommend writing tests for the view model? The dialog is simple, but not as simple as the previous one I wrote.

I'm not a big fan of writing tests for the sake of it. My strategy is usually to program everything and when I discover a problem in the logic during testing, I'll write a test for it. Hence, if the dialog is simple enough that you don't make any errors, then you don't need to write tests ;-).
An exception is when there is really complex logic that is definitely worth testing.

tobiasdiez

Most of your comments in the code make sense to me. I've added some further remarks.

tobiasdiez · 2018-10-21T14:43:16Z

src/main/java/org/jabref/gui/exporter/ExportCustomizationDialogViewModel.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ExportCustomizationDialogViewModel extends BaseDialog<Void> {


The View/Dialog class should derive from BaseDialog and not the view model.

tobiasdiez · 2018-10-21T14:49:19Z

src/main/java/org/jabref/preferences/PreferencesService.java

@@ -35,4 +35,8 @@

    public void updateEntryEditorTabList();

+    List<String> getStringList(String key);


The PreferencesService should only contain usecase-specific preference methods and the caller shouldn't care about how these information are stored in the preferences. So, here, the method would look like List<Exporter> getCustomExportFormats() and then in the implementation there would be calls like preferences.getStringList(JabRefPreferences.CUSTOM_EXPORT_FORMAT + i)).

Thus, most of the code in CustomExportList should go into JabrefPreferences.

tobiasdiez · 2018-10-21T14:53:10Z

src/main/java/org/jabref/gui/exporter/ExportCustomizationDialogViewModel.java

+    }
+
+    //possibly not necessary, and if so getSortedList will have to return the correct type, not Eventlist<List<String>>
+    public List<ExporterViewModel> loadExporters() {


You might don't need the gui-wrapper ExporterViewModel around Exporter. In general such a wrapper is needed to adapt the properties of the model to display it to the user. But, here, the data of an exporter is relatively simple and the already in a suitable format for the gui.

I have put the data in the GUI wrapper ExporterViewModel for now. How would I create the TableView and TableColumns without a ViewModel for the data? All the examples I could find in JabRef and online had some sort of ViewModel.

You can also simply use Exporter as the datatype for the TableView. But if you have already created a ViewModel for it, keep it. It is probably the cleaner approach and more versatile for the future.

Okay. I've finished, but not tested, most of the ViewModel. I just have to add the dialog to add or modify the exporter, and then create the View and FXML. Unless there's something I forgot...

abepolk · 2018-10-28T00:01:00Z

At this point, I've refactored most of the code from the dialogs. The reasoning is mostly implemented already or in the comments. Take a look if you have time and let me know what you think! Otherwise, I can keep going and let you know when I'm done.

Siedlerchr · 2018-10-29T07:15:59Z

src/main/java/org/jabref/gui/exporter/CreateModifyExporterDialogViewModel.java

+            .addExtensionFilter(Localization.lang("Custom layout file"), StandardFileType.LAYOUT)
+            .withDefaultExtension(Localization.lang("Custom layout file"), StandardFileType.LAYOUT)
+            .withInitialDirectory(getExportWorkingDirectory()).build();
+        dialogService.showFileOpenDialog(fileDialogConfiguration).ifPresent(f -> setText(f.toAbsolutePath().toString())); //implement setting the text


To set the text, you would create a StringProperty here, e.g. selectedPath and then bind that property to a textfield in the View

Fixed it. I think I had the property, but I hadn't realized this is what I needed here. Thanks for your recommendation!

Siedlerchr · 2018-10-29T07:17:38Z

src/main/java/org/jabref/gui/exporter/ExportCustomizationDialogViewModel.java

+
+    }
+
+    public void modifyExporter(int row) {


Do you really need an int here? It would be generally better if you just operate on the row object

I don't think I need it. I changed it to use a property selectedExporters.

abepolk · 2018-11-02T21:04:52Z

Hi, I am having trouble using EasyBind to bind a ListProperty in the view model to a table view in the View. Specifically, I want to bind viewModel.selectedExportersProperty to the selected rows in exporterTable within class ExportCustomizationDialogView. I am using KeyBindingsDialogView as an example of how to use EasyBind. However, KeyBindingsDialogView uses a treeTable structure in lines 56-59, which is not what I want in my dialog. I have spent at least four hours today reading about JavaFX, data binding, properties, observers, and other things, but I still can't figure it out. I have some ideas on the recent pull request, but Eclipse still throws compiler errors. Any ideas on what I should do?

Siedlerchr · 2018-11-02T21:30:59Z

@NorwayMaple No need to complex binding. Just use setItems.
I created a simple patch fixing some of the issues you mentioned and generated some stubs.
Remove the .txt extensions and import it with git am < file.patch

Another thing I just noticed, for the SimpleXYZProperty in javafx, there exists always an interface, e.g. XYZ Property. Always use that.

0001-fix-some-binding-issues.patch.txt

abepolk · 2018-11-02T23:42:07Z

Thank you! I took a brief look at it, and it looks good. In the future, how can I decide when to use EasyBind?

Siedlerchr · 2018-11-03T16:21:50Z

@NorwayMaple Easybind is only needed in rare cases. I would go with the rule of thumb, if you can't achieve your goal with normal javafx bindings, try Easbindings.

abepolk · 2018-11-03T18:55:45Z

I finished the View and am trying to load it from the JabRef menu, but Afterburner throws an error:

14:44:21.187 [JavaFX Application Thread] ERROR org.jabref.FallbackExceptionHandler - Uncaught exception occurred in Thread[JavaFX Application Thread,5,main] - java.lang.IllegalStateException: Cannot load org.jabref.gui.exporter.exportcustomizationdialog
	at com.airhacks.afterburner.views.ViewLoader.load(ViewLoader.java:174)

etc...

I grepped exportcustomizationdialog and ExportCustomizationDialog, which is the old Swing version, in src/, and all references to the old Swing dialog are in the Swing dialog itself. Any ideas what is going on?

abepolk · 2018-11-27T01:59:55Z

I made a number of the changes. For some reason, the cancel button is not showing up in the subdialog. Also, I tried to use a try/catch block to catch an IndexOutOfBoundsException when selectedExporters.get(0) is called on an empty list, i.e. no exporters are selected. However the exception doesn't get caught. Separately (but not pushed), I tried just doing a check for it if (selectedExporters.isEmpty()) {return;}, but that led to the same uncaught exception.

Siedlerchr · 2018-11-27T07:33:19Z

src/main/java/org/jabref/gui/exporter/ExportCustomizationDialogViewModel.java

+        try {
+            exporterToModify = selectedExporters.get(0);
+            dialog = new CreateModifyExporterDialogView(exporterToModify, dialogService, preferences, loader);
+        } catch (IndexOutOfBoundsException ex) {


normally isEmpty should work. Does the indexoutofbounds exception realy targets to this line?

It's not clear. I put debug breakpoints on lines 56, 57, 59, and 60. I also put one in the line viewModel.modifyExporter() in ExporterCustomizationDialogView. That one is the only one it stops at before it throws the exception, but I don't understand how that is possible, since there should be no intervening lines. Also I find it odd that it should throw the very same exception that I was trying to catch.

You can try to make selectedExporters a normal array list.

I don't think it has to do with selectedExporters because it returns at line 59 when none are selected. Something else may be going on.

abepolk · 2018-12-07T15:39:01Z

@Siedlerchr @tobiasdiez I think I have finished the corrections you guys recommended. Do you think anything remains to be changed?

Thanks,
Abe

tobiasdiez

Thanks for the followup. The code looks very good to me. Please remove the outdated comments (I only marked a few, there might be more) and maybe run the code formatter on the files (also on the fxml ones), then this is good to go.

tobiasdiez · 2018-12-07T23:47:20Z

src/main/java/org/jabref/gui/exporter/ExportCustomizationDialogView.java

+
+    @Inject private DialogService dialogService;
+    @Inject private PreferencesService preferences;
+    @Inject private JournalAbbreviationLoader loader; // Should this be injected?


tobiasdiez · 2018-12-07T23:47:41Z

src/main/java/org/jabref/gui/exporter/ExportCustomizationDialogViewModel.java

+    }
+
+    private void loadExporters() {
+        List<TemplateExporter> exportersLogic = preferences.getCustomExportFormats(loader); //Var exportersLogic may need more descriptive name


abepolk · 2018-12-08T19:47:17Z

I removed most of the comments - let me know if I need to remove the last few. For the code formatter, I ran it, then git reset --hard and just removed some extra newlines (it wanted me to add indents that didn't make sense as well). Thanks for your help!

tobiasdiez

Looks good to me. Thanks a lot for all the work you put in this PR. I will now press the magic button 😃

* upstream/master: Bump applicationinsights-logging-log4j2 from 2.2.1 to 2.3.0 (#4540) Bump antlr4-runtime from 4.7.1 to 4.7.2 (#4542) Bump applicationinsights-core from 2.2.1 to 2.3.0 (#4541) Bump antlr4 from 4.7.1 to 4.7.2 (#4543) Fix journal name "Astronomy Journal" to "Astronomical Journal" fix-for-issue-4489 (#4538) Sorting of read-status isn't working as expected #4521 (#4536) Add preselect last used export format in export to clipboard dialog (#4533) fixed 4365 put html in clipboard (#4519) [WIP] Convert Exporter Customization Dialog to javafx (#4394) Fixes #4437 (#4531) Bump checkstyle from 8.14 to 8.15 (#4528) # Conflicts: # src/main/java/org/jabref/preferences/PreferencesService.java

* upstream/master: (49 commits) improve styling of preferences side menu (#4556) Cleanup interfaces (#4553) Bump fontbox from 2.0.12 to 2.0.13 (#4552) Bump pdfbox from 2.0.12 to 2.0.13 (#4551) Bump wiremock from 2.19.0 to 2.20.0 (#4550) Fixes that renaming a group did not change the group name in the interface (#4549) Bump applicationinsights-logging-log4j2 from 2.2.1 to 2.3.0 (#4540) Bump antlr4-runtime from 4.7.1 to 4.7.2 (#4542) Bump applicationinsights-core from 2.2.1 to 2.3.0 (#4541) Bump antlr4 from 4.7.1 to 4.7.2 (#4543) Fix journal name "Astronomy Journal" to "Astronomical Journal" fix-for-issue-4489 (#4538) Sorting of read-status isn't working as expected #4521 (#4536) Add preselect last used export format in export to clipboard dialog (#4533) fixed 4365 put html in clipboard (#4519) [WIP] Convert Exporter Customization Dialog to javafx (#4394) Fixes #4437 (#4531) Bump checkstyle from 8.14 to 8.15 (#4528) Correct PNAS abbrev. (#4524) Create test (#4518) ...

abepolk added 3 commits October 19, 2018 09:57

Exporter to JavaFX commit 1

f120b41

Work on Export Customization Dialog VM

12e3e7d

Work on VM

ec364ad

Siedlerchr reviewed Oct 21, 2018

View reviewed changes

tobiasdiez reviewed Oct 21, 2018

View reviewed changes

abepolk added 8 commits October 25, 2018 07:19

Combine methods in CustomExporterList and move to preferences

f5c89fa

work on VM

6773b22

Add methods to VM

1f318f0

Fix typos

1a0b760

started work on exporter subdialog

5c65b57

Continue work on exporter subdialog

2de4124

Implement save exporter from add/modify exporter dialog

a869aa2

Add two comments

9ae84c0

More work on VMs and prefs

e992c5d

Siedlerchr reviewed Oct 29, 2018

View reviewed changes

abepolk added 3 commits October 29, 2018 11:28

Fix browse method in export customization subdialog

3a4baa3

Remove int reference in VM

f63d43c

Create FXML for main exporter customization dialog

1f094d6

Siedlerchr mentioned this pull request Nov 1, 2018

Add docbook 5 support #4319

Merged

6 tasks

abepolk added 2 commits November 1, 2018 20:36

Work on View, not complete

83b7dac

Work on using EasyBind

99d7041

fix some binding issues add stubs

0479893

More work on Export Customization Dialog

547898c

abepolk added 8 commits November 26, 2018 11:53

Remove SortedList TODO

df42676

Inline closeDialog

f31bc51

loadExporters to private

d95a1aa

forEach to Stream and other fixes

80ffa33

inline init

5f56c25

Fixes to ExporterViewModel

f1d920e

Remove Optional

f22e97a

Add constants to TemplateExporter

252ecbb

Siedlerchr reviewed Nov 27, 2018

View reviewed changes

abepolk added 11 commits November 29, 2018 14:36

Simplify getExtensions without dot

ccdbf1f

Merge branch 'master' into exporter_to_javafx

aba31a6

Change try-catch to empty list check

8cd7ff3

Remove unnecessary method

3e09a7b

Fix property getters

3051022

Fix property getters again

0ef3061

Fix typo

7bf6dd6

Update references to customFormats

93838de

Add import

a79d037

Travis CI fixes

70479cb

Further Travis CI fixes

bf22adf

tobiasdiez requested changes Dec 7, 2018

View reviewed changes

abepolk added 2 commits December 8, 2018 14:39

Remove extra comments

c2b3489

Remove extra newlines

a364e1d

tobiasdiez approved these changes Dec 9, 2018

View reviewed changes

tobiasdiez merged commit 41f2d61 into JabRef:master Dec 9, 2018

		@@ -35,4 +35,8 @@

		public void updateEntryEditorTabList();

		List<String> getStringList(String key);

[WIP] Convert Exporter Customization Dialog to javafx #4394

[WIP] Convert Exporter Customization Dialog to javafx #4394

Conversation

abepolk commented Oct 21, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abepolk commented Oct 28, 2018

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

abepolk commented Nov 2, 2018

Siedlerchr commented Nov 2, 2018

abepolk commented Nov 2, 2018

Siedlerchr commented Nov 3, 2018

abepolk commented Nov 3, 2018

abepolk commented Nov 27, 2018

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

abepolk commented Dec 7, 2018

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abepolk commented Dec 8, 2018

tobiasdiez left a comment

Choose a reason for hiding this comment