Add file description to gui and fix sync bug #3210

Siedlerchr · 2017-09-11T19:17:25Z

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)
If you changed the localization: Did you run gradle localizationUpdate?

Add tests for identifying file descr bug

tobiasdiez · 2017-09-12T09:31:55Z

The problem is that the changes in the underlying LinkedFile are not propagated to the list LinkedFilesEditorViewModel.files so that the serialization

jabref/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java

Lines 74 to 82 in 2c5e7f2

    
           private static String getStringRepresentation(List<LinkedFileViewModel> files) { 
        
               // Only serialize linked files, not the ones that are automatically found 
        
               List<LinkedFile> filesToSerialize = files.stream() 
        
                       .filter(file -> !file.isAutomaticallyFound()) 
        
                       .map(LinkedFileViewModel::getFile) 
        
                       .collect(Collectors.toList()); 
        
               return FileFieldWriter.getStringRepresentation(filesToSerialize); 
        
           }

is never called. By the way, also changes to the file name or icon do not trigger an update and thus are not serialized.

As a solution, I propose to convert the data fields in LinkedFile to real properties, add Observable[] getObservables() to LinkedFile and return these observables here:

jabref/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

Lines 109 to 111 in 2c5e7f2

    
           public Observable[] getObservables() { 
        
               return new Observable[] {this.downloadProgress, this.isAutomaticallyFound}; 
        
           }

Ps: Can you pls add a screenshot how the file editor now looks like with the description added (especially when the file link is relatively long). Thanks.

