Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix action helper #6122

Merged
merged 7 commits into from
May 3, 2020
Merged

Fix action helper #6122

merged 7 commits into from
May 3, 2020

Conversation

stefan-kolb
Copy link
Member

@stefan-kolb stefan-kolb commented Mar 14, 2020 •
edited by tobiasdiez
Loading

  • only reason here that I can think of is that the selection (maybe only on Linux) makes problems and returns an empty List

The only reason I can think of might be caused by the following code snippet:

    public static BooleanExpression isAnyFieldSetForSelectedEntry(List<Field> fields, StateManager stateManager) {
        BibEntry entry = stateManager.getSelectedEntries().get(0);
        return Bindings.createBooleanBinding(
                () -> entry.getFields().stream().anyMatch(fields::contains),
                entry.getFieldsObservable(),
                stateManager.getSelectedEntries());
    }

BibEntry entry = stateManager.getSelectedEntries().get(0);

Also I do not know how to trigger this method without an entry selected it might still be an empty list.
At least this is what the error log says. So we can only get the entry when it exists.

Fixes #6085.

  • Found another bug:
    How to reproduce:
  1. select two entries (one with an attached file one without) First needs to be the one with a file!
  2. select open folder in context menu
  3. exception occurs for selected entry without attached file

@stefan-kolb
fix: not every selected entry will have a file
383391d 
Related #6085
@stefan-kolb
fix: this will probably fix missing selection information on Linux
e6e27a9 
I is hard to test this, as normally a selection should be given when the method is triggered.
Fixes #6085
@stefan-kolb @tobiasdiez
Update src/main/java/org/jabref/gui/actions/ActionHelper.java
1bd2770 
Co-Authored-By: Tobias Diez <tobiasdiez@gmx.de>
@stefan-kolb
Copy link
Member Author

Thanks @tobiasdiez , that looks good!

@calixtus
Copy link
Member

Does this also cover the case that an entry stays selected while a field is emptied in the entry editor?

calixtus
calixtus reviewed Mar 14, 2020
View reviewed changes
src/main/java/org/jabref/gui/actions/ActionHelper.java Outdated
} else {
return entry.get().getFields().stream().anyMatch(fields::contains);
}
}, selectedEntries);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing entry.getFieldsObserable() here. Is this intended?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. This is a problem - it might not be available (that's why I need the complex code above). I'm not sure what side effects this might have, I need you guys' expertise here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure but maybe the following works:

EasyBind.monadic(entry)
             .flatMap(BibEntry:getFieldsObservable)
             .map(entryFields -> entryFields.anyMatch(...))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this situation occur? I.e. the fields of the entry change dynamically?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This binding should update in the case when the selected entry changes as well as when the fields of the currently selected entry change

@stefan-kolb
Copy link
Member Author

stefan-kolb commented Mar 14, 2020 •
edited
Loading

Does this also cover the case that an entry stays selected while a field is emptied in the entry editor?

We need to test this. Probably not right now as the binding to getFields() is missing like you already mentioned.

@JabRef JabRef deleted a comment from codecov bot Mar 17, 2020
@JabRef JabRef deleted a comment from codecov bot Mar 29, 2020
@tobiasdiez
Copy link
Member

What's the status here?

@stefan-kolb
Copy link
Member Author

I have no correct solution for both cases as discussed above. however, I'm not sure if the field change? can actually occur.

@tobiasdiez tobiasdiez self-assigned this Apr 14, 2020
@tobiasdiez
Copy link
Member

Maybe with

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

Line 39 in fb2a6d7

private final ObservableList<BibEntry> selectedEntries = FXCollections.observableArrayList();
extractor.

@tobiasdiez tobiasdiez marked this pull request as draft April 17, 2020 15:59
@calixtus calixtus mentioned this pull request Apr 22, 2020
Closed
@Siedlerchr Siedlerchr mentioned this pull request May 1, 2020
Closed
@Siedlerchr
Copy link
Member

I can reproduce the error always if I close all databases and then try to click on the View Menu. Also on Windows

@tobiasdiez
Copy link
Member

I found a way to fix the binding. It should work now.

@tobiasdiez tobiasdiez marked this pull request as ready for review May 3, 2020 11:41
@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 3, 2020
Siedlerchr
Siedlerchr approved these changes May 3, 2020
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also work for the view menu with no database open?

@Siedlerchr Siedlerchr merged commit b472942 into master May 3, 2020
@Siedlerchr Siedlerchr deleted the fix-action-helper branch May 3, 2020 13:20
Siedlerchr added a commit that referenced this pull request Jul 1, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into allowReordering
dcbe938 
* upstream/master:
  Fix action helper (#6122)
koppor pushed a commit that referenced this pull request Oct 1, 2022
Squashed 'buildres/csl/csl-styles/' changes from 201e022..7bde3e4
7486037 
7bde3e4 Add style for the Geographical Analysis journal (#6145)
6fa1551 Create taylor-and-francis-chicago-b-author-date.csl (#6232)
eba2e8c Create taylor-and-francis-ama.csl (#6221)
dda9d57 ACS, AMA, Vancouver: Remove hardcoded space after `citation-number` (#6248)
8f5fe92 GitHub Workflows security hardening (#6246)
284bc10 Create angiology.csl (#6122)
eb141cc Update society-of-biblical-literature-fullnote-bibliography.csl (#6157)
dddb459 Rewrite law-citation-manual.csl (#6171) (#6171)
b975c96 Update Cell to numeric-superscript style (#6245)
3a41b08 Create isara-iso-690.csl (#6201)
5a128fe Create Biomembranes.csl (#6200)
da2e0c0 Capitalize-first short titles for legislation ("Statute"). (#6241)
af7f08d Create proceedings-of-the-estonian-academy-of-sciences-author-date.csl (#6209)

git-subtree-dir: buildres/csl/csl-styles
git-subtree-split: 7bde3e4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calixtus calixtus calixtus left review comments

@Siedlerchr Siedlerchr Siedlerchr approved these changes

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Assignees

@tobiasdiez tobiasdiez

Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exception when trying to use context menu
5 participants
@stefan-kolb @calixtus @tobiasdiez @Siedlerchr @koppor