Fix for Issue #4306 - Disable Rename when Filename is the same

[ghost]

Potential fix for #4306.
The method performNameConflictCheck added in LinkedFileViewModel helps check whether or not the filename is already equal to the suggested filename. If so, it returns true, otherwise, it returns false. This method is then called in LinkedFilesEditor during createContextMenuForFile() to decide whether or not the Rename, Move, or Rename and Move menu options are to be disabled.

The method itself works as I tested it by running the code live with the JVM. Having the filename == suggested filename indeed disabled those three menu options in the context menu. The issue however, is that I'm having trouble writing my test case for it. I'm running into a couple of nullpointers being thrown when I try to run my test case checkNameConflict() in LinkedFileViewModelTest. Specifically, it reports that there are nullpointers when accessing the getSuggestedFilename() called in within checkNameConflict in the LinkedFileViewModel class.

---

• Change in CHANGELOG.md described - N/A
• Tests created for changes - Work In Progress
• Manually tested changed features in running JabRef
• Screenshots added in PR description (for bigger UI changes) N/A
• Ensured that [the git commit message is a good one] (https://github.com/joelparkerhenderson/git_commit_message)
• Check documentation status (Issue created for outdated help page at help.jabref.org?)

[Siedlerchr]

If you can't get the test working it is not a problem. In such cases it is sufficient if you have manually tested the things. e.g. without a file with two files etc.

[ghost]

@Siedlerchr Awesome! I will amend my Pull Request with a new commit that will remove the test case code later tonight and also fix what was flagged by the Codacy PR Check!

[ghost]

@tobiasdiez @Siedlerchr Is there something I should do to pass the codecov/project check? I'm not sure I understand the whole overall project coverage concept.

[Siedlerchr]

@Arbalest007 The codacy are usually helpful, it outlines some potential issues. However, it also produces false positives (e.g. unused methods for @FXML annotated methods or junit parameter methdos)
The codecoverage is an indicator how much of the codebase is covered by tests. However, that is not that important as we usually focus on testing the most important functions (e.g. the logic/model classes).

Most of the GUI code is manually tested and that is mostly sufficient. While the ViewModel functionality can be tested sometimes it often requires substantial amount of mocking around with objects which often adds a new level of complexity

[tobiasdiez]

Thanks for your PR! The check needs to be adapted for the "move files" option but apart from that the code looks good to me!

[tobiasdiez]

For the rename file functionality, the check is ok. However, for move file and rename and move file the check needs o be adapted: We only want to disable the "move file" option if the file is already in the right place (i.e. the moveToDefaultDirectory method would do nothing). In other words, this check should not compare the file name but the directory with the suggested directory.
This was probably my fault for not being clear enough in the issue description.

[tobiasdiez]

this overwrites the previous disable state coming from the line before. Suggestion:
renameFile.setDisable(linkedFile.getFile().isOnlineLink() || linkedFile.performNameConflictCheck());

[tobiasdiez]

please leave the imports as they were before (we have the convention not to use the * import matcher). You can also import our code style into IntelliJ: https://github.com/JabRef/jabref/tree/master/config

[ghost]

@tobiasdiez As requested I added a new method performDirectoryConflictCheck for comparing the filepaths and merged the CreateContextMenu conditionals into single OR statements. The method worked as intended from my testing!

[Siedlerchr]

Looks Good to me!
Thanks for your contribution!

[tobiasdiez]

Sorry that it took so long for me to have a look at the code. It looks good now, except one minor point where sometimes a NPE would have been thrown (see below for details).

[tobiasdiez]

This kind of code documentation is somewhat contrary to the usual style used in the rest of the code. Either move these comments before the method as JavaDoc comments, or use normal inline comments as done below.

[tobiasdiez]

The get method throws an exception if the optional is empty. I would propose to rewrite the code as return OptionalUtil.equals(newDir, currentDirectory, Files::isSameFile); and add the following method to OptionalUtil

public <T,U> boolean equals(Optional<T> left, Optional<U> right, BiPredicate<T,U> equality) {
   if(!left.isPresent()) {
       return !right.isPresent();
   } else {
       if (right.isPresent()) {
           return equality.test(left.get(), right.get());
      } else {
           return false;
      }
   }
}

(there might be syntax errors in the above code)

[Siedlerchr]

Happy new year to you, It would be really nice if you could address the few issues raised here and finish the PR so we can finally merge it.

[Siedlerchr]

Superseeded by #4727