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

Issue 5835 nearby #5843

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Conversation

tristan81
Copy link
Contributor

Fixes #5835

Fix for reported bug: Nearby map does not show new pins when moving map a little, especially noticeable when zoomed in

What changes did you make and why?

Modified NearbyParentFragment.java:

  • Onscroll - removed distance threshold that was requiring the user to scroll a distance of at least 2000m before a search was initiated. Added logic to delay search by 800ms when OnScroll is fired, mulitple scrolls within 800ms are debounced and only most recent is actioned.

  • OnDestroy - destroy any queued OnScroll initiated searches that could be waiting to run when the OnDestroy event is fired.

Tests performed (required)

  • Tested main app on virtual device API Level 35

  • Ran all automatic tests, same results as unmodified code base.

  • Tested Nearby screen on virtual device:
    -- multiple scrolls within 800ms threshold
    -- scroll and wait 800ms for search initiation
    --make successive scrolls within 10 seconds

Test notes:

  • Experienced occassional HTTP 500 error but it is coming from the unchanged UpdateMarkers()

Screenshots (for UI changes only)

  • no UI changes included

OnScroll - removed distance threshold, delay search by 800ms, discard multiple OnScroll within 800ms.
OnDestroy - destroy any queued OnScroll events
loadPlacesDataAsync - set batchSize from 50 back to original 3
@nicolas-raoul
Copy link
Member

Testing now. :-)

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Working great and the code looks good. Thanks a lot Tristan!

@nicolas-raoul nicolas-raoul merged commit d0e64d7 into commons-app:main Oct 2, 2024
1 check passed
@nicolas-raoul
Copy link
Member

Ah, just after merging I notice this issue, pins disappearing/reappearing many times at the beginning of the video, after moving the map once:

screen-20241002-141556.mp4

Any idea what it might be?

@nicolas-raoul
Copy link
Member

screen-20241002-142150.mp4

@tristan81
Copy link
Contributor Author

It wasn't noticeable in my area but I can see it now when I move my position to the nearest city centre where the place markers are dense.

I think I know what is causing it, sub methods that load the places (i.e. populatePlacesForCurrentLocation()) are spawning processes. Forcing them to stop when there is a new OnScroll can leave the UI in an inconsistent state because they are controlling UI features like the progress bar, locking/unlocking the map.

The places are updating in blocks of three, I assume this is so the UI isn't locked for the duration of populating the places which in my closest city can take 10 mins for all the places to load. Instead of querying for all the places in the viewable map area in a single HTTP request it is running many HTTP for each place/attraction which is visible in logcat.

I will need to work out how to get methods like populatePlacesForCurrentLocation to gracefully terminate if stopped before completion.

Should we revert the merge?

@nicolas-raoul
Copy link
Member

Better not revert the merge, as it is a clear improvement. :-)
If you have time to send another pull request building upon it, that would be wonderful.

@tristan81
Copy link
Contributor Author

okay I will see if I can improve it.

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.

Nearby map does not show new pins when moving map a little, especially noticeable when zoomed in
2 participants