ActionHelper to test for present file #6151

calixtus · 2020-03-21T19:30:04Z

It took a little bit more refactoring than expected, for it does not only check, if StandardField.FILE has text inside, but if the file is also really present in the file system. There are some design decisions i'm unsure of:
There is no alert about that no file is really present. The menu entry is just disabled, although there is a file symbol in the main table (since StandardField.FILE is not empty).
Feedback could be implemented in the files editor in the entry editor as validation.
The other thing is that in theory at last, the table does not yet automatically update, if a file is added or removed (so no file is attached). This would require probably an array of observables to be used as an invalidation alert. I'm unsure where to start with that. However, only in theory. Pracitcally the entry is unselected automatically if I change something about the attached files in the entry editor. So nobody would notice, if this would not be implemented.

Change in CHANGELOG.md described (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for bigger UI changes)
Checked documentation: Is the information available and up to date? If not: Issue created at https://github.com/JabRef/user-documentation/issues.

calixtus · 2020-03-24T16:36:39Z

I added some validation. The regular validation mechanics of controlsfx seem somewhat broken. The icon is shown, but behind every other ui element, so instead I just marked the background of the cells with files that do not exist in the filesystem in the warning color.

Theoretically, this one is mergable.

tobiasdiez

Thanks! Looks good to me in general, but I have a few comments/suggestions.

tobiasdiez · 2020-03-28T14:57:11Z

src/main/java/org/jabref/gui/actions/ActionHelper.java

@@ -36,4 +40,18 @@ public static BooleanExpression isAnyFieldSetForSelectedEntry(List<Field> fields
                entry.getFieldsObservable(),
                stateManager.getSelectedEntries());
    }
