Fix #87: store caches on multiple lists #5521

pstorch · 2016-03-09T22:29:32Z

I couldn't resist and started working on this issue #87 .

It's not ready yet, but as this is a bigger change I would like to have the chance to get some feedback.

I introduced a n:m relationship table between caches and lists. Then I started changed all code which was working with the one ListId of the Cache to be able to handle a Set of ListIds.

The main visible difference is in the CacheDetailActivity.

It's not fully tested and needs some polishing, too. Any feedback is welcome.

If I'm totally wrong here, please tell me, too. Then it was just a programming exercise for me.
Otherwise I'll continue.

samueltardieu · 2016-03-10T09:04:29Z

As you can see on http://ci.cgeo.org/job/cgeo%20pull%20request/121/console some symbols are missing from resources.

samueltardieu · 2016-03-10T09:22:58Z

main/src/cgeo/geocaching/CacheDetailActivity.java

+                new StoreCacheHandler(CacheDetailActivity.this, progress).sendEmptyMessage(CancellableHandler.DONE);
+            } else {
+                storeCache(StoredList.TEMPORARY_LIST.id, new StoreCacheHandler(this, progress));
+            }
        }


This fragment, which is more a repetition of the fragment above, should be refactored to not be repeated.

Yes, there is also a similar code fragment in another file. I'll refactor them.

pstorch · 2016-03-10T18:01:23Z

Now it's compiling, but 3 Unittests are failing. I'll look into them. But maybe not this evening.

samueltardieu · 2016-03-10T18:04:09Z

