Allow users to add more attachments to report a bug #22
Conversation
hplin
commented
Apr 10, 2018
hplin
commented
|
@hplin Can you check the lint failures that Travis is seeing? It'd be good to get a passing CI build before going through this change thoroughly |
|
Hi @drewhannay |
There was a problem hiding this comment.
Overall looks ok to me, I'm not a fan of the way we use the attachment views to hold the uri data - seems like using a proper collection view with an adapter would make this cleaner. Let me know what you think.
Lets make sure to get the translation stuff figured out before we push this
| @@ -121,23 +239,95 @@ public boolean onMenuItemClick(MenuItem item) { | |||
| }; | |||
| } | |||
|
|
|||
| private ArrayList<Uri> getAttachments() { | |||
There was a problem hiding this comment.
If we need access to the attachments, it probably makes more sense to store them as a property of this fragment so that we don't need to traverse the whole view heirarchy just to find them
There was a problem hiding this comment.
I will change it to use ListView and your concern should be addressed.
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| private static class AttachmentViewHolder { |
There was a problem hiding this comment.
Lets move this into its own class, seems like there's enough code here to warrant that
| Intent intent = new Intent(ACTION_END_FEEDBACK_FLOW); | ||
|
|
||
| intent.putExtra(SCREENSHOT_URI, imageUri); | ||
| intent.putExtra(TITLE, getString(getTitleResId(feedbackType))); | ||
| intent.putExtra(MESSAGE, userMessage); | ||
| intent.putExtra(USER_DATA, userData); | ||
| if (attachments != null && !attachments.isEmpty()) { | ||
| intent.putParcelableArrayListExtra(EXTRA_ATTACHMENTS, attachments); |
There was a problem hiding this comment.
Any reason we need to define EXTRA_ATTACHMENTS in both the fragment and the activity?
There was a problem hiding this comment.
Well, I just follow the same logic. When delegate tried to get the bundled values, it can use the constants defined in the same class.
There was a problem hiding this comment.
Yea makes sense, i guess these are two different semantic bundles. Sounds good!
| } | ||
|
|
||
| private void removeAttachment(@NonNull Uri fileUri) { | ||
| for (int i = 0, count = attachmentsView.getChildCount(); i < count; i++) { |
There was a problem hiding this comment.
We may want to consider using an adapter here, since this seems like it would lend itself to some kind of list or grid...
There was a problem hiding this comment.
I thought about that but in that case we need to use ListView (for RecyclerView we need to add the dependencies). In most of the cases, users won't add too many attachments or remove them so I think it might be fine to add/remove views from a view group.
Maybe it is easier to use a ListView in this case, I can change it later.
| /** | ||
| * @return true if more attachments are allowed to be attached to the reports | ||
| */ | ||
| public boolean isAddAttachmentFeatureEnabled() { |
There was a problem hiding this comment.
lets call this something like "shouldAllowAdditionalAttachments" or something, to make it a bit more expressive of what this method indicates
| /** | ||
| * Make the file uri persistent | ||
| */ | ||
| static void persistFilePermissions(@NonNull Context context, @NonNull Uri uri, @Nullable Intent data) { |
There was a problem hiding this comment.
I'm not sure i understand why we need this, can you explain a bit?
There was a problem hiding this comment.
I think we don't need this for external shared documents. This is only for if we want to share the internal files.
I will remove it
There was a problem hiding this comment.
Not sure what you mean by "internal files" - this only supports files on the local disk right?
|
@kkoser i18n strings are completely in |
