Fixed 4616 : Option for editing depictions by Ayan-10 · Pull Request #4725 · commons-app/apps-android-commons

Ayan-10 · 2021-12-10T14:01:17Z

Description (required)

Made option for editing depictions.

Fixes #4616

What changes did you make and why?

In the current app depicts can be added only while uploading, it's very necessary for a user to edit depictions.

I reused the depiction selection UI which is being used while uploading for letting user select the depicts
Then I compare it with old depicts and send only new depicts for posting to the server
A POST API call will post the new depicts in the commons server

Tests performed (required)

Tested lastest master betaDebug and ProdDebug on Pixel 3 with API level 30.

TODO

All tested with latest version after resetting data

codecov · 2021-12-10T14:14:19Z

Codecov Report

Merging #4725 (e647f8b) into master (1078c70) will increase coverage by 0.45%.
The diff coverage is 54.41%.

@@             Coverage Diff              @@
##             master    #4725      +/-   ##
============================================
+ Coverage     50.59%   51.04%   +0.45%     
- Complexity     2228     2293      +65     
============================================
  Files           338      341       +3     
  Lines         15599    15960     +361     
  Branches       1370     1410      +40     
============================================
+ Hits           7892     8147     +255     
- Misses         7114     7193      +79     
- Partials        593      620      +27

Impacted Files	Coverage Δ
...e/nrw/commons/notification/NotificationHelper.java	`100.00% <ø> (ø)`
...free/nrw/commons/wikidata/WikidataEditService.java	`29.59% <33.33%> (+2.76%)`	⬆️
...ee/nrw/commons/upload/depicts/DepictsFragment.java	`55.33% <41.66%> (-27.02%)`	⬇️
...ree/nrw/commons/upload/depicts/DepictsPresenter.kt	`62.31% <45.71%> (-10.05%)`	⬇️
...fr/free/nrw/commons/media/MediaDetailFragment.java	`37.10% <55.55%> (+2.99%)`	⬆️
...a/fr/free/nrw/commons/wikidata/WikiBaseClient.java	`35.00% <66.66%> (+11.47%)`	⬆️
...n/java/fr/free/nrw/commons/upload/UploadModel.java	`37.14% <72.72%> (+11.56%)`	⬆️
...ree/nrw/commons/upload/depicts/DepictEditHelper.kt	`86.84% <86.84%> (ø)`
.../free/nrw/commons/repository/UploadRepository.java	`95.77% <92.85%> (-0.78%)`	⬇️
...r/free/nrw/commons/contributions/MainActivity.java	`74.86% <100.00%> (+0.13%)`	⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1078c70...e647f8b. Read the comment docs.

nicolas-raoul

Thanks, that is extremely helpful!
I noticed a few issues:

After saving, depiction does not appear immediately in the media details (above the pen).
Pressing the "Back" button in the depictions editor sometimes does nothing at first, the UI freezes, then all of the backs are done very quickly. I observed something similar in the categories editor pull request.
The nearby banner appears at the top, if you open the dialog fast enough after starting the app.

…ndroid-commons into 4616_edit_depicts

Ayan-10 · 2021-12-15T06:48:35Z

@nicolas-raoul I think "Depictions not updating instantly" issue is resolved now.

nicolas-raoul · 2021-12-22T08:54:07Z

Would you mind solving the conflict? Sorry for the delay in reviewing, and thanks for your patience!

Ayan-10 · 2021-12-22T13:05:26Z

@nicolas-raoul I resolved the conflicts and also could you please review #4515 once? the requested changes are made a long ago.

nicolas-raoul

I noticed another issue: On a picture that already has a depiction, the Edit Depicts dialog does not show that depiction. If I search for it, it is shown as checked.
Desired behavior: When opening Edit Depicts, it should show all current depictions (with the checkbox checked).
Thanks a lot!

nicolas-raoul

Also, pressing Back should bring back to the media details.
Currently, pressing back brings me to the list of media (one level too far).

Ayan-10 · 2021-12-29T04:09:52Z

I noticed another issue: On a picture that already has a depiction, the Edit Depicts dialog does not show that depiction. If I search for it, it is shown as checked. Desired behavior: When opening Edit Depicts, it should show all current depictions (with the checkbox checked). Thanks a lot!

To show all existing depicts at the top we have to make a call to get DepictedItems because only Depicted IDs are available as existing depicts. In this PR I just compared the existing IDs with the search result and changed their checked states accordingly.

nicolas-raoul · 2021-12-29T05:25:50Z

So you would have to call the API (with the depicted IDs) to get the information required for displaying them, right?

That sounds doable, and I think it is important to have a consistent behavior with the Upload Wizard :-)

nicolas-raoul · 2021-12-30T01:42:57Z

Great, the UI now shows already existing depictions! :-)

Remaining bugs:

Pressing "Back" goes back one level too far
If you wait for a few minutes on the UI, pressing the "Back" button in the depictions editor sometimes does nothing at first, the UI freezes.
The Nearby top banner sometimes appears, for instance if you open the dialog fast enough after starting the app.
Open the UI for a picture, close it, open for another picture: The depictions of the previous picture are still visible for a few seconds. They should not appear.
The UI does not take the whole screen: A bit of the media details is still visible at the bottom.
After adding a new depiction and saving, then opening the edition dialog again, the checkbox is not checked anymore.
When editing several pictures in a row, the edits are erroneous, for instance a picture for which I checked and saved "street" got actually edited (when looking at the Commons website) as "beach", a depiction I had used for a different picture before.

Ayan-10 · 2021-12-30T07:51:59Z

Open the UI for a picture, close it, open for another picture: The depictions of the previous picture are still visible for a few seconds. They should not appear.

@nicolas-raoul Currently the depictions of the previous picture are appearing as checked. I will remove the checked state of previous depictions. Is this ok with you or the previous ones should not appear at all?

nicolas-raoul · 2021-12-30T08:04:00Z

I will remove the checked state of previous depictions

Sounds good! :-)

nicolas-raoul · 2022-03-19T13:48:38Z

codeconv says that this pull request reduces unit test coverage significantly... would you mind trying to add more unit tests? @madhurgupta10 any recommendations maybe?

madhurgupta10 · 2022-03-19T15:19:35Z

The diff coverage is 40.93%.

The diff coverage is 40.93% which means only about 40% of new lines are tested. Looking at the scope of this PR it is highly important to get around 70-80%

@Ayan-10 Try to break your code blocks into smaller blocks to test individual units (TDD)

Ayan-10 · 2022-03-19T16:18:23Z

@madhurgupta10 Thanks a lot for the suggestion. But I think I added tests for most of the new lines. I think the problem here is a lot of lines gets auto intended that's why even if I didn't change anything on those lines, those lines are showing as changed lines. I will try to reverse the indentation. Other than that I added tests for all the lines that I really added or edited. @nicolas-raoul I think that will increase the diff coverage.

Thanks.

…ndroid-commons into 4616_edit_depicts

nicolas-raoul · 2022-03-20T01:22:17Z

Thanks for reversing the indentation changes, that makes the pull request much clearer! :-)

The code coverage now looks much better, but still some effort needed before a good coverage rate is reached. Would you mind following Madhur's advice breaking your code blocks into smaller blocks to test individual units? Thanks a lot!

Ayan-10 · 2022-03-20T16:04:59Z

@nicolas-raoul Yes, I can surely break the codes into small test individual units. But according to codecov/patch all the large blocks are already covered, among the left ones most of them are individual units. The problem that I found here is most of the uncovered lines need a real device for testing otherwise they will show errors in unit testing for example getParentFragment or getView. I tried to cover the lines as much as possible even the single line codes. If I try I can increase the coverage a little more but 70-80% apparently seems not possible.

@madhurgupta10 please have a look and if I am wrong somewhere please give your input.

Thanks a lot.

madhurgupta10 · 2022-03-20T16:07:38Z

I think the coverage is sufficient for this PR to be merged, you can add instrumentation tests in future if needed.

nicolas-raoul

Minor variable name refactoring

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsContract.java

nicolas-raoul · 2022-03-21T07:21:05Z

app/src/main/res/layout/fragment_media_detail.xml

                </LinearLayout>

+                <Button
+                  android:id="@+id/depictEditButton"


depictEditButton -> depictionsEditButton

app/src/main/java/fr/free/nrw/commons/contributions/MainActivity.java

nicolas-raoul · 2022-03-21T07:22:49Z

app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.java

