Move quest filtering logic to ViewModel

[irakliy01]

A small refactoring on quest filtering to improve performance. Now filtering is done in the ViewModel. Also introduced ResourceProvider, useful for accessing resources from non-UI layers/components.

[mnalis]

Now filtering is done in the ViewModel [...]

Thanks for the effort! But, I noticed however there was this comment in the code:

// the filtering is not done in the view model because it involves accessing localized
// resources, which we consider UI (framework) specific data and view models should be
// free of that.
// NOTE: This is very slow though, it involves getting the string resource, lowercasing it

Perhaps I'm misunderstanding it, but it seems to be saying that filtering in view model is unwanted (even if it brings performance improvements) for design decisions? Or does this PR address that concern somehow?

[irakliy01]

To avoid the ViewModel directly accessing resources , I introduced the ResourceProvider abstraction (interface). The QuestSelectionViewModel now depends on the ResourceProvider interface to fetch the necessary quest titles. It doesn't know about context or specific android resource mechanisms. The DefaultResourceProvider implementation encapsulates the logic for actually retrieving the strings, including handling the correct application locale to ensure the ViewModel receives the properly localized titles.
So, the key idea is that while the filtering operation happens in the ViewModel for performance, the access to localized resources needed for that operation is handled through an abstraction (ResourceProvider), maintaining the separation of concerns and keeping the ViewModel decoupled from direct android framework dependencies like context for resource loading. This felt like a standard way to approach it, but let me know if you have concerns or if it doesn't quite align with your vision for the app's architecture.

[westnordost]

I think yours is a pretty swift solution! I didn't even think of that! In general, you've made it so that there are no direct dependencies of the view model to the UI parts, very cool! Thank you for this! I don't quite like that TextFieldValue is passed to the ViewModel, but it looks like it is just a pure data class, so I guess that's okay. (Couldn't it just be a String, though?)

Say, how did you spot this in the code? Were you familiarizing yourself with the codebase, or did you notice slowdowns yourself and started investigating as to the cause of this?

Note that ShowQuestFormsScreen the exact same quest filtering as in the quest selection setting screen is used. It would make sense if those two places would work consistently (But this needn't be done in this same PR).

[westnordost]

Note / FYI:

Sooner or later, we want to migrate to Compose Resources (org.jetbrains.compose.components), I hope this class won't make it more difficult to migrate.

[irakliy01]

While I haven't worked directly with KMM before, I checked the documentation on accessing resources outside composables you mentioned. It looks like they provide ways to do that, which suggests the core functionality needed by the ViewModel should still be possible.

My hope is that introducing the ResourceProvider interface might actually help with the migration. Since the ViewModel depends on the interface, the DefaultResourceProvider (which uses Android Context) could potentially be swapped out later with a new implementation that uses the Compose Resources API, without needing major changes in the ViewModel itself.

[westnordost]

Suggested change

	private val _searchText = MutableStateFlow(TextFieldValue())
	override val searchText: StateFlow<TextFieldValue> = _searchText
	override val searchText = MutableStateFlow(TextFieldValue())

(etc.)

...would be more consistent with the rest of the codebase. I.e. the implementation uses MutableStateFlow while the interface just exposes a StateFlow. So, I don't need to effectively duplicate all StateFlow fields (one private MutableStateFlow, one public StateFlow).

[irakliy01]

Actually, I forgot to add _searchText.asStateFlow(). This ensures that the user of searchText can't cast it to MutableStateFlow and modify the state. Existing StateFlows in the ViewModel can be modified in the composable code like this:

    LaunchedEffect(Unit) {
        delay(2000)
        (viewModel.searchText as MutableStateFlow<String>).value = "AAA"
    }

This is just an example. In real case scenarios I've seen desperate devs trying to modify states they shouldn't modify. This is why it's a common practice to expose StateFlows to the UI components and back them with MutableStateFlows.
(But those cases are so rare, that I can just change this code to make it more consistent with the codebase)

[westnordost]

Well, I am not desperate. I don't cast :-)

[westnordost]

Also, "desperate" is not the word I'd use for devs that break the interface by casting (instead of doing it properly). ;-)

[irakliy01]

I meant in big projects sometimes managers push devs to make quick fixes. In those cases developers forget about clean code and start making mess in a project. Backing fields, Api/Impl and other patterns/techniques are meant to fight with that.

[westnordost]

Yeah, I know what you mean. ...And it is only a matter of time until such botch comes back haunting the developers, oftentimes not even hitting the dabblers but those that come after... 😬 Hrr, memories, that gives me the creeps.

(So, maybe the pattern you use is generally preferable, as a guard against such practices. But well, being kind of the BDFL in this project, I won't allow botch in the first place, so such safeguard is less necessary. Readability and consistency is higher on my list.)

[irakliy01]

I don't quite like that TextFieldValue is passed to the ViewModel, but it looks like it is just a pure data class, so I guess that's okay. (Couldn't it just be a String, though?)

Yeah, I agree that probably a String would be better here than TextFieldValue for KMM. But I'll have to introduce my own version of TextFieldValue data class for cursor location to work.

Say, how did you spot this in the code? Were you familiarizing yourself with the codebase, or did you notice slowdowns yourself and started investigating as to the cause of this?

I wanted to add a country-specific presets feature (before I found enabledInCountries property), since in my country, some quests are not relevant. And that's when I stumbled upon this code and decided to refactor it. Refactor was motivated by fixing that potential performance bottleneck and improving the architecture by introducing the ResourceProvider.

Note that ShowQuestFormsScreen the exact same quest filtering as in the quest selection setting screen is used. It would make sense if those two places would work consistently (But this needn't be done in this same PR).

I can update it in the next commit

[irakliy01]

I can wait for #6210 to update deprecated configuration.locale to configuration.locales

[westnordost]

But I'll have to introduce my own version of TextFieldValue data class for cursor location to work.

I think the cursor will be fine. Only on configuration change, the cursor location wouldn't be remembered. Otherwise it should work just as fine with a String. FWIW, TextFieldValue is available in common, i.e. no need to create an own class to make it work in multiplatform.

[westnordost]

Perfect.

By the way, collectAsStateWithLifecycle() is Android-only. But I can also change that later when I actually start migrating to Compose Multiplatform.

[westnordost]

I wanted to add a country-specific presets feature (before I found enabledInCountries property), since in my country, some quests are not relevant.

Anyway, for quests that are not only not relevant but even useless (because the thing is always X or never Y, e.g. if there are no tactile pavings on crosswalks at all, if there are no cycleways at all, etc.), you are invited to create a PR to disable such quests for Georgia.