Journal abbreviations dialog ported to JavaFX #1526

boceckts · 2016-06-29T13:58:31Z

I tried to extract the logic in a test driven approach to completely separate ui and ui-model and to have a high code test coverage. The design of the dialog was made after the ui-model was finisehd and is close to what was specified in #1396.

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)

Siedlerchr · 2016-06-29T15:32:09Z

src/main/java/net/sf/jabref/gui/journals/ManageJournalAbbreviationsView.java

+
+                            @Override
+                            protected void updateItem(String s, boolean b) {
+                                super.updateItem(s, b);


Try using a lambda for the callback, makes the code more readable:
http://stackoverflow.com/questions/23978676/convert-callback-into-java-8-lambda-expression

Please use more speaking variable names for s and b, for example like here:
http://code.makery.ch/blog/javafx-8-tableview-cell-renderer/

Siedlerchr · 2016-06-29T15:44:15Z

Looks good 👍 !
Instead of the "+" Button would suggest simply the text "Add".
What is the purpose of the "-" Icon? Deletion? If yes, a kind of trash can symbol would be more suitable

Siedlerchr · 2016-07-04T14:15:12Z

src/main/java/net/sf/jabref/gui/journals/AbbreviationsFile.java

-    public AbbreviationsFile(File file) {
-        super(file.getAbsolutePath());
+    public AbbreviationsFile(String filePath) {
+        path = FileSystems.getDefault().getPath(filePath.replaceAll("\\\\", "/"));


You could simply use Paths.get(filePath) this is a shorthand for the FileSystems.getDefault...

What are you trying to do here? I do not understand the Replace stuff... In case you are trying to escape sth like Slashes, that is absolutely not necessary, as the Path methods are able to handle both relative and absolute.
Have a look here for some examples
https://docs.oracle.com/javase/tutorial/essential/io/pathOps.html

The idea was indeed to escape the backslashes for windows systems...but thanks for the tip

boceckts · 2016-07-11T18:58:08Z

I updated the description and added screenshots for the latest commit. Is it really the best idea to add the "+" to the end of the list of journal abbrevaitions? This would mean the user always has to scroll to the bottom to add more journal abbreviations to the list. In my opinion this is less ideal than having a button that is always visible and in the same place. Any thoughts?

tschechlovdev · 2016-07-12T15:10:06Z

src/main/java/net/sf/jabref/gui/journals/ManageJournalAbbreviationsViewModel.java

+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+


Could you add for all classes, that you've added, a short comment?

tobiasdiez · 2016-07-12T16:53:45Z

Personally, I prefer inline editing for lists/tables with only a handful of columns. See for example http://designingwebinterfaces.com/ultimate-guide-to-table-ui-patterns. The very first row could be an "insert row", i.e. it shows (in italic) "Create new abbreviation" and when you click you just have an empty textfield to insert the new abbreviation.

Other from that, I really like how the dialog looks. Maybe change the close/remove icon? Btw what is the purpose of this "JabRef Lists" button at the bottom?

boceckts · 2016-07-12T17:10:00Z

@tobiasdiez so when clicking on the edit icon on the right the column should be editable?
The "JabRef Lists" button shows the jabref built in journal abbreviations which are not modifyable. In the swing version there is a "view" button to show these abbreviations in a separate window.

tobiasdiez · 2016-07-12T17:44:14Z

@boceckts Yes exactly, clicking edit (and I would also support double clicking) makes the row editable. Mhh on second thought, one might not even need the edit button but show it directly in the cell upon mouse hover and a single click then makes the cell editable. Decide based on what is easier to implement.

For the "JabRef List" button, you also have a JabRef built in list in the drop-down. What is the difference here?

boceckts · 2016-07-12T17:49:12Z

@tobiasdiez The built in lists in the drop down will only appear when the toggle button is activated

tobiasdiez · 2016-07-12T18:18:54Z

Is there a need to hide it from the drop-down list? If not, then I would just remove the toggle button.

boceckts · 2016-07-12T18:22:22Z

@tobiasdiez I just thought it's nice to give the user the option to only display their custom lists

koppor · 2016-07-13T18:12:10Z

@boceckts Please remove "JabRef lists" button. 🌻

codecov-io · 2016-07-18T14:28:46Z

Current coverage is 10.79% (diff: 4.20%)

Merging #1526 into javafx will increase coverage by 0.02%

@@             javafx      #1526   diff @@
==========================================
  Files           705        707     +2   
  Lines         46412      46379    -33   
  Methods           0          0          
  Messages          0          0          
  Branches       7637       7630     -7   
==========================================
+ Hits           5002       5008     +6   
+ Misses        40963      40921    -42   
- Partials        447        450     +3

Powered by Codecov. Last update bc6f0c9...0524ca4

tschechlovdev · 2016-08-06T09:48:38Z

src/main/java/net/sf/jabref/gui/journals/ManageJournalAbbreviationsView.java

+            @Override
+            protected void updateItem(Boolean isPseudoAbbreviation, boolean isEmpty) {
+                super.updateItem(isPseudoAbbreviation, isEmpty);
+                if (isPseudoAbbreviation != null) {


is this really needed ? When can this be null?

Yes it is necessary because we have to call setGraphic(null) if the item to update is null. Otherwise we get some weird graphic bugs when switching from a custom list to a built in list since the graphics don't get reset.

tobiasdiez · 2016-08-14T11:25:50Z

src/test/java/net/sf/jabref/gui/journals/ManageJournalAbbreviationsTest.java

+                .observableArrayList(Globals.journalAbbreviationLoader.getBuiltInAbbreviations());
+        ObservableList<AbbreviationViewModel> actual = viewModel.abbreviationsProperty().get();
+        ObservableList<Abbreviation> actualAbbreviations = FXCollections.observableArrayList();
+        actual.forEach(abbViewModel -> actualAbbreviations.add(abbViewModel.getAbbreviationObject()));


use map function, viewModel.getAbbrevations().stream().map(getAbbrevaitonObject).collect()

tobiasdiez · 2016-08-14T11:32:15Z

I just made some final remarks. After these are fixed, this PR can be merged in my opinion. @koppor pls do the final review & merge, thanks.

@boceckts (and the all others who worked on this PR): good job! I really like your code, especially that you wrote tests to implement the logic! One can see that you learned a lot in the last few weeks.

- foreach converted to stream - return path as optional - updated language - test class completely refactored

the view model now decides whether a file or abbreviation can be edited or removed and provides boolean properties for easy binding with the ui components

boceckts · 2016-08-27T17:30:57Z

I think I now addressed all of your comments.

tobiasdiez · 2016-09-01T15:14:42Z

I'll merge this now, since it looks good to me and all the javafx code will be finally reviewed anyway.

koppor · 2016-09-07T17:59:39Z

Does not work in javafx. @boceckts Can you please check?

Exception in thread "JavaFX Application Thread" java.lang.IllegalStateException: Cannot load net.sf.jabref.gui.journals.managejournalabbreviations
        at com.airhacks.afterburner.views.FXMLView.loadSynchronously(FXMLView.java:94)
        at com.airhacks.afterburner.views.FXMLView.initializeFXMLLoader(FXMLView.java:101)
        at com.airhacks.afterburner.views.FXMLView.getView(FXMLView.java:113)
        at net.sf.jabref.gui.journals.ManageJournalAbbreviationsView.showAndWait(ManageJournalAbbreviationsView.java:201)
        at net.sf.jabref.gui.journals.ManageJournalsAction.lambda$actionPerformed$0(ManageJournalsAction.java:21)
        at com.sun.javafx.application.PlatformImpl.lambda$null$173(PlatformImpl.java:295)
        at java.security.AccessController.doPrivileged(Native Method)
        at com.sun.javafx.application.PlatformImpl.lambda$runLater$174(PlatformImpl.java:294)
        at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
        at com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
        at com.sun.glass.ui.win.WinApplication.lambda$null$148(WinApplication.java:191)
        at java.lang.Thread.run(Thread.java:745)
Caused by: javafx.fxml.LoadException: No resources specified.
/C:/git-repositories/jabref/jabref/build/resources/main/net/sf/jabref/gui/journals/managejournalabbreviations.fxml:22

        at javafx.fxml.FXMLLoader.constructLoadException(FXMLLoader.java:2597)

boceckts · 2016-09-07T18:08:16Z

@koppor I will look into it, did this happen after the javafx branch has been updated?

boceckts · 2016-09-07T18:40:13Z

@koppor Ok so I tracked the issue and came to the conclusion that the this pr #1918 causes the exception. As described in the pr's description @tobiasdiez had to out comment the line bundle = Localization.getMessages(); since otherwise the localization tests would fail. After adding it back it works for me again.

tobiasdiez · 2016-09-07T18:42:13Z

@boceckts do the localization tests also work after readding the bundle = ... line?

boceckts · 2016-09-07T18:49:59Z

No but it gives me the same errors when the line is out commented, I'm not sure if this statement is causing the failing tests..

koppor · 2016-09-07T19:50:44Z

Yes, this happens with the current javafx branch (merge of master). I did not check the state before the merge. My gut feeling is that the merge broke it. But I'm not sure.

I is not about failing tests, it is about that "Options -> Manage Journal Abbreviations" is a noop.

boceckts · 2016-09-07T19:56:10Z

Yes it's about opening the dialog. But still, re-adding the line as described by me two comments ago fixes the issue for me.

tobiasdiez · 2016-09-07T19:57:55Z

Reenabled in #1935.

boceckts added the stupro label Jun 29, 2016

Siedlerchr reviewed Jun 29, 2016
View reviewed changes

Siedlerchr reviewed Jul 4, 2016
View reviewed changes

tschechlovdev reviewed Jul 12, 2016
View reviewed changes

tobiasdiez added the status: devcall label Jul 12, 2016

koppor removed the status: devcall label Jul 13, 2016

boceckts added the stupro-ready-for-internal-review label Aug 1, 2016

tschechlovdev reviewed Aug 6, 2016
View reviewed changes

tobiasdiez reviewed Aug 14, 2016
View reviewed changes

tobiasdiez assigned koppor and unassigned tobiasdiez Aug 14, 2016

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 15, 2016

boceckts added 8 commits August 15, 2016 16:25

More Code refactoring

57a86a0

- foreach converted to stream - return path as optional - updated language - test class completely refactored

Moved listeners to view model

5a3146f

Moved enable disable logic to the view model

6644587

the view model now decides whether a file or abbreviation can be edited or removed and provides boolean properties for easy binding with the ui components

Fix pseudoAbbreviation not showing add button

2e9418b

Fixed wrong button tool tips

1d22ad8

Fix abbreviation couldn't be edited

5bca5cc

Refactored view model so less code is needed

96cabeb

Saving abbreviations moved to new background thread

86a36c4

boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 31, 2016

tobiasdiez merged commit 94f3d13 into JabRef:javafx Sep 1, 2016

boceckts deleted the JournalAbbreviationsFX branch September 5, 2016 10:00

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Journal abbreviations dialog ported to JavaFX #1526

Journal abbreviations dialog ported to JavaFX #1526

boceckts commented Jun 29, 2016 •

edited

Loading

Siedlerchr Jun 29, 2016

Siedlerchr commented Jun 29, 2016

Siedlerchr Jul 4, 2016

boceckts Jul 6, 2016

boceckts commented Jul 11, 2016

tschechlovdev Jul 12, 2016

tobiasdiez commented Jul 12, 2016

boceckts commented Jul 12, 2016

tobiasdiez commented Jul 12, 2016

boceckts commented Jul 12, 2016

tobiasdiez commented Jul 12, 2016

boceckts commented Jul 12, 2016

koppor commented Jul 13, 2016

codecov-io commented Jul 18, 2016 •

edited

Loading

tschechlovdev Aug 6, 2016

boceckts Aug 8, 2016

tobiasdiez Aug 14, 2016

tobiasdiez commented Aug 14, 2016

boceckts commented Aug 27, 2016

tobiasdiez commented Sep 1, 2016

koppor commented Sep 7, 2016

boceckts commented Sep 7, 2016

boceckts commented Sep 7, 2016

tobiasdiez commented Sep 7, 2016

boceckts commented Sep 7, 2016

koppor commented Sep 7, 2016

boceckts commented Sep 7, 2016

tobiasdiez commented Sep 7, 2016


		import org.apache.commons.logging.Log;
		import org.apache.commons.logging.LogFactory;

Journal abbreviations dialog ported to JavaFX #1526

Journal abbreviations dialog ported to JavaFX #1526

Conversation

boceckts commented Jun 29, 2016 • edited Loading

Siedlerchr Jun 29, 2016

Choose a reason for hiding this comment

Siedlerchr commented Jun 29, 2016

Siedlerchr Jul 4, 2016

Choose a reason for hiding this comment

boceckts Jul 6, 2016

Choose a reason for hiding this comment

boceckts commented Jul 11, 2016

tschechlovdev Jul 12, 2016

Choose a reason for hiding this comment

tobiasdiez commented Jul 12, 2016

boceckts commented Jul 12, 2016

tobiasdiez commented Jul 12, 2016

boceckts commented Jul 12, 2016

tobiasdiez commented Jul 12, 2016

boceckts commented Jul 12, 2016

koppor commented Jul 13, 2016

codecov-io commented Jul 18, 2016 • edited Loading

Current coverage is 10.79% (diff: 4.20%)

tschechlovdev Aug 6, 2016

Choose a reason for hiding this comment

boceckts Aug 8, 2016

Choose a reason for hiding this comment

tobiasdiez Aug 14, 2016

Choose a reason for hiding this comment

tobiasdiez commented Aug 14, 2016

boceckts commented Aug 27, 2016

tobiasdiez commented Sep 1, 2016

koppor commented Sep 7, 2016

boceckts commented Sep 7, 2016

boceckts commented Sep 7, 2016

tobiasdiez commented Sep 7, 2016

boceckts commented Sep 7, 2016

koppor commented Sep 7, 2016

boceckts commented Sep 7, 2016

tobiasdiez commented Sep 7, 2016

boceckts commented Jun 29, 2016 •

edited

Loading

codecov-io commented Jul 18, 2016 •

edited

Loading