Skip to content

Read once and reuse the query file's content #600

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 1 commit into from
May 19, 2017

Conversation

whym
Copy link
Collaborator

@whym whym commented May 16, 2017

See #569 (comment)

Also fixes the bug introduced in #599 (the variable names RAD and RADIUS being inconsistent)

@whym whym force-pushed the nearby-file branch 4 times, most recently from 2834a15 to 7739085 Compare May 16, 2017 13:18
@whym whym requested a review from maskaravivek May 16, 2017 13:21
@@ -13,7 +13,7 @@ SELECT
SERVICE wikibase:around {
?item wdt:P625 ?location.
bd:serviceParam wikibase:center "Point(${LONG} ${LAT})"^^geo:wktLiteral.
bd:serviceParam wikibase:radius "${RADIUS}" . # Radius in kilometers.
bd:serviceParam wikibase:radius "${RAD}" . # Radius in kilometers.
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to change the param to RAD

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I slightly prefer it because it will make code look more aligned.

                .replace("${RAD}", String.format(Locale.ROOT, "%.2f", radius))
                .replace("${LAT}", String.format(Locale.ROOT, "%.4f", cur.latitude))

Copy link
Member

Choose a reason for hiding this comment

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

In your previous PR I guess you forgot to make the change in the nearby_query.rb, so the nearby places stopped loading. Can you test it thoroughly once? I will be happy to merge it then. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The nearby list and map work okay for me with these devices.

  • Android 6.0.1, API 23 (real device)
  • Android 7.1.1, API 25 (emulator)
  • Android 5.1.1, API 22 (emulator)

Sorry about the oversight - the previous patch was incomplete.

@misaochan
Copy link
Member

Is this ready to merge?

@whym
Copy link
Collaborator Author

whym commented May 19, 2017

I don't see a remaining issue.

@misaochan misaochan merged commit f6a7759 into commons-app:master May 19, 2017
@whym whym deleted the nearby-file branch July 19, 2017 06:43
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.

3 participants