Made Split to Nearby Query into a fast query for coordinates + a details query for each pin #5731

kanahia1 · 2024-05-14T15:11:25Z

Description (required)

Fixes #4560

What changes did you make and why?

Splitted to the query into two queries
Improved the pop up for the marker which will probably fix Nearby: Pins overlapping place name #5692
Nearby: Load pins as I move the screen, cache pins, remove "search this area" button #5664

Tests performed (required)

Tested prodDebug on Samsung S21 FE with API level 33.

Screenshots (for UI changes only)

WhatsApp.Video.2024-05-14.at.20.36.17_5a90b1f7.mp4

nicolas-raoul · 2024-05-15T01:59:29Z

Testing now.

Just wondering: Is there a specific reason why your branch is called kanahia1-main?
It is usually good to have a branch name that reflects the issue ID and an extremely short summary of the issue.
It makes it easier to work on different things in parallel. :-)

kanahia1 · 2024-05-15T04:56:44Z

There's no specific reason for it 😄 From next time I will try to follow proper naming.

nicolas-raoul

Would you mind implementing a loop that asynchrously loads each item's details, for all shown points? Depending on the user's settings it will make many points disappear, which is not ideal user experience but it is the best we can do while having fast nearby maps in areas with many items.

Thanks! 🙂

app/src/main/resources/queries/rectangle_query_for_nearby.rq

kanahia1 · 2024-05-15T07:37:47Z

Sure, I will do asynchronous loop to load pin's details

kanahia1 · 2024-05-15T15:46:56Z

Hey @nicolas-raoul, I'm bit confused here.

Should the queries look like given below?

Run query for loading only pins (coordinates)
Run query to load all the data about all the pins async, which may also remove the pins from screen (This query will be similar to what we have already)
In case, user clicked over the pin and data has not been loaded (from 2.) then run query to load data for that particular pin as it is much faster.

nicolas-raoul · 2024-05-15T23:48:55Z

Sounds good! :-)

For the second query, you can specify the QIDs of the pins, that will make the query much faster.
Here is a example of how to specify QIDs: https://www.wikidata.org/wiki/User:Vojt%C4%9Bch_Dost%C3%A1l/app-Czech_uploads
I believe this is the relevant documentation: https://www.w3.org/TR/sparql11-query/#inline-data
Obviously, the second query does not need to care about coordinates.
Let me know if anything is unclear. :-)

nicolas-raoul · 2024-05-16T00:11:27Z

By the way, I believe it would be user-friendly to first show pins in grey color to mean "unfiltered". Then upon receiving the result from the second request, hide the pins that do not match the filter, and set the normal color to the pins that match the filter. What do you think? 🙂

kanahia1 · 2024-05-16T03:47:59Z

Yeah, That would be great!

kanahia1 · 2024-05-16T09:52:46Z

Hello @nicolas-raoul, I have improvised query as per suggestions by you and it's working fine for the smaller regions

WhatsApp.Video.2024-05-16.at.15.15.29_37c183a9.mp4

But I have faced an issue because we are using List of QID (which becomes really large for city like capital of India) the server returns error 414 which is because our url length exceeds a limit (which is because of the large query)

WhatsApp.Video.2024-05-16.at.15.16.45_392342b7.mp4

How about replacing the List of QID with similar method we had earlier?

nicolas-raoul · 2024-05-16T11:02:23Z

Would you mind splitting the second query into several queries of 50 QIDs max, and using the results as they come? Ideally sort them by their distance to the center of the search rectangle, then run the queries starting from the nearest 50 QIDs.

Sorry to add something again, but when a not-yet-loaded QID is tapped by the user, it would be great to run the second query for that point, in parallel with the above, it will probably return the result fast and the user will see the label/class/filter status/etc without having to wait for all points to load.

kanahia1 · 2024-05-16T11:15:55Z

Thank you for suggestion and I feel it will make queries even better.

And I have added that when pins are not loaded still the user can get the details for a pin.

Screen_Recording_20240516_164159_Commons.mp4

