Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
import org.jabref.model.entry.LinkedFile;
import org.jabref.model.strings.StringUtil;
import org.jabref.model.util.FileHelper;
import org.jabref.model.util.OptionalUtil;
import org.jabref.preferences.FilePreferences;
import org.jabref.preferences.PreferencesService;

import de.saxsys.mvvmfx.utils.validation.FunctionBasedValidator;
Expand Down Expand Up @@ -332,9 +332,29 @@ public boolean isGeneratedNameSameAsOriginal() {
* @return true if suggested filepath is same as existing filepath.
*/
public boolean isGeneratedPathSameAsOriginal() {
Optional<Path> newDir = databaseContext.getFirstExistingFileDir(preferences.getFilePreferences());
FilePreferences filePreferences = preferences.getFilePreferences();
Optional<Path> newDir = databaseContext.getFirstExistingFileDir(filePreferences);
if (newDir.isEmpty()) {
// could not find default path
return false;
}
Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 Nov 24, 2021

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).

Copy link
Contributor Author

@ruoyu-qian ruoyu-qian Nov 26, 2021

Choose a reason for hiding this comment

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

Hi @k3KAW8Pnf7mkmdSMPHz27!

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!

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 Nov 29, 2021

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

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 :/

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 Nov 29, 2021

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

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)?

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

Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 Nov 29, 2021

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

Copy link
Contributor Author

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. :)


// append File directory pattern if exits
String targetDirectoryName = "";
if (!filePreferences.getFileDirectoryPattern().isEmpty()) {
targetDirectoryName = FileUtil.createDirNameFromPattern(
databaseContext.getDatabase(),
entry,
filePreferences.getFileDirectoryPattern());
}
Path targetDir = newDir.get().resolve(targetDirectoryName);

Optional<Path> currentDir = linkedFile.findIn(databaseContext, preferences.getFilePreferences()).map(Path::getParent);
if (currentDir.isEmpty()) {
// Could not find file
return false;
}
Path oldDir = currentDir.get();

BiPredicate<Path, Path> equality = (fileA, fileB) -> {
try {
Expand All @@ -343,7 +363,7 @@ public boolean isGeneratedPathSameAsOriginal() {
return false;
}
};
return OptionalUtil.equals(newDir, currentDir, equality);
return FileHelper.equals(targetDir, oldDir, equality);
}

