Add an option to invalidate the quest cache #706

ENT8R · 2017-11-23T16:27:29Z

This PR adds an option in the settings to easily delete all saved quests without needing to wipe the whole data of the app. This PR fixes #300

ENT8R · 2017-11-23T16:28:52Z

Currently there is no confirmation dialog if the user clicks on the button (maybe even accidentally). Do you think that this should be added?

ghost · 2017-11-23T17:55:26Z

It can be bothersome to reload all of it if you have a slow or no connection at the moment. If it's not too much work it may be a good idea :)

westnordost · 2017-11-23T18:07:18Z

Oh no, I think there has been a misunderstanding. The user should never have the option to delete all quests. Just the cache should be invalidated. More information in the comments below.

ENT8R · 2017-11-23T18:08:04Z

Oops...

westnordost · 2017-11-23T18:08:20Z

app/src/main/java/de/westnordost/streetcomplete/data/AQuestDao.java

+	{
+		SQLiteDatabase db = dbHelper.getReadableDatabase();
+		db.execSQL("DELETE FROM " + getTableName());
+	}


So this, by the way, also deletes to-be-uploaded quests, reverted quests, ... everything.

westnordost · 2017-11-23T18:10:01Z

app/src/main/res/xml/preferences.xml

+            android:key="quests.deletion"
+            android:title="@string/pref_title_quests_deletion"
+            android:summary="@string/pref_title_quests_deletion_summary"
+            android:widgetLayout="@layout/widget_image_delete"


I misuse the widget layout in some cases only to signal that something is a subpage or that something leads out of this app (into a browser). It should not be used to generally display an icon.

ENT8R · 2017-11-23T18:18:57Z

So I will close this for the first time

westnordost · 2017-11-23T18:34:15Z

So the option in the settings should be called "Invalidate Quest Cache" and what it should do is to clear the downloaded tiles table only.

To clarify for the user, a message like the following should be shown:
"Invalidating the cache causes the quests to be updated next time they are downloaded each.

The quest cache is invalidated automatically after one week and immediately when a quest you solved turns out to be already answered by someone else."

Optionally, the dialog could also ask whether to clear the cache just here (in the tile the user is looking at) or everywhere because I think for most users, the former is what they actually want to do.

ENT8R · 2017-11-23T19:19:19Z

I'm sorry, this is maybe a dumb question, but how do I get the current shown tile (to be more precise: the Point I have to pass as an argument to remove(Point tile) in DownloadedTilesDao.java)

westnordost · 2017-11-23T19:22:55Z

It needs to be calculated from a LatLon:

SlippyMapMath.enclosingTile(current position as lat lon, ApplicationConstants.QUEST_TILE_ZOOM)

Now, getting that LatLon is the hard(er) part. Hint: MapFragment saves the current LatLon into SharedPreferences on onPause().

But this is all optional, you don't need to do it.

ENT8R · 2017-11-23T19:28:00Z

Oh great thank you! I think this will help me a lot!

ENT8R · 2017-11-24T15:09:39Z

Now I updated the code to fit your proposals. There is now the option in the settings to either invalidate all quests in the currently shown tile or just invalidate all known tiles.
It looks like this:

rugk · 2017-11-24T15:13:17Z

"Shown tile" the "shown" is unnecessary IMHO.
And "cancel" should be at the bottom, that is just what I as a user would expect.

westnordost · 2017-11-24T15:26:44Z

Not sure if the subtitle of the Invalidate Quest Cache is necessary. I do not want to put more text than necessary there. Opinions?

Also, could you split the text into paragraphs, like I did in my comment?

Finally, I wouldn't introduce the term "tile" to the user here, is is not introduced to the user anywhere and basically is just a technical detail.
An idea: Add a title - "Where to invalidate the cache?", the answer options are then "Here", "Everywhere" (and "Cancel")

ENT8R · 2017-11-24T15:30:17Z

And "cancel" should be at the bottom

I have no idea how to change the order of the buttons...

westnordost · 2017-11-24T15:28:38Z

app/src/main/java/de/westnordost/streetcomplete/data/tiles/DownloadedTilesDao.java

@@ -70,6 +70,12 @@ public int remove(Point tile)
 				DownloadedTilesTable.Columns.Y + " = ?", whereArgs);
 	}

