Removed BIB file directory from search when preferences has option unchecked #6451

silverhorse · 2020-05-08T22:12:53Z

Fixes #5891
For "Automatically set file links" feature, the BIB file directory is always added for search, even though the checkbox is not clicked. If the checkbox is clicked, it is added at first position in list of directories to search, which is expected behaviour. However, it is still added to list of directories to search even when checkbox is left unselected, which is faulty. I fixed this by removing code which adds the directory when preference is set to false.

Also, changed the text of the file preferences in order to make it clear that files will be searched only when the checkbox "Search for files relative to BIB file location" is selected. Also changing user documentation to reflect this text change - JabRef/user-documentation#283

Before:

After:

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 UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

koppor · 2020-05-08T22:29:33Z

Note to reviewers: We should really carefully think about that. Refs #459 (and linked PRs from there).

silverhorse · 2020-05-08T22:38:04Z

Note to reviewers: We should really carefully think about that. Refs #459 (and linked PRs from there).

Yes, I feel there is something amiss. Because when there is no directory selected in meta-data, user provided dir or BIB default directory, then the search directory list will be empty!
May be we should provide another option where the primary dir is removed only explicitly by user.

koppor · 2020-05-08T23:21:51Z

We certainly need to check the documentation. Does it cover all cases discussed in the linked PR? Is the documetnation enough to serve as specification?

JabRef has very powerful diretory features and we should be careful when touching that code.

silverhorse · 2020-05-08T23:49:21Z

We certainly need to check the documentation. Does it cover all cases discussed in the linked PR? Is the documetnation enough to serve as specification?

JabRef has very powerful diretory features and we should be careful when touching that code.

No, I have to look at the linked PR and check for all cases. I will get to it in a bit. Certainly, its not taken lightly. Will check for all cases. Its a WIP.

Siedlerchr · 2020-05-09T18:10:26Z

So, if all three directories are empty (main file dir and the two in library properties and "Use bib file location as primary dir is checked" ) then it would return now an empty collection.
Previously it always returned the bib file location as last resort.

And when I check the checkbox "Use bib file location" the bib file location is returned as first location.

@silverhorse It would be nice if you add a test case for the Move Files Cleanup formatter which tests with all directories empty and the checkbox not checked. It should probably return false or does nothing

silverhorse · 2020-05-09T19:07:30Z

@Siedlerchr yes will add a testcase for all dirs empty. Thanks!

silverhorse · 2020-05-10T00:41:35Z

Tested with all combinations of the four options -

Library properties - general dir
Library properties - user specified dir
Preferences - main file directory
Preferences - BIB file dir

silverhorse · 2020-05-11T21:20:14Z

@koppor and @Siedlerchr plz review when you can. I have added some tests and done manual testing. Plz let me know if there are other scenarios I should test apart from one mentioned in msgs.

Siedlerchr · 2020-05-12T09:18:21Z

src/test/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtilTest.java

        // Due to mocking the externalFileType class, the file extension will not be found

+        when(databaseContext.getFileDirectoriesAsPaths(any())).thenReturn(Collections.singletonList(path.getParent()));


Why did you move the mockings here?

This mocking of getFileDirectoriesAsPaths method returns non empty values, which is not applicable for the newly added test case testFindAssociatedNotLinkedFilesForEmptySearchDir. The new test case expects that getFileDirectoriesAsPaths would return empty list.

Okay, but the externalFileType should be independent of this

yeah, agreed. I will move that one back to setup.

…abref into fix-for-issue-5891

Siedlerchr

For me it's fine

silverhorse · 2020-05-13T02:18:28Z

Thanks for guidance and approval @Siedlerchr . Do you think more reviewers are needed for this change or is it good to merge?

calixtus · 2020-05-13T18:20:56Z

Thanks for guidance and approval @Siedlerchr . Do you think more reviewers are needed for this change or is it good to merge?

We have a strict rule in JabRef that each PR must be reviewed by at least two core developers before it can be merged with the master branch. Be patient. JabRef will still be there tomorrow and many other issues that need to be fixed. 😉

tobiasdiez

Thanks for the fix. The changes look good to me.

