Skip to content

Fixes #2001 New main UI - progress bar in Nearby card view sometimes runs forever. #2021

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

Conversation

neslihanturan
Copy link
Collaborator

Description (required)

Fixes #2001 New main UI - progress bar in Nearby card view sometimes runs forever.

Nearby card notification is only visible if closest nearby item is loaded successfully.

Tests performed (required)

Tested on API 26, prodDebug and betaDebug.

translatewiki and others added 12 commits November 8, 2018 09:54
* Bug fix issue commons-app#1826
Changes made :
-Certain category names used to show suffixed with strings prefixed with pipe '|'. Removed everything after the pipe. As per the discussion on the thread, its safe to remove everything after the pipe, including the pipe

* review suggested changes
*Code formatting
*Extracted out the index of pipe in a variable
*Added issue link in comments
* Remove libraries section from README

* Add wiki link to "libraries used" to README
@codecov-io
Copy link

codecov-io commented Nov 22, 2018

Codecov Report

Merging #2021 into 2.9-release will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff               @@
##           2.9-release   #2021      +/-   ##
==============================================
- Coverage         4.13%   4.09%   -0.05%     
==============================================
  Files              223     223              
  Lines            11227   11243      +16     
  Branches          1032    1030       -2     
==============================================
- Hits               464     460       -4     
- Misses           10728   10749      +21     
+ Partials            35      34       -1
Impacted Files Coverage Δ
...r/free/nrw/commons/contributions/MainActivity.java 0% <0%> (ø) ⬆️
...ain/java/fr/free/nrw/commons/utils/DialogUtil.java 0% <0%> (ø) ⬆️
...w/commons/contributions/ContributionsFragment.java 0% <0%> (ø) ⬆️
.../nrw/commons/nearby/NearbyNoificationCardView.java 0% <0%> (ø) ⬆️
...s/ui/LongTitlePreferences/LongTitlePreference.java 18.18% <0%> (-36.37%) ⬇️
...ava/fr/free/nrw/commons/upload/UploadActivity.java 0% <0%> (ø) ⬆️
...fr/free/nrw/commons/media/MediaDetailFragment.java 0% <0%> (ø) ⬆️
.../fr/free/nrw/commons/nearby/NearbyMapFragment.java 0% <0%> (ø) ⬆️

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 ea37245...8c9dac8. Read the comment docs.

}

private void displayYouWontSeeNearbyMessage() {
ViewUtil.showLongToast(getActivity(), "You wont see narby place since you didn't gave permission");
Copy link
Member

Choose a reason for hiding this comment

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

Please make this a string resource and change it to "Unable to display nearest place that needs pictures without location permissions".

@@ -414,5 +414,7 @@ Upload your first media by touching the camera or gallery icon above.</string>
<string name="no_go_back">No, Go Back</string>

<string name="upload_flow_all_images_in_set">(For all images in set)</string>
<string name="nearby_card_permission_title">Permission Request</string>
<string name="nearby_card_permission_explanation">Please give location permission to see the nearest place that needs pictures</string>
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to "Would you like us to use your current location to display the nearest place that needs pictures?"

@misaochan
Copy link
Member

@neslihanturan I just tested your PR on a Samsung Galaxy S7 running API 26. The general flow works well if the user agrees to all permissions, and the loading works well also. :) Some changes are needed however:

  • Please rebase on 2.9-release so we don't have changes from master
  • Change strings as mentioned in comments
  • There is an issue that needs fixing - when I select 'no' at the initial "Permission Request" dialog, I do not ever get to the Android-generated permissions request, and therefore there is never an option to say "don't ask again". This means that every time I load Home, this dialog pops up again. There are a few possible ways of fixing it - perhaps we can have a "never ask again" option in the initial dialog, and set a flag that we then check before launching that dialog?

@neslihanturan
Copy link
Collaborator Author

Ready to be tested @misaochan :)

@misaochan
Copy link
Member

@neslihanturan It works now and does not spam the user. Great job! :)

I will go ahead and merge this, but there is one minor scenario that would be good if we can fix in 2.9.1. If I deny permissions in the dialog, then 'reset' it by enabling location permissions via Android settings and disabling again, instead of displaying the appropriate dialog, it instead displays the dialog "Permission required to display a list of nearby places". Even though I'm on Contributions tab.

However, that is a minor issue (I doubt too many users will be doing that), so it need not block the release. :)

@misaochan misaochan merged commit d62a758 into commons-app:2.9-release Nov 25, 2018
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.

6 participants