* upstream/master: Localization: French: Translation of a new string (#3212) Fix #2775: Hyphens in last names are properly parsed (#3209) Followup to Issue #3167 (#3202) Update mockito-core from 2.9.0 -> 2.10.0

Siedlerchr · 2017-09-13T11:10:42Z

@tobiasdiez I did as you suggested, but still does not work. I think it has something to do with the LIstProperty, but I am not that deep inside javafx binding stuff to know why it is not synced

tobiasdiez · 2017-09-13T11:48:53Z

src/main/java/org/jabref/model/entry/LinkedFile.java

+    }
+
+    public Observable[] getObservables() {
+        return new Observable[] {this.downloadProgress, this.isAutomaticallyFound};


you need to return description, link, fileType here (and move downloadProgress and isAutomaticallyFound back to the viewmodel).

The observable tell javafx which properties mark the object as changed, i.e. if description is changed this should mark the LinkedFile as changed and thus also LinkedFileViewModel and thus finally the list

jabref/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java

Line 55 in 23c6ac6

private final ListProperty<LinkedFileViewModel> files = new SimpleListProperty<>(FXCollections.observableArrayList(LinkedFileViewModel::getObservables));

.
For more details see https://docs.oracle.com/javase/8/javafx/api/javafx/collections/FXCollections.html#observableList-java.util.List-javafx.util.Callback- or https://stackoverflow.com/questions/31687642/callback-and-extractors-for-javafx-observablelist

Siedlerchr · 2017-09-13T14:08:01Z

Yeah! Got it now!

Fix serializing

add changelog

tobiasdiez

Codewise this looks good to me (except a few very minor remarks).
However, I'm still not convinced that we should add the description of the file to the list. In my opinion, it now looks cramped and has too much information, without really helping (for long file names, you need to scroll in order to read the description - which is more inconvenient then hovering the link). In my view it would be sufficient to decrease the time until the tooltip appears (which is relatively easy or see disscussion).

tobiasdiez · 2017-09-13T19:48:46Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

-        return new Observable[] {this.downloadProgress, this.isAutomaticallyFound};
+        List<Observable> observables = new ArrayList<>(Arrays.asList(linkedFile.getObservables()));
+        observables.add(downloadOngoing);
+        observables.add(downloadProgress);


also add isAutomaticallyFound

Just budding in here...

The file list would be cleaner if the filenames weren't shown when there is a description, as was done in JabRef 3.x. If there are very long descriptions, it will still be cluttered, but in my opinion, tooltips aren't an improvement.

The problem with descriptions only in tooltips is that you have to mouse over each one. This makes JabRef harder to use:

It forces you to lift your hands off the keyboard, but more importantly,

You can't scan the descriptions at a glance. Instead, you have to mouse over the first one, then mouse over the next one (which causes the previous description to vanish), and so on.

You can simulate the user experience by having somebody else make you a JabRef entry with, say five files with slightly different descriptions -- this is what your own entries will look like to you, a couple years after you have made them. You won't remember anything.

Now, try to decide which file to open. With tooltips, you see only one description at a time, like you're viewing them through a straw. Now compare that with the JabRef 3.x experience, where all descriptions are visible; with a glance, you can comprehend them all.

Sounds reasonable!

I now changed the order: Description is shown first and then the file link

Come to think of it, you could even make the filename a tooltip. Once you've got the description, the filename is probably not so useful, but in some situation, you still might want to know it.

But then again, there is the "see everything at a glance" argument I just made...

tobiasdiez · 2017-09-13T19:52:14Z

src/main/java/org/jabref/model/entry/FileFieldWriter.java

@@ -7,7 +7,7 @@

 public class FileFieldWriter {

-    private FileFieldWriter() {
+    FileFieldWriter() {


Why is this constructor not private anymore?

Was a relict from testing. Didn't see that it had static method on the first try

* upstream/master: Add tests for removal changes Improve telemetry (#3218) Real test linking real other entry (#3214) Check for explicit group at different location Update java-string-similarity from 0.24 -> 1.0.0 Only remove ExplicitGroups from entries, but not keyword-based groups Remove unused import in GroupTreeNode Move getEntriesInGroup to GroupTreeNode Inline Consumer updateViewModel Undo IntelliJ autoformat of FXML annotations Remove unused import Rewrite selection logic to avoid NPEs Clear group selection when a group is removed Also remove a group from entries when you remove it

Show description first then file link remove unnecesary separator

lenhard

Apart from a few very minor things, the code looks fine. +1 for merge.

lenhard · 2017-09-18T14:30:46Z

src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java

@@ -69,6 +69,7 @@ public LinkedFilesEditorViewModel(String fieldName, AutoCompleteSuggestionProvid
                text,
                LinkedFilesEditorViewModel::getStringRepresentation,
                this::parseToFileViewModel);
+


Very minor, but as far as I can see this blank line is unnecessary.

lenhard · 2017-09-18T14:31:02Z

src/main/java/org/jabref/model/entry/LinkedFile.java

+        out.writeUTF(getLink());
+        out.writeUTF(getDescription());
+        out.flush();
+


Very minor, but as far as I can see this blank line is unnecessary.

lenhard · 2017-09-18T14:31:14Z

src/main/java/org/jabref/model/entry/LinkedFile.java

+        fileType = new SimpleStringProperty(in.readUTF());
+        link = new SimpleStringProperty(in.readUTF());
+        description = new SimpleStringProperty(in.readUTF());
+


Very minor, but as far as I can see this blank line is unnecessary.

* upstream/master: Don't highlight odd rows in file list editor (#3223) Fix assertion by removing typo (#3220) Update assertion to change of online reference (#3221) Put in null return Reformatted code, renamed method, added try/catch Fix null return Changed from Path to Optional<Path> Added logic to check whether linked file already exists

tobiasdiez · 2017-09-20T10:17:34Z

I just discovered that there is additional space between the icon and the file name if the file doesn't have a description. Can you please fix this. Thanks!

Siedlerchr · 2017-09-20T12:01:10Z

Fixed in master.

* upstream/master: (188 commits) Show file description only if not empty Add file description to gui and fix sync bug (#3210) Don't highlight odd rows in file list editor (#3223) Fix assertion by removing typo (#3220) Update assertion to change of online reference (#3221) Put in null return Reformatted code, renamed method, added try/catch Add tests for removal changes Improve telemetry (#3218) Real test linking real other entry (#3214) Check for explicit group at different location Update java-string-similarity from 0.24 -> 1.0.0 Only remove ExplicitGroups from entries, but not keyword-based groups Localization: French: Translation of a new string (#3212) Fix null return Changed from Path to Optional<Path> Fix #2775: Hyphens in last names are properly parsed (#3209) Followup to Issue #3167 (#3202) Remove unused import in GroupTreeNode Move getEntriesInGroup to GroupTreeNode ...

Add file description to gui

55e3d1d

Add tests for identifying file descr bug

Siedlerchr added 2 commits September 13, 2017 11:27

Merge remote-tracking branch 'upstream/master' into fileDescr

d985361

* upstream/master: Localization: French: Translation of a new string (#3212) Fix #2775: Hyphens in last names are properly parsed (#3209) Followup to Issue #3167 (#3202) Update mockito-core from 2.9.0 -> 2.10.0

add properties to LinkedFiel

23c6ac6

tobiasdiez reviewed Sep 13, 2017

View reviewed changes

Fix observable

52b5b42

Fix serializing

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 13, 2017

Siedlerchr added this to the v4.0 milestone Sep 13, 2017

add tooltip again

3197713

add changelog

Siedlerchr changed the title ~~[WIP] Add file description to gui and fix sync bug~~ Add file description to gui and fix sync bug Sep 13, 2017

tobiasdiez requested changes Sep 13, 2017

View reviewed changes

Siedlerchr added 2 commits September 15, 2017 11:03

add automatically found to observables

f767625

Show description first then file link remove unnecesary separator

This was referenced Sep 15, 2017

DEL key does not work in file attachment box JabRef/jabref-koppor#262

Closed

snapshot: file attachments view wrong #3222

Closed

Siedlerchr added the status: devcall label Sep 18, 2017

koppor removed the status: devcall label Sep 18, 2017

lenhard approved these changes Sep 18, 2017

View reviewed changes

Siedlerchr added 2 commits September 19, 2017 14:13

Remove some empty lines

4cf1a6d

LinusDietz approved these changes Sep 19, 2017

View reviewed changes

Siedlerchr merged commit c47d2a4 into master Sep 20, 2017

Siedlerchr deleted the fileDescr branch September 20, 2017 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file description to gui and fix sync bug #3210

Add file description to gui and fix sync bug #3210

Siedlerchr commented Sep 11, 2017 •

edited

Loading

tobiasdiez commented Sep 12, 2017

Siedlerchr commented Sep 13, 2017 •

edited

Loading

tobiasdiez Sep 13, 2017

Siedlerchr commented Sep 13, 2017

tobiasdiez left a comment

tobiasdiez Sep 13, 2017

notuntoward Sep 15, 2017 •

edited

Loading

Siedlerchr Sep 15, 2017

Siedlerchr Sep 15, 2017

notuntoward Sep 15, 2017 •

edited

Loading

tobiasdiez Sep 13, 2017

Siedlerchr Sep 15, 2017

lenhard left a comment

lenhard Sep 18, 2017

lenhard Sep 18, 2017

lenhard Sep 18, 2017

tobiasdiez commented Sep 20, 2017

Siedlerchr commented Sep 20, 2017

Add file description to gui and fix sync bug #3210

Add file description to gui and fix sync bug #3210

Conversation

Siedlerchr commented Sep 11, 2017 • edited Loading

tobiasdiez commented Sep 12, 2017

Siedlerchr commented Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Siedlerchr commented Sep 13, 2017

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notuntoward Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notuntoward Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard 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

tobiasdiez commented Sep 20, 2017

Siedlerchr commented Sep 20, 2017

Siedlerchr commented Sep 11, 2017 •

edited

Loading

Siedlerchr commented Sep 13, 2017 •

edited

Loading

notuntoward Sep 15, 2017 •

edited

Loading

notuntoward Sep 15, 2017 •

edited

Loading