+	public void removeAll()
+	{
+		SQLiteDatabase db = dbHelper.getReadableDatabase();


getWritableDatabase

But you are doing the same here:
https://github.com/westnordost/StreetComplete/blob/fb3f6ccddf64e35ecb1576a7cb13471474d775f5/app/src/main/java/de/westnordost/streetcomplete/data/visiblequests/VisibleQuestTypeDao.java#L99

westnordost · 2017-11-24T15:33:44Z

app/src/main/java/de/westnordost/streetcomplete/tangram/MapFragment.java

@@ -559,7 +560,7 @@ private void saveMapState()
 	{
 		if(controller == null) return;

-		SharedPreferences.Editor editor = getActivity().getPreferences(Activity.MODE_PRIVATE).edit();
+		SharedPreferences.Editor editor = getActivity().getSharedPreferences(PREF_NAME, Activity.MODE_PRIVATE).edit();


Ah ok,... that's the caveat. Hmm, it's a but ugly that other activities access the preferences of the map this way. But I don't have an idea right now to have a cleaner interface there.

Mmh... me too...

westnordost · 2017-11-24T15:46:39Z

app/src/main/java/de/westnordost/streetcomplete/settings/SettingsFragment.java

@@ -57,6 +96,21 @@ public void onStart()
 		getActivity().setTitle(R.string.action_settings);
 	}

+	private Point getShownTile()
+	{
+		SharedPreferences preferences = getActivity().getSharedPreferences(PREF_NAME, Context.MODE_PRIVATE);


I'd prefer to have the static const here fully qualified, i.e. MapFragment.PREF_NAME etc. because this is the SettingsFragment, everything is settings and prefernces here. It is necessary to know the context of any such preference/settings.

westnordost · 2017-11-24T15:47:36Z

app/src/main/java/de/westnordost/streetcomplete/settings/SettingsFragment.java

+			LngLat pos = new LngLat(
+					Double.longBitsToDouble(preferences.getLong(PREF_LON,0)),
+					Double.longBitsToDouble(preferences.getLong(PREF_LAT,0))
+			);


You create a LngLat here, and then convert the LngLat to a LatLon. Why not simply create a LatLon in the first place? (The default implementation of that interface is OsmLatLon)

westnordost · 2017-11-24T15:49:35Z

app/src/main/java/de/westnordost/streetcomplete/settings/SettingsFragment.java

+						} else
+						{
+							Toast.makeText(getContext(), R.string.invalidation_error, Toast.LENGTH_SHORT).show();
+						}


Please leave out the success and error messages. This will only confuse the users. Instead, just check if getShownTile() is null before building the dialog and only add that answer option ("Only this tile") if the current shown tile can be determined.

Or, optionally, if possible, show that option but disable it.

But if the invalidation succeeded, there should really be no success message?

westnordost · 2017-11-24T15:51:19Z

An idea: Add a title - "Where to invalidate the cache?", the answer options are then "Here", "Everywhere" (and "Cancel")

Or, "Invalidate the cache?" - "Only here" - "Everywhere"

westnordost · 2017-11-24T15:52:01Z

The location of the positive - negative - cancel buttons is predefined by the Android system. Do not attempt to change it.

ENT8R · 2017-11-24T16:45:00Z

This is how it looks now:

westnordost · 2017-11-24T16:46:47Z

Yeah

…

On 24 November 2017 17:03:09 CET, ENT8R ***@***.***> wrote: ENT8R commented on this pull request. > @@ -47,6 +62,30 @@ getFragmentActivity().setCurrentFragment(new QuestSelectionFragment()); return true; }); + + Preference questsInvalidation = getPreferenceScreen().findPreference("quests.invalidation"); + questsInvalidation.setOnPreferenceClickListener(preference -> + { + new AlertDialogBuilder(getContext()) + .setMessage(R.string.invalidation_dialog_message) + .setPositiveButton(R.string.invalidate_shown_tile, (dialog, which) -> { + if (getShownTile() != null) + { + downloadedTilesDao.remove(getShownTile()); + Toast.makeText(getContext(), R.string.invalidation_success, Toast.LENGTH_SHORT).show(); + } else + { + Toast.makeText(getContext(), R.string.invalidation_error, Toast.LENGTH_SHORT).show(); + } But if the invalidation succeeded, there should really be **no** success message? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #706 (comment)

-- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

westnordost · 2017-11-24T16:47:22Z

Oh, that's a bug then!

…

On 24 November 2017 17:17:53 CET, ENT8R ***@***.***> wrote: ENT8R commented on this pull request. > @@ -70,6 +70,12 @@ public int remove(Point tile) DownloadedTilesTable.Columns.Y + " = ?", whereArgs); } + public void removeAll() + { + SQLiteDatabase db = dbHelper.getReadableDatabase(); But you are doing the same here: https://github.com/westnordost/StreetComplete/blob/fb3f6ccddf64e35ecb1576a7cb13471474d775f5/app/src/main/java/de/westnordost/streetcomplete/data/visiblequests/VisibleQuestTypeDao.java#L99 -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #706 (comment)

-- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

ENT8R · 2017-11-24T16:48:20Z

Oh, that's a bug then!

Should I fix it too?

westnordost · 2017-11-24T16:48:57Z

👍

…

On 24 November 2017 17:45:01 CET, ENT8R ***@***.***> wrote: This is how it looks now: <img src="https://user-images.githubusercontent.com/25306497/33218763-f57fe376-d13e-11e7-9966-48d8990733d5.png" width="250px"> <img src="https://user-images.githubusercontent.com/25306497/33218764-f5a2eab0-d13e-11e7-8cfe-1f744972692e.png" width="250px"> -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #706 (comment)

-- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

rugk · 2017-11-24T17:21:57Z

Okay, but what does "Here" mean? I thought one selection clears the "map/tile cache" (maybe call it "map cache" to avoid the term "tile") and when choosing "everywhere" it invalidates the quest cache and the map cache.

ENT8R · 2017-11-24T17:34:14Z

Mmhh... The intention behind this is that the user can redownload the quests in a specific tile (or even on the whole map), if he thinks that some of the quests are outdated, after clicking on one of the two buttons.

The app knows where it downloaded quests already and where not. This information is stored in a database (downloaded_tiles). If the user wants to invalidate the quests through the settings, this database is either completely cleared or only a specific area (tile) gets deleted. If the user now scans again for quests in this specific area, all quests in this area are redownloaded, because StreetComplete thinks that it has not scanned yet there for new quests.

It's not intented for clearing the map cache...

rugk · 2017-11-24T19:29:30Z

Ah so "here" makes sense.
It could only be a bit confusing as the last screen the user sees is the settings screen, so one does not immediately associate "here" with the current map position. If you see what I mean…

"In this area" would maybe be a bit more explicit/specific, but I admit that it may be too long.

westnordost · 2017-11-24T21:08:00Z

Hmm, yeah, actually...

I think, @rugk ist right.

You know, I have had to explain the mechanism with quest cache invalidation to various people what seemed to me a dozen times already. I had hoped that the implementation of such a "flush cache" button would bring this thing to a close (also using the confirmation message box to shortly explain the mechanism) but it seems the separation of "clear cache here" and "clear cache everywhere" is overengineered, in, that it actually adds to the confusion:
Not only the thing about "what is here?". Additionally, I came to remember that the user does not know how big the "here" is. The enclosing tile of any position might be something like 1km² (I think), but the position itself might be on the edge of that tile. Meaning, that he might be very close to an area where the cache has not been cleared. And that will confuse the user because he thinks, "well, didn't I just clear the cache here? Why isn't this working?!"

Finally, I think it is not too necessary to have the user have such a finegrained control over where to clear the cache because after one week only, all his freshly downloaded quests will be invalidated anyway. Not deleted, just invalidated.
So, if he pre-downloaded some area where he plans to go on vacation in the next few weeks, even though the quests are by then already invalidated, he can still solve them as normal.

All in all, my conclusion is to remove that "clear cache only here" option again. There is too little gain for the added complexity and confusion.

Sorry about my idée fixe, first letting you implement this and then deciding otherwise. To accommodate, I can offer to remove these parts myself so you do not have to do that.

ENT8R · 2017-11-24T22:17:16Z

No problem! I will do this tomorrow 😀

ENT8R · 2017-11-25T18:31:25Z

All in all, my conclusion is to remove that "clear cache only here" option again

This is removed now

westnordost · 2017-11-25T19:55:29Z

app/src/main/java/de/westnordost/streetcomplete/settings/SettingsFragment.java

+					.setPositiveButton(R.string.invalidate_confirmation, (dialog, which) -> {
+						downloadedTilesDao.removeAll();
+					})
+					.setNegativeButton(android.R.string.cancel, (dialog, which) -> {})


(dialog, which) -> {} can be null, by the way.

westnordost · 2017-11-25T19:56:59Z

Looks good!

Note that I will merge this into v4, as it contains both new strings and, well, is a new feature. (I will be not available the whole December to fix possible bugs, so I am very cautious right now).

ENT8R · 2017-11-25T19:58:51Z

That's fine!

westnordost · 2018-01-07T21:47:29Z

This github resolve conflict editor does not seem to work.

pietervdvn · 2018-02-08T00:21:47Z

Awesome! You guys rock a lot!

I really appreciate the app and all the effort you are putting into it!

westnordost · 2018-02-08T18:53:44Z

:-) Am 8. Februar 2018 01:21:49 MEZ schrieb Pieter Vander Vennet <notifications@github.com>:

…

Awesome! You guys rock a lot! I really appreciate the app and all the effort you are putting into it! -- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: #706 (comment)

-- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

ENT8R added 2 commits November 23, 2017 17:22

Add the option to flush the quest cache

9cf86aa

Remove unused import statement

bb45ad0

westnordost reviewed Nov 23, 2017

View reviewed changes

ENT8R closed this Nov 23, 2017

Invalidate the quest cache instead of delete all quests

d6f7fec

ENT8R reopened this Nov 24, 2017

westnordost requested changes Nov 24, 2017

View reviewed changes

Fix issues

e7b85c6

ENT8R changed the title ~~Add an option to delete the quest cache~~ Add an option to invalidate the quest cache Nov 25, 2017

Remove the option to invalidate only a specific tile

18c8c45

westnordost approved these changes Nov 25, 2017

View reviewed changes

Use null instead of empty function

a2b7519

HolgerJeromin mentioned this pull request Nov 29, 2017

Different buildings sharing same housenumber #712

Closed

westnordost added 2 commits January 7, 2018 22:46

Merge branch 'master' into flush-quest-cache

bd79d4b

Merge branch 'master' into flush-quest-cache

e58b5ec

westnordost merged commit e58b5ec into streetcomplete:master Jan 7, 2018

ENT8R deleted the flush-quest-cache branch January 14, 2018 14:53

Add an option to invalidate the quest cache #706

Add an option to invalidate the quest cache #706

Conversation

ENT8R commented Nov 23, 2017

ENT8R commented Nov 23, 2017

ghost commented Nov 23, 2017

westnordost commented Nov 23, 2017

ENT8R commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ENT8R commented Nov 23, 2017

westnordost commented Nov 23, 2017 • edited Loading

ENT8R commented Nov 23, 2017 • edited Loading

westnordost commented Nov 23, 2017

ENT8R commented Nov 23, 2017

ENT8R commented Nov 24, 2017

rugk commented Nov 24, 2017

westnordost commented Nov 24, 2017 • edited Loading

ENT8R commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westnordost Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westnordost Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westnordost commented Nov 24, 2017

westnordost commented Nov 24, 2017

ENT8R commented Nov 24, 2017

westnordost commented Nov 24, 2017 via email

westnordost commented Nov 24, 2017 via email

ENT8R commented Nov 24, 2017

westnordost commented Nov 24, 2017 via email

rugk commented Nov 24, 2017

ENT8R commented Nov 24, 2017 • edited Loading

rugk commented Nov 24, 2017

westnordost commented Nov 24, 2017 • edited Loading

ENT8R commented Nov 24, 2017

ENT8R commented Nov 25, 2017

Choose a reason for hiding this comment

westnordost commented Nov 25, 2017

ENT8R commented Nov 25, 2017

westnordost commented Jan 7, 2018

pietervdvn commented Feb 8, 2018

westnordost commented Feb 8, 2018 via email

westnordost commented Nov 23, 2017 •

edited

Loading

ENT8R commented Nov 23, 2017 •

edited

Loading

westnordost commented Nov 24, 2017 •

edited

Loading

westnordost Nov 24, 2017 •

edited

Loading

westnordost Nov 24, 2017 •

edited

Loading

ENT8R commented Nov 24, 2017 •

edited

Loading

westnordost commented Nov 24, 2017 •

edited

Loading