Skip to content

Fix crash during text file creation #5442

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.

Sign up for GitHub

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

Merged
tobiasKaminsky merged 1 commit into from
Feb 17, 2020
Merged

Fix crash during text file creation #5442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/analysis/findbugs-results.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
386
385
56 changes: 29 additions & 27 deletions src/main/java/com/owncloud/android/ui/dialog/ChooseTemplateDialogFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import android.annotation.SuppressLint;
import android.app.Activity;
import android.app.Dialog;
import android.content.Context;
import android.content.DialogInterface;
import android.content.Intent;
import android.graphics.PorterDuff;
Expand Down Expand Up @@ -241,35 +242,36 @@ protected String doInBackground(Void... voids) {
new DirectEditingCreateFileRemoteOperation(path,
creator.getEditor(),
creator.getId(),
template.getTitle())
.execute(client);

if (result.isSuccess()) {
// get file
RemoteOperationResult newFileResult = new ReadFileRemoteOperation(path)
.execute(client);

if (newFileResult.isSuccess()) {
OCFile temp = FileStorageUtils.fillOCFile((RemoteFile) newFileResult.getData().get(0));

if (chooseTemplateDialogFragmentWeakReference.get() != null) {
FileDataStorageManager storageManager = new FileDataStorageManager(
user.toPlatformAccount(),
chooseTemplateDialogFragmentWeakReference.get().requireContext().getContentResolver());
Copy link
Collaborator Author

@ezaquarii ezaquarii Feb 10, 2020

Choose a reason for hiding this comment

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

This is pretty common error I see. Both activities and fragments are passed as weak references to avoid leaks, but in case of fragment we must check if it has been detached.

storageManager.saveFile(temp);
file = storageManager.getFileByPath(path);

return result.getData().get(0).toString();
}
else {
return "";
}
} else {
return "";
}
} else {
template.getTitle()).execute(client);
if (!result.isSuccess()) {
return "";
}

RemoteOperationResult newFileResult = new ReadFileRemoteOperation(path).execute(client);
if (!newFileResult.isSuccess()) {
return "";
}

OCFile temp = FileStorageUtils.fillOCFile((RemoteFile) newFileResult.getData().get(0));
Copy link
Collaborator

@nextcloud-android-bot nextcloud-android-bot Feb 17, 2020

Choose a reason for hiding this comment

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

Codacy Issue found: Avoid declaring a variable if it is unreferenced before a possible exit point.


final ChooseTemplateDialogFragment fragment = chooseTemplateDialogFragmentWeakReference.get();
if (fragment == null) {
return "";
}

final Context context = fragment.getContext();
if (context == null) {
Copy link
Member

@tobiasKaminsky tobiasKaminsky Feb 10, 2020

Choose a reason for hiding this comment

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

There is also "isAdded" or "isDetached", which we can use, or?

getContext is: return mHost == null ? null : mHost.getContext()
isAdded is: return mHost != null && mAdded

So isAdded might even the more extensive check, or?

Copy link
Collaborator Author

@ezaquarii ezaquarii Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you can use that, as it calls @NonNull FragmentHostCallback.getContext().

However, because subsequent call to getContext() is not @NonNull, linter will still complain that you use nullable Context, encouraging another, rendundant check.

Because the action run here is not related to UI elements displayed on the screen, we can rely on Context check without risk of any glitches.

Android API is not a pinnacle of software engineering, and Fragment API is of particularly bad reputation in general. I'd say that looking for an inspiration for best practices in the official documentation is rather risky endeavour. :)

Copy link
Member

@tobiasKaminsky tobiasKaminsky Feb 14, 2020

Choose a reason for hiding this comment

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

True.
Then let us keep it this way 👍

// fragment has been detached
return "";
}

FileDataStorageManager storageManager = new FileDataStorageManager(user.toPlatformAccount(),
context.getContentResolver());
storageManager.saveFile(temp);
file = storageManager.getFileByPath(path);

return result.getData().get(0).toString();

} catch (ClientFactory.CreationException e) {
Log_OC.e(TAG, "Error creating file from template!", e);
return "";
Expand Down