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

bypass censorship in Russia #1438

Merged
merged 7 commits into from
Jun 29, 2019
Merged

Conversation

matkoniecz
Copy link
Member

@matkoniecz matkoniecz commented Jun 22, 2019

provide fallback Overpass server (thanks to Kumi Systems for providing it, and thanks again to Roland Olbricht for providing main Overpass instance)

fixes #1389


My work on this pull request was sponsored by a NGI Zero Discovery grant

@westnordost
Copy link
Member

Aren't there any quests in StreetComplete that would fail to download if no attic data is available? (The construction quests)

@matkoniecz
Copy link
Member Author

The construction quests use attic data in test suite, not in normal operation.

Construction quests use "last modified" not requiring attic - but I may test is it actually working correctly.

@westnordost
Copy link
Member

Ah okay

@westnordost
Copy link
Member

westnordost commented Jun 23, 2019 via email

@streetcomplete streetcomplete deleted a comment from matkoniecz Jun 23, 2019
@streetcomplete streetcomplete deleted a comment from matkoniecz Jun 23, 2019
@matkoniecz
Copy link
Member Author

Actually, I would very much prefer not to add a description of the Overpass URL to the localized strings but simply display the URL.

Done.

@matkoniecz
Copy link
Member Author

Construction quests use "last modified" not requiring attic - but I may test is it actually working correctly.

Tested to be sure. Attic queries are failing on Kumi servers (I reported this) but construction quest works without problems.

Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

I am surprised that this works. It shouldn't, at least not before an app restart:

In the QuestModule, the list of quest types that all make use of the OverpassMapDataDao is created as a singleton. That means, the method QuestModule::questTypeRegistry should only be called once on application start and then never again. The OverpassMapDataDao to be used for each quest in that list is passed in in that function once. So, the Prefs.OVERPASS_URL is only read once - when the OverpassMapDataDao is created, to be used in the quest type registry that is also is created.

Not sure what is the best practice is in a case like this.

However, it is clear that this class should actually also be a @Singleton because there should only ever be one instance of it as it handles the quota to Overpass. This makes exchanging the URL a bit easier because then, only the URL of one OverpassMapDataDao needs to be exchanged. OsmConnection, a field of that class, has a method named setApiUrl, so there you go. Though, I am really not sure if this is a clean solution. It relies so much on the singleton-thing.

Other input or experience on what's the best practice on exchanging injected dependencies is welcome.

If there is no good solution, a cheap one would also be to require the user to re-start the app. Could be as cheap as mentioned in the description of the setting.

app/src/main/res/values/arrays.xml Outdated Show resolved Hide resolved
@matkoniecz
Copy link
Member Author

at least not before an app restart

OK, that part was not tested - I changed setting, run queries that worked (using old server), added debug to confirm that server is changed, restarted, queries still worked and new server was appearing in logs.

And yes, restart is required for option to actually apply according to a proper testing.

If there is no good solution, a cheap one would also be to require the user to re-start the app. Could be as cheap as mentioned in the description of the setting.

I did it for now. I will now try to research is there a good way to do this.

@westnordost
Copy link
Member

That's fine actually. We don't expect users to change this server all the time.

provide fallback overpas server (thanks to Kumi Systems for providing it!)
fixes streetcomplete#1389
This reverts commit bc55cc648e3a4a1c7e8c0a1877fd4162f605ccdd.
@westnordost westnordost merged commit fedb3d4 into streetcomplete:master Jun 29, 2019
@dbf256
Copy link

dbf256 commented Jun 29, 2019

thanks, waiting for next version :)

@westnordost
Copy link
Member

This will be the next major version, not the version I just released 10mins ago. Beta of that version will be out earliest in 2 weeks or so.

@dbf256
Copy link

dbf256 commented Jun 29, 2019

Great, I see some complains (with 2 stars) from Russian users in play market, hope they will increase number of stars after this fix.

@rugk
Copy link
Contributor

rugk commented Jun 30, 2019

I see some complains (with 2 stars) from Russian users in play market

@westnordost should be able to reply to these complains. Maybe you can point them here… 😄

@matkoniecz
Copy link
Member Author

matkoniecz commented Jun 30, 2019

not the version I just released 10mins ago. Beta of that version will be out earliest in 2 weeks or so.

If someone is especially interested in this feature - texts for translations are already available for translation at POEditor (see https://github.com/westnordost/StreetComplete/blob/master/CONTRIBUTING.md#translating-the-app )

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.

Have a fallback for overpass server
4 participants