-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
Consider directory pattern when checking if a file can be moved #8244
Conversation
Disable MOVE_FILE_TO_FOLDER_AND_RENAME using the same logic as MOVE_FILE_TO_FOLDER
Added Javadoc comment
Adds test to test if isGeneratedPathSameAsOriginal takes into consideration File directory pattern
There was one unit test failed I looked into it. According to https://www.ebook.de/de/product/28983211/joshua_bloch_effective_java.html, it seems like the actual should be '2018-01-31' rather than '2018-01-01' which was set by the test. |
Adds unit tests for FileHelper.equals
HI, thanks for the PR, if you merge in the latest main branch from jabref/jabref the Unit test should be fixed. |
Thanks for the response. I have updated my code according to the checkstyle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR, codewise lgtm. And also thanks for adding a test case.
Will try this out tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @ruoyu-qian!
Thank you for looking into the issue!
I agree that code-wise it looks good and a thank you for the added test cases and JavaDoc!
There are only two required changes, anything that starts with nitpick I'd consider an improvement but not necessary (i.e., I'll approve the PR even if you don't address all/any of them)
src/test/java/org/jabref/gui/fieldeditors/LinkedFileViewModelTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
Outdated
Show resolved
Hide resolved
if (newDir.isEmpty()) { | ||
// could not find default path | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the behavior of the method. I am aware that you cannot reach this point in the code if linkedFile.findIn(databaseContext, preferences.getFilePreferences())
is empty, but if someone chooses to use this method for a different purpose in the future, I'd still want it to return true
when both
linkedFile.findIn(databaseContext, preferences.getFilePreferences())
and
databaseContext.getFirstExistingFileDir(filePreferences)
are empty (.isEmpty() == true
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing my PR. Could you elaborate on this request a bit more? You mentioned two directories are empty. Other than linkedFile.findIn(databaseContext, preferences.getFilePreferences())
, what is the other one?
I'm also trying to understand the logic here. For the part you quoted, it means if the user did not set default path in preference, this function would return false
, which means the two options would be disabled. Similarly, in line 351-354, if(currentDir.isEmpty())
, the function is also going to return false
, under the logic that in this case, we are not able to find the current directory of the file. Are you expecting different behaviors for these two cases?
Thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you expecting different behaviors for these two cases?
Oups, sorry about both the delayed response and the typo in my comment, I've updated it.
The original comment is regarding changing the behavior of the method when neither is present (no default path and no linked file)
previously the only value returned from the method was this,
return OptionalUtil.equals(newDir, currentDir, equality); |
and if both
linkedFile.findIn(databaseContext, preferences.getFilePreferences())
databaseContext.getFirstExistingFileDir(preferences.getFilePreferences()
are empty we return
jabref/src/main/java/org/jabref/model/util/OptionalUtil.java
Lines 25 to 26 in 69467d3
if (!left.isPresent()) { | |
return !right.isPresent(); |
true
.
which means the two options would be disabled
wouldn't this mean that they are enabled since it gets negated later on?
&& !linkedFile.isGeneratedPathSameAsOriginal(), |
P.S, that would be an awesome test case 😃
Huge thanks for addressing all the nitpicks and yet again, sorry about the delayed response. I'll try to be more responsive if you have follow up questions about this :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized I dodged your question a bit, the short(ish) answer to
I'm also trying to understand the logic here.
is that I believe the separation of concern can be improved upon in the original method
public boolean isGeneratedPathSameAsOriginal() { |
In my opinion, the "correct" method signature should be public Optional<Boolean> isGeneratedPathSameAsOriginal() {
because the two cases you bring up don't match very well with a true
or false
value to the question "is the generated path the same as the original path" (since there doesn't exist a generated path or an original path). That is part of what I hoped to bring up in the follow-up issue 😛
In my opinion the check for
if the user did not set default path in preference
and
Similarly, in line 351-354,
if(currentDir.isEmpty())
provides improved separation of concern if they are done in
jabref/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java
Lines 302 to 305 in 69467d3
case MOVE_FILE_TO_FOLDER -> Bindings.createBooleanBinding( | |
() -> !linkedFile.getFile().isOnlineLink() | |
&& linkedFile.getFile().findIn(databaseContext, preferencesService.getFilePreferences()).isPresent() | |
&& !linkedFile.isGeneratedPathSameAsOriginal(), |
So that those lines read
- is it an online link?
- is there a linked file?
- can we generate a path?
- is the generated path the same as the current path (or does this question not make sense, in which case we are getting an
Optional.empty
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I am writing this I realize that this is probably quite a bit into the nitpicking territory.
Imo the main thing a PR should do is improve on the code, which your PR does, so view this as a nitpick, would you like to address it otherwise we merge this PR as-is as soon as you get back to us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing the nitpick could either be to change the method signature or to match the true/false behavior of the unmodified method (in which case changing the method signature will be part of the follow-up issue when I have time to write it)
Imo you would match the behavior of the unmodified method if you remove the two if statements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @k3KAW8Pnf7mkmdSMPHz27, I now apologize for my delayed response. Thanks a lot for the detailed explanation. I realized I confused myself a bit earlier.
For the part you quoted, it means if the user did not set default path in preference, this function would return false, which means the two options would be disabled.
This would actually be the opposite to what I previously said. Simply with isGeneratedPathSameAsOriginal()
equal to false
would mean the two options being enabled rather than disabled.
I'm now getting what you would like to address here and I agree true/false
is not a perfect answer to these two cases.
My apology for not responding earlier, but since the PR has been merged, I would be happy to take a look at the follow-up issue when I have time. :)
src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
Outdated
Show resolved
Hide resolved
Oups, I forgot, could you update the
Or some text that you'd consider to be more descriptive |
removed newly added function, which is no longer needed
removed newly added tests for the function no longer exist
Thanks! I have added this fix into the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise looks good to me. If @k3KAW8Pnf7mkmdSMPHz27 approves, then we'll merge.
Thank you for your interest in JabRef. Feel free to send more PRs. :-D
* upstream/main: Consider directory pattern when checking if a file can be moved (#8244) Bump byte-buddy-parent from 1.12.1 to 1.12.2 (#8285) Bump unirest-java from 3.13.3 to 3.13.4 (#8283) Bump checkstyle from 9.1 to 9.2 (#8284) Bump classgraph from 4.8.135 to 4.8.137 (#8282) Do not resize main table columns in search dialog window (#8253) Fix NegativeArraySizeException (#8270) Update deployment.yml TEst Grand unified library properties (GRULPS) (#8264) Fixes RFC fetcher test case (#8271) Update Gradle Wrapper from 7.2 to 7.3 (#8246) Observable Preferences H (WorkingDir, getUser, NameFormatter, Version, SpecialFields) (#8260) Bump checkstyle from 9.0.1 to 9.1 (#8266) Bump mockito-core from 4.0.0 to 4.1.0 (#8267) Bump classgraph from 4.8.130 to 4.8.135 (#8268) Fix some fetcher tests (#8258) Clicking a DOI link in the preview pane no longer crashes (#8255)
* upstream/main: (259 commits) Fix exception on preview style edit and selection (#8293) Fix icon picker excpetion in groups dialog (#8290) Add doc tests (#8242) fix merge conflict Squashed 'buildres/csl/csl-locales/' changes from 0cc3885f61..d5ee85de8e Squashed 'buildres/csl/csl-styles/' changes from 3a6a0a7..3bb4b5f Consider directory pattern when checking if a file can be moved (#8244) Bump byte-buddy-parent from 1.12.1 to 1.12.2 (#8285) Bump unirest-java from 3.13.3 to 3.13.4 (#8283) Bump checkstyle from 9.1 to 9.2 (#8284) Bump classgraph from 4.8.135 to 4.8.137 (#8282) Do not resize main table columns in search dialog window (#8253) Fix NegativeArraySizeException (#8270) Update deployment.yml TEst Grand unified library properties (GRULPS) (#8264) Fixes RFC fetcher test case (#8271) Update Gradle Wrapper from 7.2 to 7.3 (#8246) Observable Preferences H (WorkingDir, getUser, NameFormatter, Version, SpecialFields) (#8260) Bump checkstyle from 9.0.1 to 9.1 (#8266) ...
Fixes #7908
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)Settings:
Local files:
When a linked file is in the main file directory, the option “move file to file directory” and “move file to file directory and rename file” are now enabled if users right click on the linked file:
If the linked file is already in the correct file directory pattern, both options are disabled: