Skip to content

Search this area #2051

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

Merged
merged 27 commits into from
Dec 5, 2018
Merged

Search this area #2051

merged 27 commits into from
Dec 5, 2018

Conversation

neslihanturan
Copy link
Collaborator

Description (required)

Fixes #591 "Search this area" to launch a nearby search around that location

What changes did you make and why?

  • When you moved on the map a button "Search this area" appears on top of the map
  • If you don't click on the button, and come back your previous position, button disappears
  • If you click to the button, a progress bar will appear on the map, and map will be locked, you can not move on the map during fetching places
  • Only your location marker and your new search results will be visible on map and list
  • If you move, map won't follow you if search this area mode on
  • If you click to extend list, map camera won't change its target
  • When you click on recenter me button, or go back to previous position on map with your fingers, nearby places around you will be uploaded
  • Map will follow you as always
  • Extending list will change map camera angle, so that your location will still be visible

Tests performed (required)
Tested betaDebug, on API19 and 26
Screenshots showing what changed (optional - for UI changes)

image

…t follow users location with camera, if search this area mode is on
@codecov-io
Copy link

codecov-io commented Nov 29, 2018

Codecov Report

Merging #2051 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2051      +/-   ##
=========================================
- Coverage    4.05%   4.02%   -0.04%     
=========================================
  Files         225     226       +1     
  Lines       11357   11467     +110     
  Branches     1048    1064      +16     
=========================================
+ Hits          460     461       +1     
- Misses      10863   10972     +109     
  Partials       34      34
Impacted Files Coverage Δ
...a/fr/free/nrw/commons/nearby/NearbyController.java 0% <0%> (ø) ⬆️
...ava/fr/free/nrw/commons/nearby/NearbyFragment.java 0% <0%> (ø) ⬆️
.../java/fr/free/nrw/commons/utils/LocationUtils.java 0% <0%> (ø)
.../java/fr/free/nrw/commons/nearby/NearbyPlaces.java 0% <0%> (ø) ⬆️
...w/commons/contributions/ContributionsFragment.java 0% <0%> (ø) ⬆️
...fr/free/nrw/commons/nearby/NearbyListFragment.java 0% <0%> (ø) ⬆️
.../fr/free/nrw/commons/nearby/NearbyMapFragment.java 0% <0%> (ø) ⬆️
...n/java/fr/free/nrw/commons/auth/LoginActivity.java 0% <0%> (ø) ⬆️
.../java/fr/free/nrw/commons/auth/SessionManager.java 16.17% <0%> (+0.3%) ⬆️

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 c2addd9...9aaf270. Read the comment docs.

@misaochan misaochan self-requested a review November 30, 2018 09:10
@@ -312,6 +319,33 @@ private void refreshView(LocationServiceManager.LocationChangeType locationChang
}
}

public void refreshViewForCustomLocation(LatLng customLatLng) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we get Javadocs for these new methods, please?

* on the gap between list sheet and tab layout.
* @param isBottomListSheetExpanded
*/
private void updateMapCameraAccordingToBottomSheet(boolean isBottomListSheetExpanded) {
Copy link
Member

Choose a reason for hiding this comment

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

Codacy reports that this method is unused, could you double check? "Avoid unused private methods such as 'updateMapCameraAccordingToBottomSheet(boolean)'."

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes actually this method got obsolete after fix for #2018 .Although it was a doing a good job adjusting the job based on some calculations for adjusting map zoom level based on bottom sheet's state, but yes can be removed now.

@misaochan
Copy link
Member

misaochan commented Dec 3, 2018

@neslihanturan I just tested this on an API 26 Samsung Galaxy s7. "Search this area" works very well for me in general, great job! I like that it maintains the zoom level, too.

A couple of issues:

  • When I searched for a location 100km away, map works fine, list displays the correct names BUT the distances displayed on Nearby list are calculated based on the search point, not based on my location. So for instance it says "Toormina" is 6.8km away from me even though it is hundreds of km away.
  • Whenever I center the map on my location, it automatically searches the location even though I didn't choose to search. It also seems to assume that I always want the max number of results, regardless of my zoom level. So what happens is that whenever the user taps the center button, a progress bar automatically starts running for about a minute (on my VDSL, will take longer on slower connections), and displays A LOT of pins that the user might not necessarily want. Screenshot below:

screenshot_20181203-191525_commons

Solution: aside from the initial load, I do not think that the user's position should be treated as special. Meaning that when I load Nearby, then yes, automatically search around my location and display a few pins (not 600!). if I search 100km away, then tap the center button, do not initiate a search around my location automatically. Rather, display the "Search this area" button as normal.

@neslihanturan
Copy link
Collaborator Author

Whenever I center the map on my location, it automatically searches the location even though I didn't choose to search. It also seems to assume that I always want the max number of results, regardless of my zoom level. So what happens is that whenever the user taps the center button, a progress bar automatically starts running for about a minute (on my VDSL, will take longer on slower connections), and displays A LOT of pins that the user might not necessarily want. Screenshot below:

Actually loading points for your real location when you come back wasn't a bug, it was a feature:D I thought both recenter button and manual recentering should load real nearby pins automatically.

But we can switch to manual version as you suggested.

When it comes to pin numbers and distances, You are right sorry I will take care

@neslihanturan
Copy link
Collaborator Author

  • Enormous number of pin problem
  • Autorefresh problem
  • Distances
  • Forgotten javadocs

Are fixed, but since I changed a lot this should be tested carefully. Possible test cases:

  • Move while map is on
  • Go out of nearby borders and see if map is updated nearby pins

@neslihanturan
Copy link
Collaborator Author

Ready to be tested @misaochan :)

@misaochan
Copy link
Member

This works well for me, thanks! :) The only issue now is that I found that rotating the screen makes the map auto-refresh, and then it NEVER loads. Not even when rotating it back to portrait, or after going to Contributions and back to Nearby.

I will merge this PR first, please fix this in your next PR. Logs below:

12-06 03:01:28.282 19338-21801/fr.free.nrw.commons D/NearbyPlaces: 33 results at radius: 2.617924
12-06 03:01:28.300 19338-21801/fr.free.nrw.commons D/NearbyPlaces: https://query.wikidata.org/#SELECT%0A%20%20%20%20%20(SAMPLE(%3Flocation)%20as%20%3Flocation)%0A%20%20%20%20%20%3Fitem%0A%20%20%20%20%20(SAMPLE(COALESCE(%3Fitem_label_preferred_language%2C%20%3Fitem_label_any_language))%20as%20%3Flabel)%0A%20%20%20%20%20(SAMPLE(%3FclassId)%20as%20%3Fclass)%0A%20%20%20%20%20(SAMPLE(COALESCE(%3Fclass_label_preferred_language%2C%20%3Fclass_label_any_language%2C%20%22%3F%22))%20as%20%3Fclass_label)%0A%20%20%20%20%20(SAMPLE(COALESCE(%3Ficon0%2C%20%3Ficon1))%20as%20%3Ficon)%0A%20%20%20%20%20(SAMPLE(COALESCE(%3Femoji0%2C%20%3Femoji1))%20as%20%3Femoji)%0A%20%20%20%20%20%3FwikipediaArticle%0A%20%20%20%20%20%3FcommonsArticle%0A%20%20%20%20%20(SAMPLE(%3FCommons_category)%20as%20%3FCommons_category)%0A%20%20%20WHERE%20%7B%0A%20%20%20%20%20%23%20Around%20given%20location...%0A%20%20%20%20%20SERVICE%20wikibase%3Aaround%20%7B%0A%20%20%20%20%20%20%20%3Fitem%20wdt%3AP625%20%3Flocation.%0A%20%20%20%20%20%20%20bd%3AserviceParam%20wikibase%3Acenter%20%22Point(153.0408%20-27.4531)%22%5E%5Egeo%3AwktLiteral.%0A%20%20%20%20%20%20%20bd%3AserviceParam%20wikibase%3Aradius%20%224.24%22%20.%20%23%20Radius%20in%20kilometers.%0A%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%23%20...%20and%20without%20an%20image.%0A%20%20%20%20%20MINUS%20%7B%3Fitem%20wdt%3AP18%20%5B%5D%7D%0A%0A%20%20%20%20%20%23%20Get%20the%20label%20in%20the%20preferred%20language%20of%20the%20user%2C%20or%20any%20other%20language%20if%20no%20label%20is%20available%20in%20that%20language.%0A%20%20%20%20%20OPTIONAL%20%7B%3Fitem%20rdfs%3Alabel%20%3Fitem_label_preferred_language.%20FILTER%20(lang(%3Fitem_label_preferred_language)%20%3D%20%22en%22)%7D%0A%20%20%20%20%20OPTIONAL%20%7B%3Fitem%20rdfs%3Alabel%20%3Fitem_label_any_language%7D%0A%0A%20%20%20%20%20%23%20Get%20Commons%20category%20(P373)%0A%20%20%20%20%20OPTIONAL%20%7B%20%3Fitem%20wdt%3AP373%20%3FCommons_category.%20%7D%0A%0A%20%20%20%20%20%23%20Get%20the%20class%20label%20in%20the%20preferred%20language%20of%20the%20user%2C%20or%20any%20other%20language%20if%20no%20label%20is%20available%20in%20that%20language.%0A%20%20%20%20%20OPTIONAL%20%7B%0A%20%20%20%20%20%20%20%3Fitem%20p%3AP31%2Fps%3AP31%20%3FclassId.%0A%20%20%20%20%20%20%20OPTIONAL%20%7B%3FclassId%20rdfs%3Alabel%20%3Fclass_label_preferred_language.%20FILTER%20(lang(%3Fclass_label_preferred_language)%20%3D%20%22en%22)%7D%0A%20%20%20%20%20%20%20OPTIONAL%20%7B%3FclassId%20rdfs%3Alabel%20%3Fclass_label_any_language%7D%0A%0A%20%20%20%20%20%20%20%23%20Get%20icon%0A%20%20%20%20%20%20%20OPTIONAL%20%7B%20%3FclassId%20wdt%3AP2910%20%3Ficon0.%20%7D%0A%20%20%20%20%20%20%20OPTIONAL%20%7B%20%3FclassId%20wdt%3AP279*%2Fwdt%3AP2910%20%3Ficon1.%20%7D%0A%20%20%20%20%20%20%20%23%20Get%20emoji%0A%20%20%20%20%20%20%20OPTIONAL%20%7B%20%3FclassId%20wdt%3AP487%20%3Femoji0.%20%7D%0A%20%20%20%20%20%20%20OPTIONAL%20%7B%20%3FclassId%20wdt%3AP279*%2Fwdt%3AP487%20%3Femoji1.%20%7D%0A%0A%20%20%20%20%20%20%20OPTIONAL%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%3FwikipediaArticle%20%20%20schema%3Aabout%20%3Fitem%20%3B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20schema%3AisPartOf%20%3Chttps%3A%2F%2Fen.wikipedia.org%2F%3E%20.%0A%20%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%20%20%20OPTIONAL%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%3FwikipediaArticle%20%20%20schema%3Aabout%20%3Fitem%20%3B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20schema%3AisPartOf%20%3Chttps%3A%2F%2Fen.wikipedia.org%2F%3E%20.%0A%20%20%20%20%20%20%20%20%20%20%20SERVICE%20wikibase%3Alabel%20%7B%20bd%3AserviceParam%20wikibase%3Alanguage%20%22en%22%20%7D%0A%20%20%20%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20%20%20%20OPTIONAL%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%3FcommonsArticle%20%20%20schema%3Aabout%20%3Fitem%20%3B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20schema%3AisPartOf%20%3Chttps%3A%2F%2Fcommons.wikimedia.org%2F%3E%20.%0A%20%20%20%20%20%20%20%20%20%20%20SERVICE%20
    wikibase%3Alabel%20%7B%20bd%3AserviceParam%20wikibase%3Alanguage%20%22en%22%20%7D%0A%20%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%20%7D%0A%20%20%20%7D%0A%20%20%20GROUP%20BY%20%3Fitem%20%3FwikipediaArticle%20%3FcommonsArticle%0A
12-06 03:01:28.307 19338-21801/fr.free.nrw.commons I/System.out: (HTTPLog)-Static: isSBSettingEnabled false
    (HTTPLog)-Static: isSBSettingEnabled false
12-06 03:01:28.423 19338-21804/fr.free.nrw.commons D/NearbyPlaces: 131 results at radius: 4.235801
12-06 03:01:28.423 19338-21804/fr.free.nrw.commons D/NearbyController: Sorting places by distance...
12-06 03:01:28.425 19338-19338/fr.free.nrw.commons D/NearbyFragment: Populating nearby places
12-06 03:01:28.464 19338-19338/fr.free.nrw.commons D/NearbyFragment: Map fragment already exists, just update the map and list
12-06 03:01:28.944 19338-21796/fr.free.nrw.commons D/NearbyPlaces: Reading from query result...
12-06 03:01:29.694 19338-21796/fr.free.nrw.commons D/NearbyPlaces: 800 results at radius: 4.235801
12-06 03:01:29.694 19338-21796/fr.free.nrw.commons D/NearbyController: Sorting places by distance...
12-06 03:01:29.698 19338-19338/fr.free.nrw.commons D/NearbyFragment: Populating nearby places
12-06 03:01:29.799 19338-19338/fr.free.nrw.commons D/NearbyFragment: Map fragment already exists, just update the map and list
12-06 03:01:33.146 19338-21799/fr.free.nrw.commons D/NearbyPlaces: Reading from query result...
12-06 03:01:34.415 19338-21799/fr.free.nrw.commons D/NearbyPlaces: 559 results at radius: 4.235801
12-06 03:01:34.415 19338-21799/fr.free.nrw.commons D/NearbyController: Sorting places by distance...
12-06 03:01:34.424 19338-19338/fr.free.nrw.commons D/NearbyFragment: Populating nearby places
12-06 03:01:34.506 19338-19338/fr.free.nrw.commons D/NearbyFragment: Map fragment already exists, just update the map and list
12-06 03:01:38.163 19338-21801/fr.free.nrw.commons D/NearbyPlaces: Reading from query result...
12-06 03:01:38.549 19338-21801/fr.free.nrw.commons D/NearbyPlaces: 275 results at radius: 4.235801
12-06 03:01:38.549 19338-21801/fr.free.nrw.commons D/NearbyController: Sorting places by distance...
12-06 03:01:38.551 19338-19338/fr.free.nrw.commons D/NearbyFragment: Populating nearby places
12-06 03:01:38.615 19338-19338/fr.free.nrw.commons D/NearbyFragment: Map fragment already exists, just update the map and list

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.

4 participants