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.

Sign up for GitHub

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

Jump to bottom

Made Nearby show all pins that could be presented on the screen, rather than a circle #5553

Merged
merged 20 commits into from
Mar 24, 2024
Merged

Made Nearby show all pins that could be presented on the screen, rather than a circle #5553

merged 20 commits into from
Mar 24, 2024

Conversation

kanahia1
Copy link
Contributor

Description (required)

Fixes #5480

What changes did you make and why?

Tests performed (required)

Tested prodDebug on Samsung S21 FE with API level 33.

Screenshots (for UI changes only)

WhatsApp.Video.2024-02-16.at.02.50.42_95e8f853.mp4

@kanahia1
Copy link
Contributor Author

@nicolas-raoul, Can you please review this PR 🙂

nicolas-raoul
nicolas-raoul requested changes Feb 16, 2024
View reviewed changes
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.

It works and I love it already!

Just a few nits to correct.

app/src/main/java/fr/free/nrw/commons/mwapi/OkHttpJsonApiClient.java Outdated Show resolved Hide resolved
app/src/main/java/fr/free/nrw/commons/nearby/NearbyController.java Outdated Show resolved Hide resolved
app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java Outdated Show resolved Hide resolved
app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java Outdated Show resolved Hide resolved
app/src/main/resources/queries/nearby_query.rq Outdated Show resolved Hide resolved
nicolas-raoul
nicolas-raoul requested changes Feb 17, 2024
View reviewed changes
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.

A few more very minor things

app/src/main/java/fr/free/nrw/commons/nearby/NearbyController.java Outdated Show resolved Hide resolved
app/src/main/java/fr/free/nrw/commons/nearby/NearbyController.java Outdated
return null;
}

List<Place> places = nearbyPlaces.getFromWikidataQuery(screenTopRight,screenBottomLeft,Locale.getDefault().getLanguage(),shouldQueryForMonuments, customQuery);
Copy link
Member

Choose a reason for hiding this comment

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

spaces, line length

app/src/main/java/fr/free/nrw/commons/nearby/NearbyPlaces.java Outdated Show resolved Hide resolved
app/src/main/java/fr/free/nrw/commons/nearby/NearbyPlaces.java Outdated Show resolved Hide resolved
nicolas-raoul
nicolas-raoul reviewed Feb 17, 2024
View reviewed changes
app/src/main/java/fr/free/nrw/commons/mwapi/OkHttpJsonApiClient.java
@@ -328,6 +329,76 @@ public List<Place> getNearbyPlaces(final LatLng cur, final String language, fina
throw new Exception(response.message());
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need the methods above and below this one (with the radius parameter)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's required during upload to prepopulate categories and depictions based on the image coordinates where their radius is used.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes thanks! :-)

Does nearby_query.rq still work with a radius though?
If not, maybe splitting the file into two files named for instance rectangle_query_for_nearby.rq and radius_query_for_upload_wizard.rq might be required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@kanahia1
Copy link
Contributor Author

kanahia1 commented Feb 18, 2024 •
edited

@nicolas-raoul, I have made the changes suggested by you 1e732ed. I completely forgot about the monuments since they are also using radius query we may need different files for them, Can you please share the names for nearby_query_monuments.rq

@nicolas-raoul
Copy link
Member

How about rectangle_query_for_nearby_monuments.rq?
Is a radius query needed for monuments?

@kanahia1
Copy link
Contributor Author

kanahia1 commented Feb 26, 2024 •
edited

I found out that everywhere for radius, shouldQueryForMonuments is set to false. So removing radius query for monuments will be okay :-) d7ce78e

@kanahia1
Copy link
Contributor Author

Hey @nicolas-raoul

Caused by: com.android.build.gradle.tasks.ResourceException: /home/runner/work/apps-android-commons/apps-android-commons/app/src/main/res/values-yue-hant: Error: Invalid resource directory name
	at com.android.build.gradle.tasks.MergeResources.doFullTaskAction(MergeResources.kt:291)
	at com.android.build.gradle.tasks.MergeResources.doTaskAction(MergeResources.kt:320)

Is failing of checks related to #5571

@nicolas-raoul
Copy link
Member

Sorry for the delay, would you mind fixing thee conflict?

@kanahia1
Copy link
Contributor Author

Hey @nicolas-raoul, I think all the conflicts are fixed now. Can you please look into it.

@kanahia1
Copy link
Contributor Author

Friendly ping @nicolas-raoul

nicolas-raoul
nicolas-raoul requested changes Mar 24, 2024
View reviewed changes
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.

Would you mind performing the stylistic changes in a different pull request?
They make it very difficult to spot what actually changed.
Thanks! :-)

@kanahia1
Copy link
Contributor Author

Hey @nicolas-raoul, Sorry to ask you a simple question but can you please give more information what kind of stylistic changes should I make or should do another pull request by closing this one (So that I could be easy to review)

@nicolas-raoul
Copy link
Member

I must have dreamt, I don't see them anymore, sorry my bad! 😰

nicolas-raoul
nicolas-raoul approved these changes Mar 24, 2024
View reviewed changes
Copy link
Member

@nicolas-raoul nicolas-raoul left a comment •
edited

Choose a reason for hiding this comment

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

Working well even for Manhattan, thanks!

@nicolas-raoul nicolas-raoul merged commit 724e4db into commons-app:main Mar 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-raoul nicolas-raoul nicolas-raoul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Nearby show all pins that could be presented on the screen, rather than a circle
2 participants
@kanahia1 @nicolas-raoul