-
Notifications
You must be signed in to change notification settings - Fork 1.3k
More powerful pin filtering of Nearby map via allowing users to modify SPARQL query #3410
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
Comments
My suggestions on how to implement this:
What do you think @neslihanturan @VojtechDostal @nicolas-raoul ? |
Sounds perfect! |
I love the use case @misaochan . Now I am more clear about this issue to create a UI. I agree @nicolas-raoul for the last step. Also we can have reset to default or save my query buttons for further usages maybe? So that they can bring their query back instead of typing all over again. |
Custom SPARQL is rather niche, so personally I don't really think a "save my query" feature would be worth the complexity&maintenance. People who write SPARQL queries should be able to save their queries in a memo app, a text file or even an email draft :-) By the way, the advantage of letting users modify the whole query (rather than just a fraction like WikiShootMe) is that we can do much deeper customization, for instance here is how I would possibly use this feature:
This is all doable by editing the SPARQL query. |
I agree that we probably don't need a "save my query" function for the time being, and also the full query should be editable.
Ah, when I said "invalid" I was actually thinking about the request failing - are they different? I agree with both of you, auto-resetting is probably a bad idea now that I think about it. :) |
Hi everyone, there are some points needs to be clarified appeared during my mockup attempts. Currently we use this query:
Where location, label, description, class, classLabel, icon, wikipediaArticle, commonsArticle, commonsCategory, pic, destroyed are variables that we reach and use their values in NearbYResultItem.kt class. On the other hand ${LONG}, ${LAT}, ${RAD} and ${LANG} are the inputs that we send to the query. So: 1- We should notify the user about they can use these inputs (${LONG}, ${LAT}, ${RAD},${LANG}) in their query as well.
|
For my use case number 2 above, I need to modify the COALESCE, which is outside of WHERE. |
Hmm, we can let people edit COALESCE without editing "as ?label", "as ?description" parts. Them might be superusers when it comes to SPARQL but it is not only about the SPARQL, also about to know about the code in our side and we can not expect them to know the variable names that our code preferred. |
We can list the variables in the tooltip or "read more". Having several text fields separated by |
"Having several text fields separated by as ?label/etc areas would make copy/pasting (from the wiki for instance) very cumbersome and error-prone." |
Looks good to me! A few minor things:
|
Looks good to me @neslihanturan . :) @nicolas-raoul 's changes sound good as well.
I think that it might be beneficial to check that these variables all exist when the user taps "Apply" - if any of them don't exist, show an error dialog and do not proceed. Otherwise, for instance if a user removes commonsCategory, they could get a nice map with all the pins shown, but the app will probably crash at the final stage of a Nearby upload since we require a check for that variable. Given that the events are so far apart, they may not connect the crash to their SPARQL modification, and file a crash report that we will be scratching our heads over for months (especially if they don't mention in the crash report that they made a modification). |
Checking that the request's String contains the String I would recommend against checking for the String |
Makes sense. I wonder if it's possible to make a note in ACRA if a modified query is being used in Nearby? |
Looks perfect to me! Maybe "Advanced" is enough, instead of "Advanced options"? |
Looks great to me. :) I think "Advanced" would look nicer with the UI, but "Advanced Options" seems clearer. I'm OK with either one. |
I also think that Advanced Options is clearer, but I a okay whichever you wish. |
Sorry for nitpicking, let's go with "Advanced Options" since you both think it is clearer :-) |
Hi @misaochan , question around this
|
I think most people will not modify the But if we want to take care of those who modify it, I can see two cases that could be considered as special:
|
Thanks for the input @nicolas-raoul , this should work : ) |
Agreed, handling those cases should be sufficient. :) IMO the basic premise of this feature is that it's only for advanced users, and anything the user changes in that text field can come with a potential of rendering Nearby unusable for them. (Which is why I think, as I mentioned at #3410 (comment) , that we should add a note to the ACRA crash report and Send Logs that a custom SPARQL query was used, so that it doesn't raise false alarms if such errors are reported.) |
As requested by @VojtechDostal , power users familiar with SPARQL would find it useful to be able to partially modify the SPARQL query themselves to filter results. We can probably implement this in a similar manner to https://tools.wmflabs.org/wikishootme#lat=50.0720077&lng=14.424068199999999&zoom=15
The text was updated successfully, but these errors were encountered: