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

Fixed crash during folder creation (autosync) #5465

Merged
merged 6 commits into from
Feb 18, 2020

Conversation

Kilian-Perisset
Copy link
Member

@Kilian-Perisset Kilian-Perisset commented Feb 13, 2020

Hi,
I fixed bug relative to folder picker, because mFile was always returning null.

Fixes #4792

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #5465 into master will increase coverage by 0.31%.
The diff coverage is 41.17%.

@@             Coverage Diff              @@
##             master    #5465      +/-   ##
============================================
+ Coverage     21.78%   22.09%   +0.31%     
  Complexity        3        3              
============================================
  Files           398      398              
  Lines         33326    33362      +36     
  Branches       4674     4680       +6     
============================================
+ Hits           7259     7373     +114     
+ Misses        24888    24802      -86     
- Partials       1179     1187       +8
Impacted Files Coverage Δ Complexity Δ
...loud/android/ui/activity/FolderPickerActivity.java 32.57% <41.17%> (+32.57%) 0 <0> (ø) ⬇️
...va/com/owncloud/android/ui/SquareLinearLayout.java 0% <0%> (-50%) 0% <0%> (ø)
...d/android/operations/GetCapabilitiesOperation.java 75% <0%> (-12.5%) 0% <0%> (ø)
...xtcloud/client/account/UserAccountManagerImpl.java 60.68% <0%> (-1.73%) 0% <0%> (ø)
...wncloud/android/jobs/MediaFoldersDetectionJob.java 23.27% <0%> (-1.5%) 0% <0%> (ø)
.../third_parties/daveKoeller/AlphanumComparator.java 82.14% <0%> (-1.2%) 0% <0%> (ø)
...owncloud/android/ui/adapter/OCFileListAdapter.java 35.42% <0%> (-0.72%) 0% <0%> (ø)
.../java/com/owncloud/android/utils/DisplayUtils.java 30.9% <0%> (-0.43%) 0% <0%> (ø)
...in/java/com/owncloud/android/utils/ThemeUtils.java 52.69% <0%> (-0.39%) 0% <0%> (ø)
...cloud/android/ui/activity/FileDisplayActivity.java 22.97% <0%> (-0.32%) 0% <0%> (ø)
... and 18 more

@nextcloud-android-bot
Copy link
Collaborator

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

@@ -0,0 +1,71 @@
package com.owncloud.android.ui.activity;
Copy link
Member

Choose a reason for hiding this comment

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

@Shagequi can you add the license header please :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will be done

@@ -302,7 +299,8 @@ public boolean onOptionsItemSelected(MenuItem item) {
boolean retval = true;
switch (item.getItemId()) {
case R.id.action_create_dir: {
CreateFolderDialogFragment dialog = CreateFolderDialogFragment.newInstance(getCurrentFolder());
OCFile currentFolder = getCurrentFolder();
CreateFolderDialogFragment dialog = CreateFolderDialogFragment.newInstance(currentFolder);
Copy link
Member

Choose a reason for hiding this comment

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

no need to split this into two statements, could be reverted (or did I miss a detail here?)

Copy link
Member

Choose a reason for hiding this comment

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

I guess this was just for debugging, but true, this can be reverted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup it was for debugging.
I'm glad it's this name, I sometimes have an excess of imagination in my development (for naming variables or logs 😅)...

I'll fix it ASAP, am working on another project

Copy link
Member

Choose a reason for hiding this comment

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

Wo worries, no hurries :)

@AndyScherzinger
Copy link
Member

@Shagequi Just two minor issues 👍

if (currentFile.isFolder()) {
finalFolder = currentFile;
} else if(currentFile.getRemotePath() != null) {
String parentPath = currentFile
Copy link
Member

Choose a reason for hiding this comment

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

I would use new File(currentFile.getRemotePath()).getParent() for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I forgot this point too,
will be fixed asap too

Copy link
Member

Choose a reason for hiding this comment

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

"asap" -> do not hurry too much :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

This method doesn't work, we need to use the FileStorageManager in all cases, I stay with this solution for now

Copy link
Member

Choose a reason for hiding this comment

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

String parentPath = new File(currentFile.getRemotePath()).getParent();

                String parentPath1 = currentFile
                        .getRemotePath()
                        .substring(0, currentFile.getRemotePath()
                        .lastIndexOf(currentFile.getFileName()));

Should be exactly the same, and is not error prone :-)

Copy link
Member Author

@Kilian-Perisset Kilian-Perisset Feb 14, 2020

Choose a reason for hiding this comment

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

But, why create a File object, if we're expecting an OCFile object ?

Copy link
Member

Choose a reason for hiding this comment

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

This is just for handling the path part.
I think, using File, is more error prone than trying to to it by our selves.

@nextcloud-android-bot
Copy link
Collaborator

Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

@Kilian-Perisset
Copy link
Member Author

@tobiasKaminsky Now it's done, would you please merge this important fix ?

@tobiasKaminsky tobiasKaminsky force-pushed the fix/create-folder-crash-autosync branch from 49842a9 to a93313c Compare February 17, 2020 14:05
@tobiasKaminsky
Copy link
Member

I rebased, added two tests and if everything is green, we can merge.

@nextcloud-android-bot
Copy link
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12714.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot
Copy link
Collaborator

Codacy

360

Lint

TypemasterPR
Warnings7777
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings71
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings78
Security Warnings45
Dodgy code Warnings138
Total385

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings71
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings78
Security Warnings45
Dodgy code Warnings138
Total385

@Kilian-Perisset
Copy link
Member Author

All the tests are green :D

@tobiasKaminsky tobiasKaminsky merged commit c243453 into master Feb 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix/create-folder-crash-autosync branch February 18, 2020 06:28
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.11.0 milestone Feb 18, 2020
tobiasKaminsky added a commit that referenced this pull request Feb 19, 2020
3f62afa Merge remote-tracking branch 'origin/master' into dev
5d428fe Merge pull request #4788 from nextcloud/instantupload_all
49f3a67 ConflictsResolveActivity: code style and readability improvements
6540e3e Upload list conflicts: replace trash icon with menu and add resolve action
a4ff1f1 Customize upload sync conflict notification strings
e77ffa0 Fix synced folder layout form control widgets alignment
91ef307 Fix database migration of field forceOverwrite to NameCollisionPolicy
52d089b FileUploader: fix codacy issues and SpotBugs
ec27f84 FileUploader: code cleanup
7713a28 FileUploader: require explicit NameCollisionPolicy and change default used value
6783e8a Make file uploads ask the user what to do when the file already exists on remote
213002f Make newly created synced folders auto upload existing files by default
1b7bd72 ADD: [instantupload] setting to also upload existing files
c243453 Merge pull request #5465 from nextcloud/fix/create-folder-crash-autosync
d178cde daily dev 20200218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App crashes every time I try to create a new folder
5 participants