Stats storages and crash in Aggregate mode

[tuancoltech]

🚀 Summary

Fix crash in Android Navigator, Aggregation mode.
Issue: #412

[tuancoltech]

This is for fixing a side effect when files access permission toggle was disabled, caused by this commit: c7d14c1

[kirillt]

ARK-Builders/arklib-android#125 merged

[kirillt]

@tuancoltech am I right that we only need to bump arklib-android version before merging?

[tuancoltech]

@tuancoltech am I right that we only need to bump arklib-android version before merging?

@kirillt This PR doesn't depend on any change in arklib-android, so please feel free to merge it :)

[kirillt]

@tuancoltech there are some unresolved references in the last build.

[sonarqubecloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Incomplete fix

[kirillt]

It seems that just logging the exception can be harmful for the storage. We can have 2 kinds of exception here:

1. Exception during decoding the storage, i.e. reading from disk.
2. An unlikely problem during putAll.

In either case, the tagLabeledAmount mapping is incorrect. When new stats appear, the will be written to the disk, but only part of the original storage will be written. E.g. if we have 5 entries in the storage, we read 2 of them and then we have an exception. We continue execution with 2/5 entries. Then new entry is coming, so we write 3/6 to the disk. This means we lose some entries in case of the exception.

Stats storage is not that important comparing to tags or scores, but it enhances UX by sorting tags in last-used order. We should avoid losing this data if possible.

[kirillt]

Another concern is that the fix is applied to TagLabeledNStorage, but there are also 3 other similar storages: TagLabeledTSStorage.kt, TagQueriedTSStorage, TagQueriedNStorage. These storages have the same bug, they should be fixed, too.

[kirillt]

But the best approach would be to re-design stats storages... They were implemented during autumn 2022. At summer 2023, we've refactored storages completely and separated reusable primitives. See FileStorage and FolderStorage from arklib-android.

In fact, the data stored in stats storages is very simple key-value mapping. I think, it's possible to implement them similarly to ScoreStorage. Scores storage is id-to-integer mapping, and stats storages are either tag-to-integer or tag-to-timestamp. But in any case, the code will be very similar to scores storage.

Better design possible

[mdrlzy]

I think, it's possible to implement them similarly to ScoreStorage

They are not quite like ScoreStorage. They can often receive new events (once per second), which will provoke overwriting of large file (there is only one for the entire root). Here we need to implement a new storage, which will have a debounce flush to disk for new entries.

I think there is no point in fixing them here; it’s better to move and refactor in arklib-android.

[kirillt]

@mdrlzy good note about debouncing. We can implement such a parameter in our base storage classes. Then stats storage should be similar to score storage, just debounce parameter would differ.

[kirillt]

@tuancoltech let me know if you need help with implementing StatsStorage

[tuancoltech]

@tuancoltech let me know if you need help with implementing StatsStorage

@kirillt Sure, I'll ask here in the case.

[kirillt]

@tuancoltech We might need small refactoring in arklib-android because BaseStorage hard-codes ResourceId as the type acceptable as keys of a storage. In case of tag stats, keys belong to the String type.