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

Fixes #4255 - Media details activity shows depictions in random languages, rather than in the user's locale #4275

Merged
merged 3 commits into from Mar 1, 2021

Conversation

Pratham2305
Copy link
Contributor

@Pratham2305 Pratham2305 commented Feb 24, 2021

Description (required)

Fixes #4255

What changes did you make and why?

Since App fetches a list of a depiction in different languages but I have noticed that depiction is only shown in the first language given in list, which is causing this issue. I have added the conditions Now it will first check if the depiction caption is available in the user's locale and if not then it will also check for English as it is the default language, else show in any available language.

Tests performed (required)
Tested prodDebug on Pixel 3 with API level 29.

(image selected from the feature tab with depictions)
https://commons.m.wikimedia.org/wiki/File%3AD%C3%BClmen%2C_Merfeld%2C_Feldweg_am_M%C3%BChlenbach_--_2021_--_4278-80.jpg

default language: English

Latest Master 3.0.0-debug

Screenshot_1614252218

Updated

Screenshot_1614251879

@misaochan
Copy link
Member

@Pratham2305 could you post a screenshot of the fixed media details view, please? Thanks!

@Pratham2305
Copy link
Contributor Author

@misaochan Yes, Posted!

for (IdAndCaptions idAndCaption : idAndCaptions) {
depictionContainer.addView(buildDepictLabel(
idAndCaption.getCaptions().values().iterator().next(),
//Check if the Depiction Caption is available in user's locale if not then check for english, else show any available.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally love ternary operations but using 2 levels of ternary makes it hard to read like a very long sentence. I am good to merge this PR after you turned them to use if braces. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@codecov-io
Copy link

Codecov Report

Merging #4275 (23e2784) into master (d6dcd43) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4275      +/-   ##
============================================
- Coverage     10.40%   10.37%   -0.04%     
  Complexity      474      474              
============================================
  Files           343      343              
  Lines         13069    13113      +44     
  Branches       1052     1060       +8     
============================================
  Hits           1360     1360              
- Misses        11641    11684      +43     
- Partials         68       69       +1     
Impacted Files Coverage Δ Complexity Δ
...fr/free/nrw/commons/media/MediaDetailFragment.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...mons/contributions/ContributionBoundaryCallback.kt 95.45% <0.00%> (-4.55%) 6.00% <0.00%> (ø%)
...ava/fr/free/nrw/commons/utils/PermissionUtils.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...va/fr/free/nrw/commons/campaigns/CampaignView.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...w/commons/contributions/ContributionsFragment.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...commons/nearby/fragments/NearbyParentFragment.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)

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 d6dcd43...23e2784. Read the comment docs.

@misaochan
Copy link
Member

Wonderful, thank you @Pratham2305 ! Merging since @neslihanturan 's review has been addressed. :)

@misaochan misaochan merged commit 93cd89a into commons-app:master Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media details activity shows depictions in random languages, rather than in the user's locale
4 participants