Use smart item selection for building levels

[smichel17]

…but keep the same sort order.

Also, store duplicates in history for all quests, so if we want to switch to the "smart" order later, there will already be data.

Fun story: when I was implementing this, I initially added a version of get that implemented the previous de-duplication and maximum size on the other end… then found out we didn't need to use it anywhere, because get is only called from the extension functions defined right there, which all do it themselves already.

(as discussed in #3373 (comment))

[matkoniecz]

I am curious how it will feel in practice, here I never had trouble with needed and recently used elements being often missing

[smichel17]

I converted this back to a draft because I was testing and it didn't seem to work the way I expected, so I want to add some logging and see what's going on.

[smichel17]

here I never had trouble with needed and recently used elements being often missing

In my area

• Most buildings are 1/0, 1/1, 2/0, or 2/1.
• Every so often there's a 3/0, 3/1, or 1/2.

The common values are grouped in blocks, where there's a bunch of 1/0 and 1/1 together, then a bunch of 2/0 and 2/1. This means when you do encounter two oddballs in the same neighborhood, it's more likely to knock out a common value of the block you're not in, than the previous oddball. I hope to mitigate this by saving more history.

In the worst case scenario, two oddballs can force me to open the keyboard 6 times

Bold + italics = had to open the keyboard to enter it

• I'm on a street that's mostly 2/0 and 2/1. At a corner, I come across a 3/0, then a 3/1.
  This kicks the 1/0 out of my history. It's the oldest since I hadn't used it on this street.
• I turn the corner and now I'm in a block of 1/0 and 1/1. The first house is 1/0.
  This kicks out the 1/1, since I also hadn't used it on the previous street.
• Next building is a 1/1. This kicks out the 2/0.
• I turn another corner and I'm back in 2/0 and 2/1s. First building is a 2/0. This kicks out 2/1.
• Next building is a 2/1. This finally kicks out the 3/0 and I'm back to my four most-used presets, plus the oddball 3/1, which will be kicked out next.

This only happens if you run into values in the exact same order that they were knocked out. This doesn't happen often, but it does happen. Shorter runs of "bad luck", like if the oddballs aren't exactly on the corner, are more frequent still annoying.

[smichel17]

Okay, that's fixed. Doesn't help to count 30 into the history if you only store 5 at max :P

[westnordost]

Huh, I lost track. Didn't I merge this already?

[smichel17]

#3373 added smart sort for building type quest, but not building level
#3419 converted LastUsedValuesStore (which I'm tired of writing… I'll just say favs) to use sequences
This enables it for the building level quest and starts storing history with duplicates for all quests

[westnordost]

Ah, I see, good!

Well, most of the code in these classes is from you now, so my ability to assess what is the best solution is very limited. If you think it can be made easier, go ahead!

[smichel17]

Implementation is near-totally replaced, but the api remains mostly unchanged, except using sequences instead of linkedlists. I also tried to keep naming conventions, so I hope it should be pretty similar to reason about. 🤷

[smichel17]

Left to do:

• Remove type from store, so it just handles strings, and mapping is handled only externally (mostly in extension functions) Refactor the store so that serialization is defined outside of it.
• Clean up tests — with 900b86f, they can now be more specific about which function[ality] they're testing

[smichel17]

I removed types from the store locally, but wasn't happy with the result. So I'm continuing to experiment, and I'll let you know if I find something I like.

However, in terms of effort-to-review, I'm also feeling like it was a mistake to mix refactoring and feature work in the same PR, even though the refactoring is to fix awkward types created by the feature.

I'm considering reverting this PR back to d6aaba4 (when westnordost reviewed it), merging that, and then sending a separate PR with just the refactor. Or, I could go all the way and finish refactoring here, then request another review. Any preference?

[smichel17]

Okay, I am done with this (except for the comment/rename). I can either merge the last-reviewed commit and send the refactoring as a separate PR, or you can re-review it altogether.

[westnordost]

I find that this change makes using this store look really complicated. It's just a persisted list of last selected items, man.

[smichel17]

Is it having to create a Factory object that feels that way?
Or having to to chain a [few|bunch] of methods off the end of your get?
Or both?

[smichel17]

On Factory: I'd rather pass the key & serialization functions when initializing the store, but I'm not sure how to make that work with injection.

On chaining: it's easy enough to wrap that in a helper method, but it's actually different in all 3 places where the store is used, so to me it feels like we're just moving the logic further away from the place that cares about it.

Also, for what it's worth, I had a similar reaction when looking at the initial code. (by now, of course, I've gotten used to it)

