Implemented issue #3229

[derebaba]

Added an error dialog if the file is open in another process and cannot be renamed. [#3229]. This is my first contribution. Please give me feedback if you see any problems.

[lenhard]

Hi @derebaba

Thanks for your pull request! Your contribution is certainly very welcome and looks good overall.

I still have two points that should be addressed:

• You add two new localization entries here. The way in which you use the localization strings in the code is fine, but you need to add the keys to the language files, otherwise the build will fail. To do so, you need to add the keys manually to the English file jabref/src/main/resources/l10n/JabRef_en.properties and then execute ./gradlew localizationUpdate on the command line, which will synchronize the keys in all files. There's also a pretty good description of that when you run the tests and the localization test fails.
• Apart from that, you are using the older file api java.io for doing the rename check. We try to get rid of that and try to use the new file api java.nio instead. I would expect something like Files.isWritable() to do the trick. Can you please adjust your solution to use the NIO API?

[derebaba]

Hi @lenhard

Thanks for your feedback. Are the keys ordered in any way in properties file? Or should I add new keys at the end?

[Siedlerchr]

@derebaba You should first check if there is already a key with the same/or similar text which you can reuse. Otherwise add it to the end and.

[derebaba]

I synchronized the keys and removed the check which used the old api. However, Files.isWritable() returns true even if the file is open in another process. I searched a lot but I don't think the check is possible with the new api. Therefore, I moved the error dialog code to the catch block in FileUtil.java.

[Siedlerchr]

That is not a good idea, as you are now violating the architecture: https://github.com/JabRef/jabref/wiki/High-Level-Documentation
You are having gui components in a logic method. This FileUtil method is used in many places.
So I suggest you create a method which throws the exception

[tobiasdiez]

I think we need to help @derebaba here a bit (since architectural questions are probably not a good start into a project).

It is also not possible to simply move the dialog service stuff to RenamePdfCleanup since this class also resides in logic. Thus I would propose the following:

1. Extract the body of the FileUtil.renameFile method to a new method FileUtil.renameFileWithException
   

   jabref/src/main/java/org/jabref/logic/util/io/FileUtil.java

   Lines 194 to 199 in c47d2a4

   	if (replaceExisting) {
   	return Files.move(fromFile, fromFile.resolveSibling(toFile),
   	StandardCopyOption.REPLACE_EXISTING) != null;
   	} else {
   	return Files.move(fromFile, fromFile.resolveSibling(toFile)) != null;
   	}

2. Similarly, extract the body of the cleanup method to a new method cleanupWithException (which then uses the new renameFileWithException method)

   jabref/src/main/java/org/jabref/logic/cleanup/RenamePdfCleanup.java

   Line 61 in 8a420b4

   public List<FieldChange> cleanup(BibEntry entry) {

3. Catch the exception and show the error dialog in the gui:
   

   jabref/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java

   Line 206 in c47d2a4

   FileUtil.renameFile(fileConflictCheck.get(), file.get(), true);

It's a bit complicated but the best I can come up with (especially since the FileUtil and CleanupJob classes are designed to hide exceptions instead of to expose them). @JabRef/developers what do you think?

[derebaba]

Thanks for the comprehensive explanation. I got a little confused at first. Check out my latest commit. It should do the trick if I understand correctly.

[tobiasdiez]

Thanks for the quick implementation! The code is exactly how I meant it, good job!
The only problem left is the code duplication from the old methods and the new ones that throw an exception. But this should be easy to fix by calling the withException method in the original one; along the following lines:

public static boolean renameFile(Path fromFile, Path toFile, boolean replaceExisting) {
        try {
            return renameFileWithException(fromFile, toFile, replaceExisting);
        } catch (IOException e) {
            LOGGER.error("Renaming Files failed", e);
            return false;
        }
}

Similarly for the cleanup method. If this is changed as well, I give my 👍 for merge.

[derebaba]

I removed the duplicate code as you explained. It is my first contribution ever to an open source project. Thank you for being so helpful.

[Siedlerchr]

Yeah! That looks good now! Thanks for your contribution!
We are looking forward to see more from you ;)