+
+    public static BooleanExpression isFilePresentForSelectedEntry(StateManager stateManager, PreferencesService preferencesService) {
+        List<LinkedFile> files = stateManager.getSelectedEntries().get(0).getFiles();


I think the current code runs into similar problems that we had recently in a PR by @stefan-kolb: if the currently selected entry changes, then the binding is not updated.
Here, this should be relatively easy to fix:

EasyBind.flatMap(stateManager.getSelectedEntries().getValueAt(0), entry -> entry.getFiles()) .map(files -> to boolean)

Here the getFiles methods needs to be changed to return an observable list. This can be done using the current code and a EasyBind.map(getFieldBinding(FILE), ... parse).

I played a bit around with it. I was able to make getFiles return an ObservableList (by BindingsHelper::map, I had no luck with EasyBind), but EasyBind.flatMap does not exist, so i worked with map. But now the ActionHelper never works. I really don't know how to proceed, what am I doing wrong?

Sorry, it wasn't in EasyBind but here:

jabref/src/main/java/org/jabref/gui/util/BindingsHelper.java

Line 68 in 093a6fb

public static <T, U> ObservableList<U> map(ObservableValue<T> source, Function<T, List<U>> mapper) {

thus BindingsHelper.map(getFieldBinding(FILES), string -> parseFiles()) should work. If not I'll have a closer look at it tomorrow.

We always think way too complicated.
I just moved the call to getFiles() into the calculation part and added stateManager.getSelectedEntries as an additional dependency, so the list is always recalculated if the currently selected entry changes.
I have tested it and it works.

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

src/main/java/org/jabref/gui/util/ViewModelListCellFactory.java

calixtus

Validation is fixed, but the ActionHelper keeps me awake.... Someone please help?

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

calixtus · 2020-03-28T21:09:11Z

src/main/java/org/jabref/gui/actions/ActionHelper.java

@@ -36,4 +40,18 @@ public static BooleanExpression isAnyFieldSetForSelectedEntry(List<Field> fields
                entry.getFieldsObservable(),
                stateManager.getSelectedEntries());
    }
+
+    public static BooleanExpression isFilePresentForSelectedEntry(StateManager stateManager, PreferencesService preferencesService) {
+        List<LinkedFile> files = stateManager.getSelectedEntries().get(0).getFiles();


I played a bit around with it. I was able to make getFiles return an ObservableList (by BindingsHelper::map, I had no luck with EasyBind), but EasyBind.flatMap does not exist, so i worked with map. But now the ActionHelper never works. I really don't know how to proceed, what am I doing wrong?

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

…s of IntelliJ

# Conflicts: # src/main/java/org/jabref/model/entry/BibEntry.java

…ionCaseInsensitive * upstream/master: (25 commits) ActionHelper to test for present file (#6151) Reduce memory footprint (#6298) Add missing abbreviated journal names (#6292) fix l10n again fix checkstyle fix l10n Try to minimize CodeCov "wrong metrics" Showing correct icon on main table linked files column (#6264) Fix labels for outdated dependency issue Change one more line Squashed 'src/main/resources/csl-styles/' changes from c31d9ca..c1793d2 Resolve unit test from failing Add one more change Fix errors RIS import takes the wrong date and duplicates abstract (#6272) Update EntryTypeView.java Change to the old school format Fix XmpExporterTest (#6289) Add checkstyle screenshot (and lint guidelines-...md) Squashed 'src/main/resources/csl-styles/' changes from db54e56..c31d9ca ...

calixtus added 3 commits March 18, 2020 18:28

Added ActionHelper to test for present file

c6d0812

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

b0ffa0d

Added Validation

d16ded3

JabRef deleted a comment from codecov bot Mar 24, 2020

calixtus marked this pull request as ready for review March 24, 2020 16:36

calixtus changed the title ~~[WIP] ActionHelper to test for present file~~ ActionHelper to test for present file Mar 24, 2020

calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 24, 2020

tobiasdiez requested changes Mar 28, 2020

View reviewed changes

calixtus added 2 commits March 28, 2020 17:28

Refactored Validation

bc21be7

Refactored unsuccessfully ActionHelper

0c2c4c7

calixtus commented Mar 28, 2020

View reviewed changes

JabRef deleted a comment from codecov bot Mar 28, 2020

calixtus changed the title ~~ActionHelper to test for present file~~ [WIP] ActionHelper to test for present file Mar 28, 2020

calixtus added status: help wanted and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Mar 28, 2020

calixtus added 4 commits April 6, 2020 08:37

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

4c4a6dd

Fixed issues about opening an online link and applied some suggestion…

feb8998

…s of IntelliJ

Fixed recalculation of selected entry

0ad24f6

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

cee5bee

calixtus added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: help wanted labels Apr 13, 2020

calixtus changed the title ~~[WIP] ActionHelper to test for present file~~ ActionHelper to test for present file Apr 13, 2020

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

45ae2e6

# Conflicts: # src/main/java/org/jabref/model/entry/BibEntry.java

tobiasdiez approved these changes Apr 17, 2020

View reviewed changes

tobiasdiez merged commit 07cb5b8 into JabRef:master Apr 17, 2020

calixtus deleted the fix_koppor_430 branch April 17, 2020 16:08

calixtus mentioned this pull request Apr 22, 2020

UI consistency - BibTexStringEditorDialog rework #6287

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ActionHelper to test for present file #6151

ActionHelper to test for present file #6151

calixtus commented Mar 21, 2020 •

edited

Loading

calixtus commented Mar 24, 2020

tobiasdiez left a comment

tobiasdiez Mar 28, 2020

calixtus Mar 28, 2020

tobiasdiez Mar 28, 2020

calixtus Apr 13, 2020

calixtus left a comment •

edited

Loading

calixtus Mar 28, 2020

ActionHelper to test for present file #6151

ActionHelper to test for present file #6151

Conversation

calixtus commented Mar 21, 2020 • edited Loading

calixtus commented Mar 24, 2020

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez Mar 28, 2020

Choose a reason for hiding this comment

calixtus Mar 28, 2020

Choose a reason for hiding this comment

tobiasdiez Mar 28, 2020

Choose a reason for hiding this comment

calixtus Apr 13, 2020

Choose a reason for hiding this comment

calixtus left a comment • edited Loading

Choose a reason for hiding this comment

calixtus Mar 28, 2020

Choose a reason for hiding this comment

calixtus commented Mar 21, 2020 •

edited

Loading

calixtus left a comment •

edited

Loading