It looks like you might be trying to insert several times the same cache into the same list (probably the default one, where the cache may also be, while you're removing the other list it was into).

pstorch · 2016-03-10T18:26:35Z

Yes, the where clause which should check if the cache is already on the standard list was wrong.
It was a quick fix. It's good to have Unittests.

pstorch · 2016-03-12T08:01:46Z

I refactored the similar code blocks mentioned above.

There was a comment in my mail, but not here on github saying:

Using "Store in list" in cache details let the user choose between the standard list and a new list. Other list names are not listed.

Is it obsolete? I couldn't reproduce it. When "Store in list" is clicked, only these Lists are available in the popup which the cache is not yet stored in.

Open Issues I see and I'm still working on:

Presentation of the List of Lists in the CacheDetails (I don't like the "List:" header much)
recognizing if a Geocache isOffline() uses the getLists() Collection. This is not 100% correct. Result: if you store a cache, remove it from all lists and then again store it on a list, the cache is downloaded again. Maybe using the DB _id would be better here.
Count of caches on CacheListsDropDown Menu not working correctly for StandardList.
Review usage of customListIdOffset (I would like to get rid of it)
In the CacheList Menu a new Item to copy caches to another list would be good

pstorch · 2016-03-12T22:59:09Z

Added CopyToListCommand
Adjusted Menu in CacheDetailActivity to correspond with the buttons in the offline_box
Count of Caches per List in the CacheListDropDownMenu seems to work now (I restarted with a blank DB). But needs more testing with migration from old db schema

samueltardieu · 2016-03-14T08:48:41Z

Have you tried running gradle testBasicDebugUnitTest? It looks like this is failing.

pstorch · 2016-03-14T09:04:43Z

Not yet, I'll check.

pstorch · 2016-03-14T21:20:09Z

I fixed the failing UnitTest. Didn't know that there are other tests to be executed by gradle alone. Had to search a little bit in AndroidStudio to find them. :)

I improved the CacheDetails page a little bit. The "drop" button is back, which removed the Cache from all lists. I'm still not 100% happy with the UI, but it's getting better. Any ideas to improve it are welcome.

Regarding the DB migration: I tested again with current master and 3 different Lists. All caches were transferred correctly in the multi list schema. Only going back is not possible. c:geo crashes on startup with a NPE, because it can't downgrade the DB. I think this is a general problem and not related to this change. Should we address this in a separate Issue?

I looked again into the customListIdOffset hack. As mentioned above I would like to remove it, but I'm not sure if there is another clean solution. The only idea I have is changing the IDs of the PseudoLists to a negative value, but that would be another hack. So because it's working today, I guess we better keep it that way.

samueltardieu · 2016-03-14T22:33:54Z

Concerning the hack: indeed, the clean solution would be to add another field to indicate whether the list is virtual or real, but if you look at it from a mathematical point of view, this is just an extra bit, which can be factored into the actual list id (even though we don't use a separate bit for this and just reserve some values). It might be cleaner to reserve the highest bit for that though, but we have already reserved some extra ids should we need some new virtual lists.

pstorch · 2016-03-15T22:36:21Z

Finally I adjusted the CacheDetails Popup dialog. Because there is not much room, I just added a comma separated List of Lists at the end. There is not "Remove from list" button for the individual lists. There is only a "Drop" Menu item to remove the Cache from all lists. I think this should be enough. If the user needs more detailed functionality he needs to open the full CacheDetailsPage.

Regarding the customListIdOffset I doubt that it is worth changing at the moment.

So right now I don't have any open issues left (the failing tests are because geocaching.com has some problems tonight).
I'm waiting for feedback.

samueltardieu · 2016-03-16T08:22:45Z

I am not able to try it right now, but could you please confirm that:

refreshing a cache (from its details or from any list) will keep the cache in all lists it currently belongs to?
importing a GPX (or a PQ) or importing from web will keep the cache in all lists it currently belongs to if the same cache is found during the import?

If those answers are both yes, I propose to merge the issue if there are no objections.

Ping @Lineflyer, @Bananeweizen, @SammysHP, @rsudev, @kumy, @mucek4

pstorch · 2016-03-16T09:09:40Z

Oh no, I missed the GPX Import. Need to fix it.

kumy · 2016-03-16T09:12:20Z

No objection for me. This next feature release will be a long standing one, from people I know... Will do extending tests once merged.

Lineflyer · 2016-03-16T09:27:33Z

In general ok for me.

Just one remark so far:

Finally I adjusted the CacheDetails Popup dialog. Because there is not much room, I just added a comma separated List of Lists at the end. There is not "Remove from list" button for the individual lists.

Why at all does the popup need changes here. As of now there is no reference to any lists.

pstorch · 2016-03-16T09:39:29Z

Just one remark so far:

Finally I adjusted the CacheDetails Popup dialog. Because there is not much room, I just added a comma separated List of Lists at the end. There is not "Remove from list" button for the individual lists.

Why at all does the popup need changes here. As of now there is no reference to any lists.

Now you can hit "store" all the time. If we don't show the lists you can hardly recognize what's happening. The only clue would be the decreasing list of available lists in the list selection popup.

pstorch · 2016-03-16T21:16:56Z

I still can't believe that I missed the GPX (PQ) Import feature.
Anyway, now this should also work with multiple lists.

While working on it I found two other places which needed attention:

the "remove all/selected" menu in the CacheListsActivity removed the caches from all lists, not only from the current list.
the housekeeping code DataStore.cleanIfNeeded() which is triggered on MainActivity startup was still working on the old "reason" column.

Bananeweizen · 2016-03-17T05:54:57Z

main/res/values/strings.xml

@@ -1483,4 +1496,8 @@
    <string name="invitation_title">Invite friends to use c:geo</string>
    <string name="invitation_message">Try the c:geo app for geocaching</string>

+    <string name="cache_list_remove_from">Remove from list</string>
+    <string name="list_list_headline">Lists: </string>


please trim that string

Bananeweizen · 2016-03-17T06:18:09Z

I've only done review, no testing yet. A question out of curiosity: All existing menus and commands have been reworked. I'm wondering if we should have an additional context and options menu on lists to delete the selected/all caches from all lists instead of the current. I really don't have an opinion about that myself yet. It might be confusing for users, or it might be helpful for power users. No clue.

SammysHP · 2016-03-17T07:15:43Z

I thought we decided to use tags. Each cache can have none, one or many tags (aka labels) and "lists" will be replaced by a filter (e.g. checkboxes in the current list dropdown).

samueltardieu · 2016-03-17T07:29:44Z

I thought we decided to use tags. Each cache can have none, one or many tags (aka labels) and "lists" will be replaced by a filter (e.g. checkboxes in the current list dropdown).

This proposal, which works very well for me so far, is an extension of what we have now. We can still change the wording afterwards (list -> tag) and add the new notion (collection of tags -> list) if we want, but the current step is needed anyway.

- Added n:m relationship between caches and lists - List of Lists in CacheDetailActivity - Added CopyToList Command - CacheDetailActivity Menu adjustments - Fixed basicDebugUnitTests - Adjusted CachePopup Dialog - GPX Import with multilist support - CleanIfNeeded - Code Review adjustments

- Added n:m relationship between caches and lists - List of Lists in CacheDetailActivity - Added CopyToList Command - CacheDetailActivity Menu adjustments - Fixed basicDebugUnitTests - Adjusted CachePopup Dialog

pstorch · 2016-03-17T14:10:56Z

Fixed some review comments.

Summary of Discussions from comments above:

"Delete all" menu item in CacheListMenu to delete caches completely from all lists. I could add it, but this menu is already quite full.
lists vs. tags: I think it's just a naming thing.
layout of Cache popup from map: how much list(s) handling functionality do we need here?

rsudev · 2016-03-17T14:36:27Z

list vs tags is indeed 'just a naming thing', but IMO one that can make a difference when it comes to understanding the feature easily.
When you 'store' something on a list, remove/delete becomes somewhat ambiguous and you need to consider namings like 'delete from all lists' to describe a complete removal.
When you organize your caches with tags, the two operations are distinct. You add/remove tags from a cache or store/delete a cache on/from the device.
That said, I have no objections to merge the feature to make it easier to test and gather feedback before the naming question has a 'final' decision.

SammysHP · 2016-03-17T15:29:46Z

What @rsudev said and:

"All caches" are all caches with or without a tag. Furthermore it might contain caches without tags at all. This makes an "default" tag/list obsolete.
You can select/deselect tags, e.g. "vacation" and "must-do". This might be possible with "multiple lists" as well, but is more intuitive.
Tag selections and filters can be mixed via the same UI, they are more or less on the same level while lists and filters are on different levels.

As this is in some way only a naming issue I'm also the opinion to merge this PR and test it. Before the next release we should think about the naming and possible new ways to use the new system (as written above).

samueltardieu · 2016-03-17T17:54:19Z

Let's merge and make it evolve.

Fix #87: store caches on multiple lists

samueltardieu · 2016-03-17T17:54:54Z

Thanks for this painful but very useful work @pstorch.

samueltardieu added the Do not merge / WIP Not for merging and/or work still in progress label Mar 10, 2016

samueltardieu reviewed Mar 10, 2016
View reviewed changes

pstorch force-pushed the fix_87 branch from 1fa3046 to eb86043 Compare March 12, 2016 22:55

pstorch force-pushed the fix_87 branch from 0e3061d to 7f77756 Compare March 15, 2016 22:14

pstorch changed the title ~~DO_NOT_MERGE Fix #87: store caches on multiple lists~~ Fix #87: store caches on multiple lists Mar 15, 2016

samueltardieu removed the Do not merge / WIP Not for merging and/or work still in progress label Mar 16, 2016

pstorch force-pushed the fix_87 branch from 7f77756 to 82d3818 Compare March 16, 2016 21:06

Bananeweizen reviewed Mar 17, 2016
View reviewed changes

pstorch force-pushed the fix_87 branch from 82d3818 to e1419df Compare March 17, 2016 14:05

samueltardieu added a commit that referenced this pull request Mar 17, 2016

Merge pull request #5521 from pstorch/fix_87

f17bdae

Fix #87: store caches on multiple lists

samueltardieu merged commit f17bdae into cgeo:master Mar 17, 2016

SammysHP mentioned this pull request Mar 17, 2016

Tag/Label vs List #5540

Closed

Bananeweizen mentioned this pull request Apr 1, 2016

Performance issue #5536

Closed

pstorch deleted the fix_87 branch April 18, 2016 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #87: store caches on multiple lists #5521

Fix #87: store caches on multiple lists #5521

pstorch commented Mar 9, 2016

samueltardieu commented Mar 10, 2016

samueltardieu Mar 10, 2016

pstorch Mar 10, 2016

pstorch commented Mar 10, 2016

samueltardieu commented Mar 10, 2016

pstorch commented Mar 10, 2016

pstorch commented Mar 12, 2016

pstorch commented Mar 12, 2016

samueltardieu commented Mar 14, 2016

pstorch commented Mar 14, 2016

pstorch commented Mar 14, 2016

samueltardieu commented Mar 14, 2016

pstorch commented Mar 15, 2016

samueltardieu commented Mar 16, 2016

pstorch commented Mar 16, 2016

kumy commented Mar 16, 2016

Lineflyer commented Mar 16, 2016

pstorch commented Mar 16, 2016

pstorch commented Mar 16, 2016

Bananeweizen Mar 17, 2016

Bananeweizen commented Mar 17, 2016

SammysHP commented Mar 17, 2016

samueltardieu commented Mar 17, 2016

pstorch commented Mar 17, 2016

rsudev commented Mar 17, 2016

SammysHP commented Mar 17, 2016

samueltardieu commented Mar 17, 2016

samueltardieu commented Mar 17, 2016

Fix #87: store caches on multiple lists #5521

Fix #87: store caches on multiple lists #5521

Conversation

pstorch commented Mar 9, 2016

samueltardieu commented Mar 10, 2016

samueltardieu Mar 10, 2016

Choose a reason for hiding this comment

pstorch Mar 10, 2016

Choose a reason for hiding this comment

pstorch commented Mar 10, 2016

samueltardieu commented Mar 10, 2016

pstorch commented Mar 10, 2016

pstorch commented Mar 12, 2016

pstorch commented Mar 12, 2016

samueltardieu commented Mar 14, 2016

pstorch commented Mar 14, 2016

pstorch commented Mar 14, 2016

samueltardieu commented Mar 14, 2016

pstorch commented Mar 15, 2016

samueltardieu commented Mar 16, 2016

pstorch commented Mar 16, 2016

kumy commented Mar 16, 2016

Lineflyer commented Mar 16, 2016

pstorch commented Mar 16, 2016

pstorch commented Mar 16, 2016

Bananeweizen Mar 17, 2016

Choose a reason for hiding this comment

Bananeweizen commented Mar 17, 2016

SammysHP commented Mar 17, 2016

samueltardieu commented Mar 17, 2016

pstorch commented Mar 17, 2016

rsudev commented Mar 17, 2016

SammysHP commented Mar 17, 2016

samueltardieu commented Mar 17, 2016

samueltardieu commented Mar 17, 2016