kanahia1 · 2024-05-17T12:09:34Z

Hello Nicolas, I have divided the second query into batches fixing the issue we were facing due to large size of second query. Now the first query has became more effective.

VID-20240517-WA0007.mp4

nicolas-raoul

It is getting better and better, keep up the great work :-)
Brilliant work on the SPARQL query especially!

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

app/src/main/java/fr/free/nrw/commons/mwapi/OkHttpJsonApiClient.java

app/src/main/java/fr/free/nrw/commons/nearby/NearbyPlaces.java

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

kanahia1 · 2024-05-18T05:47:08Z

Hello Nicolas, Thanks for the suggestions and now loading of pins has became even more efficient.

WhatsApp.Video.2024-05-18.at.11.13.21_560bd12d.mp4

nicolas-raoul · 2024-05-18T07:10:56Z

Looks fantastic! I will try it as soon as I get home. 🙂

kanahia1 · 2024-05-19T04:34:39Z

Hi @nicolas-raoul, on digging I am able to find that issue was caused by ongoing network call which was not stopped on starting new call. Now I have updated the code, I hope it should fix up the issue.

WhatsApp.Video.2024-05-19.at.10.03.18_d70e1a6f.mp4

nicolas-raoul · 2024-05-19T13:31:35Z

I am experiencing several unexpected things, especially when swgitching to other apps or when the screen goes black:

screen-20240519-213301.mp4

Also, maybe modifying the default zoom level would be good (zoomed more, mening it loads less items by default).

kanahia1 · 2024-05-19T14:09:16Z

Sure, I will update the zoom level ;-)

I tried switching apps. It moves the map to the default location

VID-20240519-WA0003.mp4

Actually It wasn't clear from the video provided. On switching apps the map return to the my location (in my case)

nicolas-raoul · 2024-05-29T01:25:16Z

Let's say the map is fully loaded, including points details (exist or not, etc). Then when I turn the screen off and on again, all points on the map turned back to grey again, and it starts sending requests again 3-by-3. It should not reload unless it is kicked ou of memory (this happens on a device with lots of memory and no other running app so memory is not the issue).
I send a screencast on Zulip.

kanahia1 · 2024-05-29T06:59:56Z

Hey Nicolas, I have fixed the unnecessary reloads caused onResume.

WhatsApp.Video.2024-05-29.at.12.21.09_a7068d1b.mp4

We may also need to implement method to load pending pins which are not loaded on changing the apps as it returns <-- HTTP FAILED: java.io.InterruptedIOException, I am working over it and will also share a commit for it ASAP

kanahia1 · 2024-07-10T13:02:33Z

Hi @nicolas-raoul, I have fixed the bug in 2251c62

WhatsApp.Video.2024-07-10.at.18.31.15_d1eac0ec.mp4

app/src/main/java/fr/free/nrw/commons/nearby/PlaceDao.java

app/src/main/java/fr/free/nrw/commons/nearby/PlacesLocalDataSource.java

app/src/main/java/fr/free/nrw/commons/nearby/PlacesRepository.java

nicolas-raoul · 2024-07-23T06:56:37Z

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

+
+    private List<Place> updatedPlaceList;
+    private LatLng updatedLatLng;
+    private boolean searchble;


nicolas-raoul · 2024-07-23T06:57:37Z

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

-                locationPermissionGranted();
+                if (lastFocusLocation == null && lastKnownLocation == null) {
+                    locationPermissionGranted();
+                }else if (updatedPlaceList != null){


minor: space between } and else

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

nicolas-raoul · 2024-07-28T03:59:13Z

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