Could you please change the text in the UI to clarify what this option does. "Use bib as primary" can be read differently depending on where to put the emphasize: on "use" (so either use it or not) or on "as primary" (so always use it, but not necessarily as first option). I think this was actually the origin of the whole misunderstanding.
Maybe something like "Search for files relative to library location".

I think the user documentation needs to be changed. Would be nice if you could see to this as well.

silverhorse · 2020-05-21T16:00:20Z

@tobiasdiez yes, I like your suggestion. It will remove any confusion regarding this option. Will make this change.

abadar · 2020-05-28T03:54:55Z

@tobiasdiez I am working further to help @silverhorse, here is my change:
silverhorse#1

I have question regarding localization, is there any step on how to get the translated text? or it should be automatic? I saw its recommended to use Crowdin but didn't find specific information on how the strings will be translated. can you please help out

tobiasdiez · 2020-05-28T11:36:10Z

As a developer you don't have to worry about the actual translation. Simply use Localization.lang(...) to get the localizated text and add an entry to JabRef_en. After the PR is merged, the text automatically appears on Crowdin and people can translate it there (which is then synced back to JabRef).
Some information can be found at https://devdocs.jabref.org/getting-into-the-code/code-howtos#using-localization-correctly, but this is a bit out-dated. It would be nice if you could update this documentation.

…r-issue-5891

abadar · 2020-05-28T14:54:29Z

updated user documentation: JabRef/user-documentation#283

Siedlerchr

In general looks good to me, I would adjust the wording a bit.
The file dir is not only used for searching but also for storing downloaded files (e.g. fulltext from fetcher downloads)

Siedlerchr · 2020-05-28T16:06:12Z

src/main/java/org/jabref/gui/preferences/FileTab.fxml

@@ -43,7 +43,7 @@
        <TextField fx:id="mainFileDir" HBox.hgrow="ALWAYS"/>
        <Button onAction="#mainFileDirBrowse" text="%Browse"/>
    </HBox>
-    <CheckBox fx:id="useBibLocationAsPrimary" text="%Use the BIB file location as primary file directory">
+    <CheckBox fx:id="useBibLocationAsPrimary" text="%Search for files relative to BIB file location">


Suggested change

<CheckBox fx:id="useBibLocationAsPrimary" text="%Search for files relative to BIB file location">

<CheckBox fx:id="useBibLocationAsPrimary" text="%Search and store files relative to BIB file location">

and maybe replace "bib file" with "library file" to be consistent with the more recent usage

Siedlerchr · 2020-05-28T16:43:06Z

Thanks for your contribution and the quick follow up! We hope you enjoyed contributing and we are looing forward for more PRs from you 🐱

silverhorse · 2020-05-28T17:25:00Z

Yay! Thanks @Siedlerchr and @tobiasdiez , appreciate your help! We are excited for our first OSS contribution! Thanks @abadar for last mile help. 🤩

ca943b70d7 Fix malformed XML in raptor-journal.csl (#6455) 2fad8d1104 Update knee-surgery-sports-traumatology-arthroscopy.csl (#6441) b4422b77b7 Update guide-des-citations-references-et-abreviations-juridiques.csl (#6450) cfe85da320 Create raptor-journal.csl (#6451) afbb963346 Create arkivoc.csl (#6134) 2f21ceb7b4 correct name disambiguation rules (#6442) 5b2191f38b Update bibliothek-forschung-und-praxis.csl (#6437) aaea3097d1 Update sodertorns-hogskola-oxford.csl (#6439) e4fbc605f8 Update universite-du-quebec-a-montreal.csl (#6438) 3653118f17 Small Fix to Bio-Protocol git-subtree-dir: buildres/csl/csl-styles git-subtree-split: ca943b70d73bd4c57c0b1266ee7c54907b8f9d4f

Partial fix for issue 5891

c8da256

silverhorse changed the title ~~Partial fix for issue 5891~~ Removed BIB file directory from search when preferences has option unchecked May 8, 2020

Giri added 2 commits May 9, 2020 17:02

Added more tests

ce33357

Added more tests

3df1342

silverhorse marked this pull request as ready for review May 10, 2020 00:38

Siedlerchr reviewed May 12, 2020

View reviewed changes

Giri added 2 commits May 12, 2020 09:58

Added more tests

5454a09

Merge branch 'fix-for-issue-5891' of https://github.com/silverhorse/j…

e066958

…abref into fix-for-issue-5891

Siedlerchr approved these changes May 12, 2020

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 15, 2020

tobiasdiez requested changes May 21, 2020

View reviewed changes

tobiasdiez added the status: changes required Pull requests that are not yet complete label May 21, 2020

updating text for checkbox

4b997f8

abadar mentioned this pull request May 28, 2020

updating text for checkbox silverhorse/jabref#1

Merged

5 tasks

mbadar added 2 commits May 28, 2020 09:57

reverting change from other properties files

5668049

Merge branch 'master' of https://github.com/JabRef/jabref into fix-fo…

62a6961

…r-issue-5891

abadar mentioned this pull request May 28, 2020

Updating documentation for file preferences option JabRef/user-documentation#283

Merged

Siedlerchr requested changes May 28, 2020

View reviewed changes

mbadar added 2 commits May 28, 2020 12:24

updating text

0ce7c8f

updating text

6a1940f

Siedlerchr merged commit ffa07cd into JabRef:master May 28, 2020

koppor removed status: changes required Pull requests that are not yet complete status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels May 29, 2020

koppor mentioned this pull request Jun 13, 2022

Replace "Use the bib file location as primary directory" setting JabRef/jabref-koppor#127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed BIB file directory from search when preferences has option unchecked #6451

Removed BIB file directory from search when preferences has option unchecked #6451

silverhorse commented May 8, 2020 •

edited

Loading

koppor commented May 8, 2020

silverhorse commented May 8, 2020

koppor commented May 8, 2020

silverhorse commented May 8, 2020 •

edited

Loading

Siedlerchr commented May 9, 2020

silverhorse commented May 9, 2020

silverhorse commented May 10, 2020

silverhorse commented May 11, 2020

Siedlerchr May 12, 2020

silverhorse May 12, 2020

Siedlerchr May 12, 2020

silverhorse May 12, 2020

Siedlerchr left a comment

silverhorse commented May 13, 2020

calixtus commented May 13, 2020

tobiasdiez left a comment

silverhorse commented May 21, 2020

abadar commented May 28, 2020

tobiasdiez commented May 28, 2020

abadar commented May 28, 2020

Siedlerchr left a comment

Siedlerchr May 28, 2020

tobiasdiez May 28, 2020

abadar May 28, 2020

Siedlerchr commented May 28, 2020

silverhorse commented May 28, 2020

		// Due to mocking the externalFileType class, the file extension will not be found

		when(databaseContext.getFileDirectoriesAsPaths(any())).thenReturn(Collections.singletonList(path.getParent()));

	<CheckBox fx:id="useBibLocationAsPrimary" text="%Search for files relative to BIB file location">
	<CheckBox fx:id="useBibLocationAsPrimary" text="%Search and store files relative to BIB file location">

Removed BIB file directory from search when preferences has option unchecked #6451

Removed BIB file directory from search when preferences has option unchecked #6451

Conversation

silverhorse commented May 8, 2020 • edited Loading

koppor commented May 8, 2020

silverhorse commented May 8, 2020

koppor commented May 8, 2020

silverhorse commented May 8, 2020 • edited Loading

Siedlerchr commented May 9, 2020

silverhorse commented May 9, 2020

silverhorse commented May 10, 2020

silverhorse commented May 11, 2020

Siedlerchr May 12, 2020

Choose a reason for hiding this comment

silverhorse May 12, 2020

Choose a reason for hiding this comment

Siedlerchr May 12, 2020

Choose a reason for hiding this comment

silverhorse May 12, 2020

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

silverhorse commented May 13, 2020

calixtus commented May 13, 2020

tobiasdiez left a comment

Choose a reason for hiding this comment

silverhorse commented May 21, 2020

abadar commented May 28, 2020

tobiasdiez commented May 28, 2020

abadar commented May 28, 2020

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr May 28, 2020

Choose a reason for hiding this comment

tobiasdiez May 28, 2020

Choose a reason for hiding this comment

abadar May 28, 2020

Choose a reason for hiding this comment

Siedlerchr commented May 28, 2020

silverhorse commented May 28, 2020

silverhorse commented May 8, 2020 •

edited

Loading

silverhorse commented May 8, 2020 •

edited

Loading