public void moveToDefaultDirectoryAndRename() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,12 +294,12 @@ public ContextAction(StandardActions command, LinkedFileViewModel linkedFile, Pr

this.executable.bind(
switch (command) {
case RENAME_FILE_TO_PATTERN, MOVE_FILE_TO_FOLDER_AND_RENAME -> Bindings.createBooleanBinding(
case RENAME_FILE_TO_PATTERN -> Bindings.createBooleanBinding(
() -> !linkedFile.getFile().isOnlineLink()
&& linkedFile.getFile().findIn(databaseContext, preferencesService.getFilePreferences()).isPresent()
&& !linkedFile.isGeneratedNameSameAsOriginal(),
linkedFile.getFile().linkProperty(), bibEntry.getValue().map(BibEntry::getFieldsObservable).orElse(null));
case MOVE_FILE_TO_FOLDER -> Bindings.createBooleanBinding(
case MOVE_FILE_TO_FOLDER, MOVE_FILE_TO_FOLDER_AND_RENAME -> Bindings.createBooleanBinding(
() -> !linkedFile.getFile().isOnlineLink()
&& linkedFile.getFile().findIn(databaseContext, preferencesService.getFilePreferences()).isPresent()
&& !linkedFile.isGeneratedPathSameAsOriginal(),
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/org/jabref/model/util/FileHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import java.util.Locale;
import java.util.Objects;
import java.util.Optional;
import java.util.function.BiPredicate;

import org.jabref.model.database.BibDatabaseContext;
import org.jabref.preferences.FilePreferences;
Expand All @@ -25,6 +26,18 @@

public class FileHelper {

/**
* Returns if the two paths are identical.
*
* @param left one of the two paths for comparison
* @param right the second path for comparison
* @param equality a predicate of two paths arguments
* @return true if the two paths are identical
*/
public static <T, U> boolean equals(Path left, Path right, BiPredicate<Path, Path> equality) {
return equality.test(left, right);
}

/**
* Returns the extension of a file or Optional.empty() if the file does not have one (no . in name).
*
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.gui.fieldeditors;

import java.io.IOException;
import java.net.CookieHandler;
import java.net.CookieManager;
import java.net.CookiePolicy;
Expand All @@ -22,6 +23,7 @@
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.CurrentThreadTaskExecutor;
import org.jabref.gui.util.TaskExecutor;
import org.jabref.logic.externalfiles.LinkedFileHandler;
import org.jabref.logic.net.URLDownload;
import org.jabref.logic.xmp.XmpPreferences;
import org.jabref.model.database.BibDatabaseContext;
Expand Down Expand Up @@ -255,6 +257,7 @@ void isNotSamePath() {
linkedFile = new LinkedFile("desc", tempFile, "pdf");
databaseContext = mock(BibDatabaseContext.class);
when(filePreferences.getFileNamePattern()).thenReturn("[citationkey]");
when(filePreferences.getFileDirectoryPattern()).thenReturn("");
when(databaseContext.getFirstExistingFileDir(filePreferences)).thenReturn(Optional.of(Path.of("/home")));

LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService, preferences, externalFileType);
Expand All @@ -266,12 +269,42 @@ void isSamePath() {
linkedFile = new LinkedFile("desc", tempFile, "pdf");
databaseContext = mock(BibDatabaseContext.class);
when(filePreferences.getFileNamePattern()).thenReturn("[citationkey]");
when(filePreferences.getFileDirectoryPattern()).thenReturn("");
when(databaseContext.getFirstExistingFileDir(filePreferences)).thenReturn(Optional.of(tempFile.getParent()));

LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService, preferences, externalFileType);
assertTrue(viewModel.isGeneratedPathSameAsOriginal());
}

// Tests if isGeneratedPathSameAsOriginal takes into consideration File directory pattern
@Test
void isNotSamePathWithPattern() {
linkedFile = new LinkedFile("desc", tempFile, "pdf");
System.out.println(tempFile.getParent());
databaseContext = mock(BibDatabaseContext.class);
when(filePreferences.getFileNamePattern()).thenReturn("[citationkey]");
when(filePreferences.getFileDirectoryPattern()).thenReturn("[entrytype]");
when(databaseContext.getFirstExistingFileDir(filePreferences)).thenReturn(Optional.of(tempFile.getParent()));

LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService, preferences, externalFileType);
assertFalse(viewModel.isGeneratedPathSameAsOriginal());
}

// Tests if isGeneratedPathSameAsOriginal takes into consideration File directory pattern
@Test
void isSamePathWithPattern() throws IOException {
linkedFile = new LinkedFile("desc", tempFile, "pdf");
databaseContext = mock(BibDatabaseContext.class);
when(filePreferences.getFileNamePattern()).thenReturn("[citationkey]");
when(filePreferences.getFileDirectoryPattern()).thenReturn("[entrytype]");
when(databaseContext.getFirstExistingFileDir(filePreferences)).thenReturn(Optional.of(tempFile.getParent()));

LinkedFileHandler fileHandler = new LinkedFileHandler(linkedFile, entry, databaseContext, filePreferences);
fileHandler.moveToDefaultDirectory();

LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService, preferences, externalFileType);
assertTrue(viewModel.isGeneratedPathSameAsOriginal());
}

// Tests if added parameters to mimeType gets parsed to correct format.
@Test
Expand Down
25 changes: 25 additions & 0 deletions src/test/java/org/jabref/logic/util/io/FileUtilTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.function.BiPredicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -383,4 +384,28 @@ void testIsNotBibFile() throws IOException {
Path bibFile = Files.createFile(rootDir.resolve("test.pdf"));
assertFalse(FileUtil.isBibFile(bibFile));
}

@Test
void testFilePathNotEqual() {
BiPredicate<Path, Path> equality = (fileA, fileB) -> {
try {
return Files.isSameFile(fileA, fileB);
} catch (IOException e) {
return false;
}
};
assertFalse(FileHelper.equals(existingTestFile, otherExistingTestFile, equality));
}

@Test
void testFilePathEqual() {
BiPredicate<Path, Path> equality = (fileA, fileB) -> {
try {
return Files.isSameFile(fileA, fileB);
} catch (IOException e) {
return false;
}
};
assertTrue(FileHelper.equals(existingTestFile, existingTestFile, equality));
}
}