Allow case sensitive local file rename #3593
Conversation
|
Can you run |
| newFile.setMode(OpenMode.ROOT); | ||
| a = !file.exists() && file1.exists(); | ||
| } | ||
| errorCallBack.done(newFile, a); |
There was a problem hiding this comment.
Need a fallback mechanism in case the second rename fails, fallback should attempt to rename back to original file name.
There was a problem hiding this comment.
I added a simple check that attempt to rename back to original file name once, not sure if it is sufficient.
There was a problem hiding this comment.
Thanks, changes seem fine. It would be a good practice to LOG.warn the inner else condition in case second rename fails.
Also, codacy does seem to complain can you fix the issue:
Check warning on line 673 in app/src/main/java/com/amaze/filemanager/filesystem/Operations.java
Codacy Production
/ Codacy Static Code Analysis
app/src/main/java/com/amaze/filemanager/filesystem/Operations.java#L673
These nested if statements could be combined
There was a problem hiding this comment.
I added a log message and combined those if statements.
app/src/main/java/com/amaze/filemanager/filesystem/Operations.java
Outdated
Show resolved
Hide resolved
Managed to run spotlessCheck after applying this fix but assemble debug failure is caused by something else. I am using jdk 11. |
| @@ -441,6 +441,61 @@ public static void rename( | |||
|
|
|||
| private final DataUtils dataUtils = DataUtils.getInstance(); | |||
|
|
|||
| private final Boolean isCaseSensitiveRename = | |||
There was a problem hiding this comment.
Why is the case sensitive rename different? Shouldn't there just be filesystems that support case sensitive renames and filesystems that do not? And the renames be the same function?
There was a problem hiding this comment.
This approach only cares about the value changed and not the type of filesystem. Although it does unnecessary double renaming in case-sensitive filesystem, it doesn't need to check the type of filesystem.
The 'localRename' function (can't think of a better name) is extracted from the original code to avoid code duplication.
There was a problem hiding this comment.
Why is the case sensitive rename different? Shouldn't there just be filesystems that support case sensitive renames and filesystems that do not? And the renames be the same function?
I think this should be okay as we know for sure most file systems that android supports doesn't support case sensitivity (exception ext4 in which case it needs to be kernel enabled). But I think it would make a marginal performance improvement if second part of the AND condition in this is used first.
There was a problem hiding this comment.
But I think it would make a marginal performance improvement if second part of the AND condition in this is used first.
I am more concerned on code legibility.
There was a problem hiding this comment.
I added comment on the purpose of isCaseSensitiveRename variable and refactored double rename part. Hopefully it is clearer.
I think it's building just fine now remotely. For your local builds, I think you can point to your local jre instead. Make the following change in gradle.properties (if you're in mac). |
stevealexr
force-pushed
the
release/4.0
branch
from
becb666 to
b56b12b
Compare
app/src/main/java/com/amaze/filemanager/filesystem/Operations.java
Outdated
Show resolved
Hide resolved
| @@ -441,6 +441,74 @@ public static void rename( | |||
|
|
|||
| private final DataUtils dataUtils = DataUtils.getInstance(); | |||
|
|
|||
| /** | |||
| * Determines whether double rename is required based on original and new file name regardless | |||
| * of the case-sensitivity of the filesystem | |||
There was a problem hiding this comment.
This only depends on the case sensitivity of the filesystem.
There was a problem hiding this comment.
I change it to a filesystem case-sensitivity check by checking whether both upper case and lower case input name for file exists return true.
There was a problem hiding this comment.
This new solution seem to be costly solution and calculating this each time is redundant. If a filesystem is case insensitive then it'll always be case insensitive, so you can persist the result of this in shared preferences and use that from next time.
There was a problem hiding this comment.
There are multiple local filesystems like sdcard storage and internal storage. The types of filesystem need to be stored with the mount points. Rolling back to previous solution seems easier.
There was a problem hiding this comment.
Sure please revert back to your old solution. Also note that the new code will double rename even when FS is insensitive and new file and old file names aren't similar at all.
oldFile.getSimpleName().equalsIgnoreCase(newFile.getSimpleName())
&& !oldFile.getSimpleName().equals(newFile.getSimpleName());
Please also note that I've increased the bounty price for this issue to 10USD you can claim once this PR is merged from the linked issue.
|
Please claim bounty here @stevealexr |
|
Description
The FAT filesystem in android sd card is case-insensitive. This allows case-sensitive renaming by checking whether the changed name and original name is equivalent case-insensitively and doing a double rename. Only the code for local file renaming is changed.
Not sure what indent to use.
Issue tracker
Fixes #3587
Automatic tests
Manual tests
Done
Device: Resizable
OS: Api 33
Build tasks success
Successfully running following tasks on local:
./gradlew assembledebug./gradlew spotlessCheckThese tasks failed even with unmodified source code so probably local configuration issues