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.

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

Only disable move to file dir when path equals #7269

Merged
merged 3 commits into from
Dec 30, 2020
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
### Fixed

- We fixed an issue with the style of highlighted check boxes while searching in preferences. [#7226](https://github.com/JabRef/jabref/issues/7226)
- We fixed an issue where the option "Move file to file directory" was disabled in the entry editor for all files [#7194](https://github.com/JabRef/jabref/issues/7194)

### Removed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -317,14 +317,14 @@ public boolean isGeneratedNameSameAsOriginal() {
}

/**
* Compares suggested filepath of current linkedFile with existing filepath.
* Compares suggested directory of current linkedFile with existing filepath directory.
*
* @return true if suggested filepath is same as existing filepath.
*/
public boolean isGeneratedPathSameAsOriginal() {
Optional<Path> newDir = databaseContext.getFirstExistingFileDir(filePreferences);

Optional<Path> currentDir = linkedFile.findIn(databaseContext, filePreferences);
Optional<Path> currentDir = linkedFile.findIn(databaseContext, filePreferences).map(Path::getParent);

BiPredicate<Path, Path> equality = (fileA, fileB) -> {
try {
Expand Down Expand Up @@ -434,7 +434,7 @@ public void download() {
List<LinkedFile> linkedFiles = entry.getFiles();
int oldFileIndex = -1;
int i = 0;
while (i < linkedFiles.size() && oldFileIndex == -1) {
while ((i < linkedFiles.size()) && (oldFileIndex == -1)) {
LinkedFile file = linkedFiles.get(i);
// The file type changes as part of download process (see prepareDownloadTask), thus we only compare by link
if (file.getLink().equalsIgnoreCase(linkedFile.getLink())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ public ContextAction(StandardActions command, LinkedFileViewModel linkedFile, Pr
case MOVE_FILE_TO_FOLDER_AND_RENAME, MOVE_FILE_TO_FOLDER -> Bindings.createBooleanBinding(
() -> !linkedFile.getFile().isOnlineLink()
&& linkedFile.getFile().findIn(databaseContext, preferencesService.getFilePreferences()).isPresent()
&& linkedFile.isGeneratedPathSameAsOriginal(),
&& !linkedFile.isGeneratedPathSameAsOriginal(),
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
linkedFile.getFile().linkProperty());
case DOWNLOAD_FILE -> Bindings.createBooleanBinding(
() -> linkedFile.getFile().isOnlineLink(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.LinkedFile;
import org.jabref.preferences.FilePreferences;
import org.jabref.testutils.category.FetcherTest;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -152,7 +151,6 @@ void deleteWhenDialogCancelledReturnsFalseAndDoesNotRemoveFile() {
assertTrue(Files.exists(tempFile));
}

@FetcherTest
void downloadDoesNotOverwriteFileTypeExtension() throws MalformedURLException {
linkedFile = new LinkedFile(new URL("http://arxiv.org/pdf/1207.0408v1"), "");

Expand All @@ -170,4 +168,28 @@ void downloadDoesNotOverwriteFileTypeExtension() throws MalformedURLException {
task.onFailure(Assertions::fail);
new CurrentThreadTaskExecutor().execute(task);
}

@Test
void isNotSamePath() {
linkedFile = new LinkedFile("desc", tempFile, "pdf");
databaseContext = mock(BibDatabaseContext.class);
when(filePreferences.getFileNamePattern()).thenReturn("[citationkey]"); // use this variant, as we cannot mock the linkedFileHandler cause it's initialized inside the viewModel
Copy link
Member

Choose a reason for hiding this comment

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

If it's hard to write tests, then it's a good indication that some refactoring should be done. Indeed, we use the mvvm pattern to make testing easy. Could you thus please add a constructor in the viewmodel that accepts a linkedFileHandler .

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it's just the comment which is obsolete here, as the linkedFilesHandler is not required and can be tested separately (and already is). Would need the same mocking, so no difference.

when(databaseContext.getFirstExistingFileDir(filePreferences)).thenReturn(Optional.of(Path.of("/home")));

LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService, xmpPreferences, filePreferences, externalFileType);

assertFalse(viewModel.isGeneratedPathSameAsOriginal());
}

@Test
void isSamePath() {
linkedFile = new LinkedFile("desc", tempFile, "pdf");
databaseContext = mock(BibDatabaseContext.class);
when(filePreferences.getFileNamePattern()).thenReturn("[citationkey]"); // use this variant, as we cannot mock the linkedFileHandler cause it's initialized inside the viewModel
when(databaseContext.getFirstExistingFileDir(filePreferences)).thenReturn(Optional.of(tempFile.getParent()));

LinkedFileViewModel viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogService, xmpPreferences, filePreferences, externalFileType);

assertTrue(viewModel.isGeneratedPathSameAsOriginal());
}
}