+            });
+    }
+
+    private void savePlaceToDB(Place place) {


javadoc.
In Java we ususally use full words: savePlaceToDatabase

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

app/src/main/java/fr/free/nrw/commons/nearby/presenter/NearbyParentFragmentPresenter.java

nicolas-raoul

When I load Nearby for the first time, it loads a much wider rectangle than what is seen on screen.
Then when I move the map and let the pins refresh, that time only pins visible on screen are loaded.

Any idea what makes it load too many pins the first time? It makes the initial query longer and makes many unneeded queries to the server.

kanahia1 · 2024-07-28T08:34:06Z

When I load Nearby for the first time, it loads a much wider rectangle than what is seen on screen. Then when I move the map and let the pins refresh, that time only pins visible on screen are loaded.

Any idea what makes it load too many pins the first time? It makes the initial query longer and makes many unneeded queries to the server.

Fixed this issue in 6fed9f4

kanahia1 · 2024-07-28T17:13:39Z

Hello @nicolas-raoul, I have added jcenter in 4f7cbfc which seems to be fix for https://github.com/commons-app/apps-android-commons/actions/runs/10131832421/job/28015018616?pr=5731

Could not resolve all files for configuration ':app:betaDebugCompileClasspath'.
Could not find com.dinuscxj:circleprogressbar:1.1.1.

as this library is hosted on jcenter https://mvnrepository.com/artifact/com.dinuscxj/circleprogressbar?repo=jcenter

nicolas-raoul · 2024-07-30T01:25:37Z

Maybe not a big issue, but I see the pin at the top appearing and disappearing:

PXL_20240730_012259644.mp4

sorry for the bad video quality

kanahia1 · 2024-07-30T18:04:02Z

Maybe not a big issue, but I see the pin at the top appearing and disappearing:

PXL_20240730_012259644.mp4
sorry for the bad video quality

If possible can you please share the location over zulip (for testing)

nicolas-raoul

Good job, you are quickly approaching mergeability for this pull request :-)

nicolas-raoul · 2024-07-31T23:52:00Z

app/src/main/java/fr/free/nrw/commons/bookmarks/locations/BookmarkLocationsDao.java

