Julow / Unexpected-Keyboard

A lightweight virtual keyboard for developers.
GNU General Public License v3.0
1.25k stars 168 forks source link

Emoji Update #612

Closed tmqCypher closed 1 month ago

tmqCypher commented 2 months ago

Finally got a chance to take a look at updating the emoji backend, apologies for the wait!

I made what seemed to be the most necessary changes talked about in #583:

I kept the Emoji class because it keeps EmojiGridView and EmojiGroupButtonsBar cleaner and keeps space for potential features like #173, which would require more emoji-specific functionality than is offered by KeyValue.

The code does all of the above as it is now, but as part of the update I'd like to suggest saving recently used preferences as individual string-integer entries rather than a string set. We would no longer have to parse the integers out when the preferences are loaded or redundantly save unchanged values for every keystroke.

If this sounds like a good idea, or if any other preference changes should be made, they should definitely be implemented now to avoid a much more headache-inducing multi-stage migration process in the future.

Let me know what you think!

tmqCypher commented 2 months ago

You mean having one StringSet representing the emojis that were used previously, and an extra integer preference for each of them ?

There's also [getAll()](https://developer.android.com/reference/android/content/SharedPreferences#getAll()) but it's very unsafe and worrying about LAST_USE_PREF and MIGRATION_CHECK_KEY seems like a step backward.

My idea was to store only the integer preferences, use getAll() to retrieve the emoji-integer pairs, and copy the values to _lastUsed. This wouldn't require any de-serialization and would also remove the need for MIGRATION_CHECK_KEY since migration status could be determined simply by checking for the old string set with prefs.contains(LAST_USE_REF).

All of the preferences would have a single, known type and nothing would be getting modified, so it doesn't seem unsafe to use getAll() in this case. I'm new to Android development though, so I'll trust your judgement if you'd still rather avoid it.

Julow commented 1 month ago

Sorry for the slow reply. This should not be a draft anymore, I would like to merge it!

My idea was to store only the integer preferences, use getAll() to retrieve the emoji-integer pairs, and copy the values to _lastUsed.

That works for me :) Though, the PR is working, so I suggest to merge it like this now and make further improvements later.

Julow commented 1 month ago

The new preference layout you propose seems good to me but will require yet an other migration function as I want to avoid leaving this PR open for too long. I plan to release soon and need the updated emojis.