[WIP] FileUtil: copy files using Files methods by motokito · Pull Request #1907 · JabRef/jabref

motokito · 2016-09-01T14:03:11Z

Jabref now copies files by using java.nio package instead of java.io

boceckts · 2016-09-01T14:54:58Z

            boolean res = false;
            try {
-                res = FileUtil.copyFile(file, tmpFile.toFile(), true);
+                res = FileUtil.copyFile(Paths.get(file.toURI()), tmpFile, true);


Since tmpFile is already a Path object, why don't you replace File file with Path file?

boceckts · 2016-09-01T15:07:43Z

Can you please try to refactor renameFile(String fileName, String destFilename) to use java nio as well?

Siedlerchr · 2016-09-01T15:26:14Z

Nice that you want to take car of it, well a couple of days ago I already had the same idea: 😆
Feel free to use that for your changes // Edit: There is already code for renaming and a test file
#1861

boceckts · 2016-09-01T15:56:31Z

@Siedlerchr thx for the notice
@motokito please add tests for your changes/modify existing ones (if applicable).

…feedbacks of tobias boceckts have done

Siedlerchr · 2016-09-12T18:04:48Z

+            try {
+                renameSuccessful = FileUtil.renameFile(Paths.get(expandedOldFilePath), Paths.get(newPath));
+            } catch (IOException e) {
+                e.printStackTrace();


Please use/introdue logging with the Logger class and then log the excpetion with LOGGER.error("message", ex)

…_14/09/2016

tschechlovdev

Please fix travis, seems to be a problem in the RenamePdfCleanup.

tschechlovdev · 2016-09-16T11:31:49Z

        // Check if the file already exists.
-        if (dest.exists() && !deleteIfExists) {
+        if (!Files.exists(pathToSourceFile)) {
            return false;


I think it would be nice if you would log here, why the file wasn't copied. So here because the path to the source file not exists.

tschechlovdev · 2016-09-16T11:32:22Z

-            }
-            out.flush();
+        if (Files.exists(pathToDestinationFile) && !replaceExisting) {
+            return false;


Logging here would be also nice.

tschechlovdev · 2016-09-16T11:36:01Z

-                out.write(el);
-            }
-            out.flush();
+        if (Files.exists(pathToDestinationFile) && !replaceExisting) {


Could you add a test case for the left part of the if coniditon? Or reference it if you have already one, because I don't see it.

the testcase is:
testCopyFileFromEmptySourcePathToEmptyDestinationPathWithOverrideExistFile

tschechlovdev · 2016-09-16T11:52:15Z

-            boolean renameSuccessful = FileUtil.renameFile(expandedOldFilePath, newPath);
+            boolean renameSuccessful = false;
+            try {
+                renameSuccessful = FileUtil.renameFile(Paths.get(expandedOldFilePath), Paths.get(newPath));


I think it would be better to place the try/catch construct in FileUtil.renameFile, because the exception is actually thrown by Files.move(...)

tschechlovdev · 2016-09-16T11:53:18Z

+        if (!Files.exists(fromFile) || !Files.exists(toFile)) {
+            return false;
+        }
+        return Files.move(fromFile, fromFile.resolveSibling(toFile), StandardCopyOption.REPLACE_EXISTING) != null;


remove throws IOException and add try/catch here.

Siedlerchr · 2016-09-19T16:47:20Z

-            LOGGER.error("Problem copying file", e);
-            return false;
-        }
+        FileUtil.copyFile(Paths.get(fileName), Paths.get(destFile.toURI()), true);


For the second argument you can use the method toPath() of the File class

boceckts · 2016-09-19T17:17:28Z

-            // repeating the action will have a different result.
-            // On the other hand, our temporary file should still be clean, and won't be deleted.
-            throw new SaveException("Save failed while committing changes: " + ex2.getMessage(),
-                    Localization.lang("Save failed while committing changes: %0", ex2.getMessage()));


Please remove this string from the localization files. This should fix the failing travis build. Otherwise LGTM

* Remove Localization.lang "Save failed while committing changes: %0" from the localization files * Use toPath() on the second argument of the method FileUtil.copyFile(...) in class DroppedFileHandler

motokito · 2016-09-21T11:30:49Z

@Siedlerchr Hiho i have done the feedbacks,. Please a look and merge it please 😄

Siedlerchr

Good 👍
If you please fix the one log message, I'm happpy and I merge it in!

Siedlerchr · 2016-09-21T11:34:40Z

+            try {
+                entry.updateTimeStamp();
+            } catch (IOException e) {
+                e.printStackTrace();


Please use LOGGER.error...

* Insert LOGGER.error instead of e.printStackTrace() in method updateTimeStamp(String key) in class FileUpdateMonitor

Siedlerchr · 2016-09-21T11:58:42Z

Just merged it in! 👍

* FileUtil: copy files using Files methods * Refactor renameFile, create testcase for copyFile and renameFile and feedbacks of tobias boceckts have done * Create extra testcase incl. feedbacks * Refactoring_19.09.2016 - Dennis feedback * Feedback_21.09.2016: * Remove Localization.lang "Save failed while committing changes: %0" from the localization files * Use toPath() on the second argument of the method FileUtil.copyFile(...) in class DroppedFileHandler * Feedback_21.09.2016_13:50: * Insert LOGGER.error instead of e.printStackTrace() in method updateTimeStamp(String key) in class FileUpdateMonitor

FileUtil: copy files using Files methods

d5dfe3c

boceckts added stupro labels Sep 1, 2016

boceckts reviewed Sep 1, 2016
View reviewed changes

boceckts removed the stupro-ready-for-internal-review label Sep 1, 2016

motokito added 2 commits September 12, 2016 12:34

Merge remote-tracking branch 'upstream/master' into usingFilesMethods

6f5c6f8

Refactor renameFile, create testcase for copyFile and renameFile and …

37b5297

…feedbacks of tobias boceckts have done

Siedlerchr reviewed Sep 12, 2016
View reviewed changes

motokito added 2 commits September 14, 2016 13:10

Merge remote-tracking branch 'upstream/master' into usingFilesMethods…

f7ea267

…_14/09/2016

Create extra testcase incl. feedbacks

4345bc8

tschechlovdev reviewed Sep 16, 2016

View reviewed changes

Refactoring_19.09.2016 - Dennis feedback

66e12cf

tschechlovdev approved these changes Sep 19, 2016

View reviewed changes

Siedlerchr reviewed Sep 19, 2016

View reviewed changes

boceckts suggested changes Sep 19, 2016

View reviewed changes

Feedback_21.09.2016:

fb9b95b

* Remove Localization.lang "Save failed while committing changes: %0" from the localization files * Use toPath() on the second argument of the method FileUtil.copyFile(...) in class DroppedFileHandler

boceckts approved these changes Sep 21, 2016

View reviewed changes

boceckts added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 21, 2016

Siedlerchr approved these changes Sep 21, 2016

View reviewed changes

Feedback_21.09.2016_13:50:

7544029

* Insert LOGGER.error instead of e.printStackTrace() in method updateTimeStamp(String key) in class FileUpdateMonitor

Siedlerchr merged commit bddca15 into JabRef:master Sep 21, 2016

motokito deleted the usingFilesMethods branch September 21, 2016 12:00

koppor mentioned this pull request Sep 23, 2016

[WIP] Cleanup FileUtil class and related things #1861

Closed

4 tasks

Uh oh!

Conversation

motokito commented Sep 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boceckts commented Sep 1, 2016

Uh oh!

Siedlerchr commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boceckts commented Sep 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschechlovdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

motokito commented Sep 21, 2016

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr commented Sep 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Siedlerchr commented Sep 1, 2016 •

edited

Loading