+     * By clicking on the edit depicts button, it will send user to depict fragment
+     */
+    @OnClick(R.id.depictEditButton)
+    public void onDepictEditButtonClicked() {


onDepictEditButtonClicked -> onDepictionsEditButtonClicked

nicolas-raoul · 2022-03-21T07:26:43Z

app/src/main/java/fr/free/nrw/commons/notification/NotificationHelper.java

    public static final int NOTIFICATION_EDIT_CATEGORY = 2;
    public static final int NOTIFICATION_EDIT_COORDINATES = 3;
    public static final int NOTIFICATION_EDIT_DESCRIPTION = 4;
+    public static final int NOTIFICATION_EDIT_DEPICT = 5;


NOTIFICATION_EDIT_DEPICT -> NOTIFICATION_EDIT_DEPICTIONS

nicolas-raoul · 2022-03-21T07:40:56Z

app/src/main/res/values/strings.xml

+    <item quantity="one">Depiction %1$s is added.</item>
+    <item quantity="other">Depictions %1$s are added.</item>
+  </plurals>
+  <string name="depict_edit_helper_edit_message_else">Could not add depictions.</string>


depict_edit_helper_edit_message_else -> depictions_edit_helper_edit_message_else

nicolas-raoul · 2022-03-21T07:41:08Z

app/src/main/res/values/strings.xml

  <string name="contributions_of_user">Contributions of User: %s</string>
  <string name="achievements_of_user">Achievements of User: %s</string>
  <string name="menu_view_user_page">View user page</string>
+  <string name="edit_depicts">Edit depictions</string>


edit_depicts -> edit_depictions

nicolas-raoul · 2022-03-21T07:45:18Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsPresenter.kt

+     * and saves them in local storage
+     */
+    @SuppressLint("CheckResult")
+    override fun updateDepicts(media: Media) {


updateDepicts -> updateDepictions

nicolas-raoul · 2022-03-21T07:45:54Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsPresenter.kt

+                        })
+                        {
+                            Timber.e(
+                                "Failed to update depicts"


depicts -> depictions

nicolas-raoul · 2022-03-21T07:46:22Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsContract.java

+
+        void goBackToPreviousScreen();
+
+        List<String> getExistingDepicts();


getExistingDepicts -> getExistingDepictions

nicolas-raoul · 2022-03-22T05:33:49Z

Thanks Ayan! That's a great feature to have, it was not easy but thanks for your perseverance!

Ayan-10 · 2022-03-22T09:13:31Z

Thanks for your appreciation.

Ayan-10 and others added 10 commits November 15, 2021 17:11

Dialog can't be dismissed

6abab81

Dialog can't be dismissed

e82a02e

Merge branch 'commons-app:master' into master

934aa34

Merge branch 'commons-app:master' into master

bf0aa4f

Merge branch 'commons-app:master' into master

e40d7ca

Option for editing depiction

5aad0da

Java docs added

4af1f47

Minor issues fixed

e7d41e9

Lining done

fe80b1c

Merge branch 'master' into 4616_edit_depicts

5d5f530

nicolas-raoul reviewed Dec 12, 2021

View reviewed changes

Ayan-10 and others added 3 commits December 15, 2021 09:16

Merge branch 'master' into 4616_edit_depicts

d66a9da

"Depictions not updating instantly" issue resolved

c925978

Merge branch '4616_edit_depicts' of https://github.com/Ayan-10/apps-a…

ba2f541

…ndroid-commons into 4616_edit_depicts

Merge branch 'master' into 4616_edit_depicts

48ae944

nicolas-raoul requested changes Dec 29, 2021

View reviewed changes

Merge branch 'commons-app:master' into 4616_edit_depicts

2a98e25

Ayan-10 added 2 commits December 29, 2021 20:48

Existing Depicts on the top

1c1287c

Existing Depicts on the top

d6a0102

Back press handled

117fe39

Ayan-10 added 3 commits March 18, 2022 21:31

DepictEditHelperUnitTest added

28e87f5

DepictEditHelperUnitTest added

3144ae1

Test added

87a562b

More test added

5242692

Ayan-10 and others added 10 commits March 19, 2022 23:26

Indentation Reversed

a83d9ae

Indentation reversed

025fd92

Update MediaDetailFragment.java

6d01519

Indentation reversed

c57e372

Update MediaDetailFragment.java

f658929

Indentation reversed

38b8105

Merge branch '4616_edit_depicts' of https://github.com/Ayan-10/apps-a…

93c261a

…ndroid-commons into 4616_edit_depicts

Indentation reversed

889975b

Indentation reversed

425a817

Indentation reversed

4af8c17

Ayan-10 added 2 commits March 20, 2022 10:19

More test added

faa9e30

More test added

f454db9

nicolas-raoul requested changes Mar 21, 2022

View reviewed changes

Ayan-10 added 3 commits March 21, 2022 21:33

Minor fixes

5d1925e

Minor fixes

960b844

Minor fixes

e647f8b

nicolas-raoul merged commit bd9531b into commons-app:master Mar 22, 2022

parneet-guraya mentioned this pull request Dec 23, 2024

Trying to add a depiction, existing depiction was removed #6068

Closed


		void goBackToPreviousScreen();

		List<String> getExistingDepicts();

Conversation

Ayan-10 commented Dec 10, 2021 • edited by nicolas-raoul Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

codecov bot commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nicolas-raoul left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ayan-10 commented Dec 15, 2021

Uh oh!

nicolas-raoul commented Dec 22, 2021

Uh oh!

Ayan-10 commented Dec 22, 2021

Uh oh!

nicolas-raoul left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-raoul left a comment

Choose a reason for hiding this comment

Uh oh!

Ayan-10 commented Dec 29, 2021

Uh oh!

nicolas-raoul commented Dec 29, 2021

Uh oh!

nicolas-raoul commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ayan-10 commented Dec 30, 2021

Uh oh!

nicolas-raoul commented Dec 30, 2021

Uh oh!

nicolas-raoul commented Mar 19, 2022

Uh oh!

madhurgupta10 commented Mar 19, 2022

Uh oh!

Ayan-10 commented Mar 19, 2022

Uh oh!

nicolas-raoul commented Mar 20, 2022

Uh oh!

Ayan-10 commented Mar 20, 2022

Uh oh!

madhurgupta10 commented Mar 20, 2022

Uh oh!

nicolas-raoul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-raoul commented Mar 22, 2022

Uh oh!

Ayan-10 commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Ayan-10 commented Dec 10, 2021 •

edited by nicolas-raoul

Loading

codecov bot commented Dec 10, 2021 •

edited

Loading

nicolas-raoul left a comment •

edited

Loading

nicolas-raoul commented Dec 30, 2021 •

edited

Loading