[westnordost]

The chaining is understandable IMO

[smichel17]

I'll swap it to using initializers (injection be damned 😆) and see if that seems better.

[smichel17]

Done. I experimented with adding a companion object with a fromItems function, where you pass an itemPool instead of a deserialize function, but it wasn't actually an improvement; the function declaration was longer & more complicated than the lines saved.

What we could do if we wanted to make this inject-able is to remove prefs from the constructor and pass it to each of the functions instead. (e.g. fun add(prefs: SharedPreferences, value: T)). Then favs could be initialized normally and prefs could be injected.

[FloEdelmann]

All in all, this looks very good! I have not tested this in action, though; I only reviewed the code.

What we could do if we wanted to make this inject-able is to remove prefs from the constructor and pass it to each of the functions instead. (e.g. fun add(prefs: SharedPreferences, value: T)). Then favs could be initialized normally and prefs could be injected.

Does this mean that every call of get() would then need to pass PreferenceManager.getDefaultSharedPreferences(ctx.applicationContext) as a parameter? Or does ctx.applicationContext suffice? The latter sounds helpful to me.

[smichel17]

Does this mean that every call of get() would then need to pass PreferenceManager.getDefaultSharedPreferences(ctx.applicationContext) as a parameter? Or does ctx.applicationContext suffice? The latter sounds helful to me.

Yes, although it'd be injected and you'd keep a reference. Something like

class SomeForm() {

    @Inject internal lateinit var prefs: SharedPreferences

    private val favs = LastPickedValuesStore<T>(
        key = javaClass.simpleName,
        serialize = { /* whatever */ },
        deserialize = { /* also whatever */ }
    )

    init {
        Injector.applicationComponent.inject(this)
    }

    fun later() {
        favs.get(prefs)
        val newValue: T = // whatever
        favs.add(prefs, newValue)
    }
}

…but just now I realized I think we can get the best of both worlds by lazily initializing the favs. Like

    @Inject internal lateinit var prefs: SharedPreferences

    private val favs by lazy {
        LastPickedValuesStore<T>(
            prefs,
            key = javaClass.simpleName,
            serialize = { /* whatever */ },
            deserialize = { /* also whatever */ }
        )
    }

[smichel17]

I tried implementing the injection idea… it turns out that we still need the InjectedFields trick, and it's a little more lines of code (+50 / -34). Some of that is imports, and it could be less if we added the SharedPreferences injection to the base class, but I'm not sure if it's worth it.

edit: pushed to smichel17@7e8a3a0

edit2: and here's the with the superclass, which is fewer lines: smichel17@0741fb0

[smichel17]

@FloEdelmann I think I've addressed everything you mentioned.

@westnordost You can ignore everything before this comment. The only important change since you last looked at the code is 2e8f64f. I think it makes using the store as easy as before.

edit: There is an open question about whether to keep a favs factory function. Besides that…
The only thing left to decide is what to do about injection. There are three options:

1. No injection.
   favs is a lateinit var which is initialized in onAttach (where we have a Context, to get our SharedPreferences).
   This is how the PR is now.

2. Inject the SharedPreferences into each class.
   favs becomes a val … by lazy, so it won't be constructed until after the prefs are injected.
   This requires adding the InjectedFields trick to two more classes (AGrouped… and AImage…), so it's a bit more code.
   Implemented at smichel17@7e8a3a0 (needs rebase).

3. Inject the SharedPreferences into the superclass.
   i.e. add it to the AbstractQuestAnswerFragment.InjectedFields.
   This is less code, but there might be a few classes which inherit the prefs fields even though they don't need it.
   Implemented at smichel17@0741fb0 (also needs rebase).

[FloEdelmann]

Looks good!

The only thing left to decide is what to do about injection.

Let's leave it like it is now, and postpone the question to #3488 ;)