@@ -204,6 +206,7 @@ public static class Table {
        static final String COLUMN_COMMONS_LINK = "location_commons_link";
        static final String COLUMN_PIC = "location_pic";
        static final String COLUMN_EXISTS = "location_exists";
+        static final String COLUMN_ENTITY_ID = "location_entity_id";


Could you please add a comment explaining why this line is needed? And whether it can be empty, and an example value. Thanks!

app/src/main/java/fr/free/nrw/commons/nearby/Place.java

app/src/main/java/fr/free/nrw/commons/nearby/PlaceDao.java

nicolas-raoul · 2024-07-31T23:55:34Z

app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java

    private PlaceAdapter adapter;
    private GeoPoint lastMapFocus;
    private NearbyParentFragmentInstanceReadyCallback nearbyParentFragmentInstanceReadyCallback;
    private boolean isAdvancedQueryFragmentVisible = false;
    private Place nearestPlace;
+    private volatile boolean stopQuery;
+
+    private List<Place> updatedPlaceList;


updatedPlaceList -> updatedPlacesList

Thanks for reviewing, I have made all suggested changes in the newer commit.

nicolas-raoul · 2024-08-03T13:36:04Z

I tested more there are still a few bugs but soon mergeable I think. 🙂

The biggest problem: It seems like the first time (for instance after installing the app) Nearby pins do not load, until I switch to home activity then come back:

screen-20240803-222510.mp4

kanahia1 · 2024-08-03T15:42:16Z

I have shared a newer commit fixing this bug. Can you please test it.

WhatsApp.Video.2024-08-03.at.21.09.22_36e5bf8f.mp4

nicolas-raoul · 2024-08-04T00:34:32Z

Fantastic, thanks Kanahia for this pull request, it is a great improvement!
I will merge for now, and submit anything else as new GitHub issues. 🙂

Splitted the query

8be9deb

nicolas-raoul reviewed May 15, 2024

View reviewed changes

app/src/main/resources/queries/rectangle_query_for_nearby.rq Outdated Show resolved Hide resolved

app/src/main/resources/queries/rectangle_query_for_nearby.rq Outdated Show resolved Hide resolved

Made changes to the query

3e9c3a6

Improvised query

8e343f2

kanahia1 added 2 commits May 17, 2024 17:29

Improvised query by dividing in the batches

015772c

Fixed failing tests

4da6ed9

nicolas-raoul requested changes May 17, 2024

View reviewed changes

kanahia1 added 2 commits May 18, 2024 11:03

Improved batches

296eb5b

Improved sorting

49fd84b

Fixes issue caused by search this area button

c1e16b3

Fixed failing tests

8315777

Fixed unnecessary reloads on onResume

13a390a

kanahia1 added 3 commits July 8, 2024 21:42

Fixed unit test

97019c9

Fixed unit test

5caa0bc

Fixed the bug

2251c62

Merge branch 'main' into main

67a2eb4

nicolas-raoul requested changes Jul 28, 2024

View reviewed changes

kanahia1 and others added 8 commits July 28, 2024 14:23

Fixed issue with pin loading on the first launch

6fed9f4

Merge remote-tracking branch 'origin/main'

a51affe

Merge branch 'commons-app:main' into main

4985462

Updated javadocs

efbea39

Merge remote-tracking branch 'origin/main'

20810b9

Temporary commit - only for testing

3fcfb20

Replaced Temporary commit

6d87282

Temporary commit - Added jcenter

4f7cbfc

nicolas-raoul requested changes Aug 1, 2024

View reviewed changes

kanahia1 and others added 4 commits August 1, 2024 11:08

Made minor changes

fb80b3a

Fixed unit tests

7c79e46

Fixed unit tests

6258163

Merge branch 'main' into main

5686135

kanahia1 added 2 commits August 3, 2024 21:10

Fixed minor bug

b1c98b0

Merge remote-tracking branch 'origin/main'

6d64533

nicolas-raoul merged commit 2d63f35 into commons-app:main Aug 4, 2024
1 check passed

Made Split to Nearby Query into a fast query for coordinates + a details query for each pin #5731

Made Split to Nearby Query into a fast query for coordinates + a details query for each pin #5731

Conversation

kanahia1 commented May 14, 2024 • edited Loading

nicolas-raoul commented May 15, 2024

kanahia1 commented May 15, 2024

nicolas-raoul left a comment

Choose a reason for hiding this comment

kanahia1 commented May 15, 2024

kanahia1 commented May 15, 2024

nicolas-raoul commented May 15, 2024

nicolas-raoul commented May 16, 2024

kanahia1 commented May 16, 2024

kanahia1 commented May 16, 2024

nicolas-raoul commented May 16, 2024

kanahia1 commented May 16, 2024

kanahia1 commented May 17, 2024 • edited Loading

nicolas-raoul left a comment

Choose a reason for hiding this comment

kanahia1 commented May 18, 2024

nicolas-raoul commented May 18, 2024

kanahia1 commented May 19, 2024

nicolas-raoul commented May 19, 2024

kanahia1 commented May 19, 2024

nicolas-raoul commented May 29, 2024

kanahia1 commented May 29, 2024

kanahia1 commented Jul 10, 2024

nicolas-raoul Jul 23, 2024

Choose a reason for hiding this comment

nicolas-raoul Jul 23, 2024

Choose a reason for hiding this comment

nicolas-raoul Jul 28, 2024

Choose a reason for hiding this comment

nicolas-raoul left a comment

Choose a reason for hiding this comment

kanahia1 commented Jul 28, 2024 • edited Loading

kanahia1 commented Jul 28, 2024

nicolas-raoul commented Jul 30, 2024

kanahia1 commented Jul 30, 2024

nicolas-raoul left a comment

Choose a reason for hiding this comment

nicolas-raoul Jul 31, 2024

Choose a reason for hiding this comment

nicolas-raoul Jul 31, 2024

Choose a reason for hiding this comment

kanahia1 Aug 1, 2024

Choose a reason for hiding this comment

nicolas-raoul commented Aug 3, 2024

kanahia1 commented Aug 3, 2024

nicolas-raoul commented Aug 4, 2024

kanahia1 commented May 14, 2024 •

edited

Loading

kanahia1 commented May 17, 2024 •

edited

Loading

kanahia1 commented Jul 28, 2024 •

edited

Loading