Added option to remove a language description

[madhurgupta10]

Description
Added option to remove a language description when uploading a new photo.

Fixes #2810

Tests performed
Tested betaDebug on Google Pixel 2 with API level 29.

Screenshots showing what changed

[tests-checker]

Could you please add tests to make sure this change works as expected?

[codecov-io]

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 7.47%. Comparing base (95372f3) to head (ee6c07e).

Files with missing lines	Patch %	Lines
...e/nrw/commons/upload/UploadMediaDetailAdapter.java	0.00%	6 Missing ⚠️
...upload/mediaDetails/UploadMediaDetailFragment.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             master   #3504      +/-   ##
===========================================
+ Coverage      7.41%   7.47%   +0.06%     
  Complexity      340     340              
===========================================
  Files           290     288       -2     
  Lines         12839   12759      -80     
  Branches       1011    1005       -6     
===========================================
+ Hits            952     954       +2     
+ Misses        11812   11729      -83     
- Partials         75      76       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[maskaravivek]

Maybe we could have a better design for removing descriptions. As of now adding the (-) reduces the space for description text box and also all the description boxes are no longer aligned.

Tagging @misaochan and @neslihanturan for inputs.

[ashishkumar468]

Should we use Swipe Gestures, similar to what gmail does?
https://storage.googleapis.com/spec-host-backup/mio-components%2Fassets%2F17m39WCnuYdi1omF2Xpd6fdTaSJPAbRP7%2F02-list-swipetodismiss.mp4. As the edit text is on the right, swiping is tricky over there, so left to right swipe from the left edge or something like that

[maskaravivek]

Should we use Swipe Gestures, similar to what gmail does?
https://storage.googleapis.com/spec-host-backup/mio-components%2Fassets%2F17m39WCnuYdi1omF2Xpd6fdTaSJPAbRP7%2F02-list-swipetodismiss.mp4. As the edit text is on the right, swiping is tricky over there, so left to right swipe from the left edge or something like that

I guess if we try to use swipe gestures, the edit text might get focused inadvertently.

Edit: My original comment maybe got edited by @ashishkumar468

[macgills]

I'd put an edit icon to show the delete icons, also make the delete icons red. Swipe to dismiss is very much a list action and this doesn't feel like a list, if you know what I mean

[misaochan]

Putting myself into a user's shoes, I personally would not think of swiping one of the descriptions unless we switched to a more card- or list-based design. :) What about an "x" or "-" icon on the right side of the edit text field, where the tooltip icons are? That wouldn't reduce the space available, and also wouldn't clash with the tooltip icons (because the first description should not be removable).

@neslihanturan

[ashishkumar468]

Yes, a swipe would not be the best thing to do, a red "x" icon on the right side seems to be the better way, although the point was to save space as this would shrink the EditText's width, but I can't seem to think any other way, this is how the Google-Keep app does it

[misaochan]

I like the Google-Keep app's design. :) Also I changed my mind about it being red... I think being neutral in color (similar to the screenshot above) is better. If it was red the user might think it is a warning about something being wrong with their description.

[madhurgupta10]

@misaochan @maskaravivek @ashishkumar468 Changes Made :)

[madhurgupta10]

@macgills I want to configure the color of the "X" for dark and light themes, what should be the best way to implement this?

[sivaraam]

@madhurgupta10 Just want to share a quick concern with showing the 'X' icon within the text box. It's easy for users to misinterpret that 'X' for an icon that helps them clear the description that they've entered. So, can you ensure that the 'X' icon appears separately from the text box. Let me know if you need more clarification.

[macgills]

This UI will also totally clash with Captions as the delete will need to be for a caption/decription pair @misaochan

[macgills]

@madhurgupta10 Re different icon colors actually quite a bit more annoying because our dark mode is not backed by a DayNight theme which would have allowed us to but a totally different icon in values-night with the same name and then in night mode it would just do that, or if this is a vector then we could just change the color used to draw the path but that also requires DayNight.

The method I have seen so far is declaring an attribute (or checking if there is an already suitable attribute) in the Light and Dark themes in styles.xml and referencing that attribute.

The worst scenario is in code checking if we are in dark or light and using different colors to programmatically tint

[misaochan]

This UI will also totally clash with Captions as the delete will need to be for a caption/decription pair @misaochan

Oh, right.... I had completely forgotten about that. :/ Does anyone have any ideas on how this can be mitigated? For reference, the relevant screenshot is at https://medium.com/@vanshikaa937/google-summer-of-code19-final-post-d78e601ea39d , "title replaced by multilingual captions".

[madhurgupta10]

This UI will also totally clash with Captions as the delete will need to be for a caption/decription pair @misaochan

Oh, right.... I had completely forgotten about that. :/ Does anyone have any ideas on how this can be mitigated? For reference, the relevant screenshot is at https://medium.com/@vanshikaa937/google-summer-of-code19-final-post-d78e601ea39d , "title replaced by multilingual captions".

I think the best way would be to move the X outside of the description as @sivaraam suggested and keeping it on the last edit text i.e. Descriptions

[madhurgupta10]

@madhurgupta10 Just want to share a quick concern with showing the 'X' icon within the text box. It's easy for users to misinterpret that 'X' for an icon that helps them clear the description that they've entered. So, can you ensure that the 'X' icon appears separately from the text box. Let me know if you need more clarification.

Maybe we can also have a confirmation dialog to ensure user doesn't click it by mistake

[sivaraam]

Maybe we can also have a confirmation dialog to ensure user doesn't click it by mistake

Maybe. But that sounds overkill to me. In the best case scenario, the UI should prevent such mistakes. I think if we just ensure that the 'X' 'button isn't very big and not so close to any other UI elements then we're fine. Potentially problematic elements are:

• Info icon: But given the fact it's not an icon that anyone would click on more than once, we can safely ignore this.
• Next button: This is a big button. So, the possibility of someone unintentionally hitting the 'x' icon of the closest description is likely very less. So, this shouldn't be a problem either.

To conclude, in my opinion we really do not need to show a confirmation dialog if the button meets the assumptions mentioned above.

[madhurgupta10]

@misaochan @macgills @sivaraam @maskaravivek @ashishkumar468

I made this mockup. For now, we don't have captions but in the future when we add it this is how it will look, for this PR I will make "X" outside of the description edit text.

Also, for first description, the visibility of "X" will be set to gone.

Can we go for this UI?

[macgills]

The X has to be for caption and description because it deletes both of them

[madhurgupta10]

The X has to be for caption and description because it deletes both of them

How about this UI, "X" is now in center horizontal + vertical?

Another Option could be this!

[macgills]

I like the vertically aligned but I feel like if that is vertically aligned then the language spinner may want to be as well so the item has symmetry?

[madhurgupta10]

I like the vertically aligned but I feel like if that is vertically aligned then the language spinner may want to be as well so the item has symmetry?

Yes that can be done as well, for now, we don't have captions so I would just put "X" outside of the description edit text and in future when we have captions we can go with the last UI mockup (with lang spinner also in center_vertical)

Can I implement it?

[macgills]

Make a ticket to do this on structuredData after we merge this change I'd say, I've sidetracked this PR badly enough

[madhurgupta10]

[madhurgupta10]

@macgills First commit squashed and pushed, let me know if it is done correctly

[macgills]

@madhurgupta10 yup, all looks good. I haven't run the code yet because I am working on another ticket, I'll come back to this before the end of my day hopefully

[macgills]

This is deleting the next item in the list, not the item you click on. Maybe instead of using position at all we should just use the uploadMediaDetail

[madhurgupta10]

This is deleting the next item in the list, not the item you click on. Maybe instead of using position at all, we should just use the uploadMediaDetail

private void removeDescription(final UploadMediaDetail uploadMediaDetail, final int pos) { this.uploadMediaDetails.remove(uploadMediaDetail); notifyItemRemoved(pos); }

We are using just upload media detail when removing, the position is used to notify

[macgills]

@madhurgupta10 I think the problem is also the listeners that fetch data from the list using position, now that arbitrary deletion can happen that isn't valid.
Do not retrieve from uploadMediaDetails during bind aside from at the start and get the position of the item you are removing from the list using indexOf

[madhurgupta10]

@madhurgupta10 I think the problem is also the listeners that fetch data from the list using position, now that arbitrary deletion can happen that isn't valid.
Do not retrieve from uploadMediaDetails during bind aside from at the start and get the position of the item you are removing from the list using indexOf

@macgills One change I understand from your comment is this one

private void removeDescription(final UploadMediaDetail uploadMediaDetail) {
        this.uploadMediaDetails.remove(uploadMediaDetail);
        notifyItemRemoved(uploadMediaDetails.indexOf(uploadMediaDetail));
}

but I am not sure why this isn't valid
UploadMediaDetail uploadMediaDetail = uploadMediaDetails.get(position);

[macgills]

private void removeDescription(final UploadMediaDetail uploadMediaDetail) {
        this.uploadMediaDetails.remove(uploadMediaDetail);
        notifyItemRemoved(uploadMediaDetails.indexOf(uploadMediaDetail));
}

that change won't work, you can't get the index of a removed item, well you can but it will be -1

It is just logically cleaner that everything uses the same item in the bind as we are binding the view for this specific item, as a bonus it helps stop errors and reduced duplication, lots of click listeners repeating uploadMediaDetails.get(position)

[neslihanturan]

Hi @madhurgupta10 , are you around and still interested with this PR? Seems like the PR is sleeping since removing wrong item issue is reported.

[madhurgupta10]

Hi @madhurgupta10 , are you around and still interested with this PR? Seems like the PR is sleeping since removing wrong item issue is reported.

Hi @neslihanturan sorry for the sleeping PR, I started on my GSoC project and forgot about it, please allow me some time to make the changes.

[neslihanturan]

Take your time @madhurgupta10 :)

[neslihanturan]

Kindly asking about the status of this PR @madhurgupta10 :)

[madhurgupta10]

@neslihanturan 😅I didn't work on this PR for a while and the conflicts would take a lot more effort to resolve, I am closing this for now and will leave the issue open to pick up for others.