Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preferences API extensions #255

Merged
merged 7 commits into from Mar 25, 2020
Merged

Preferences API extensions #255

merged 7 commits into from Mar 25, 2020

Conversation

Quillraven
Copy link
Contributor

Hi,

was working on preferences in my game and saw there is no nice kotlin extensions. I tried to come up with something but I was not sure how to solve get calls to avoid getInt, getString, etc..

Any ideas?

I will work on the documentation next week, if someone thinks this might be a useful addition. If not, feel free to delete this PR ;)

Copy link
Member

@czyzby czyzby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be pretty useful, although I'm not sure if we want this as a separate module or in ktx-app.

As for the get operator, we could do it like this:

inline operator fun <reified T> Preferences.get(key: String): T =
  when(T::class.java) {
    String::class.java -> getString(key) as T
    Boolean::class.java -> getBoolean(key) as T
    // TODO Other types.
    else -> throw GdxRuntimeException("Unsupported class: ${T::class.java}")
  }

Although since Kotlin does not have union types, we cannot really limit T to supported preference types, hence the runtime exception. Maybe we could default to JSON parsing for other types to ease (de)serialization of JSON preferences? A similar set operator could be added as well.

// Read:
else -> Json().fromJson(T::class.java, getString(key))

// Write:
operator fun <T> Preferences.set(key: String, value: T): Preferences =
  this.putString(key, Json().toJson(value))

preferences/src/main/kotlin/ktx/preferences/preferences.kt Outdated Show resolved Hide resolved
preferences/README.md Outdated Show resolved Hide resolved
@czyzby
Copy link
Member

czyzby commented Mar 23, 2020

I've thought about this and I think this could be added as a separate module. If there are tiny modules with pretty general purpose utilities like ktx-log, I don't see why preference utilities couldn't be separated as well.

@Quillraven
Copy link
Contributor Author

Made a quick update on the PR with reified set and get. Also thought of a way to maybe use the pair syntax but did not come up with a very nice way :D

I also replaced the mock now with an implementation as you suggested.

Will update Readme by end of this week.

@czyzby
Copy link
Member

czyzby commented Mar 23, 2020

Overloads for set were fine, you can bring them back. Just leave one generic function for JSON serialization of properties. It's get that's problematic and needs the reified types.

preferences/src/main/kotlin/ktx/preferences/preferences.kt Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
preferences/src/main/kotlin/ktx/preferences/preferences.kt Outdated Show resolved Hide resolved
@czyzby czyzby added the preferences Issues of the ktx-preferences module label Mar 24, 2020
@czyzby czyzby added this to the 1.9.10 milestone Mar 24, 2020
@Quillraven
Copy link
Contributor Author

I have merged the develop branch now and resolved the CHANGELOG.md conflicts. Also, finished the documentation and everything else from my POV.

Let me know if there is anything to change :)

@czyzby
Copy link
Member

czyzby commented Mar 25, 2020

Thanks, I believe that's all as well. I'll do some minor refactoring and publish the module snapshot today. Thank you for your contribution!

@czyzby czyzby merged commit 81bbdbf into libktx:develop Mar 25, 2020
czyzby added a commit that referenced this pull request Mar 25, 2020
@czyzby
Copy link
Member

czyzby commented Mar 25, 2020

@Quillraven I ended up changing things a bit as I looked closely into the code and the tests:

  • get now returns a nullable value, since the preference might be missing. A variant with default value was added. The tests were expanded with examples of missing preferences.
  • Double values did not work as expected with get/set (they were treated as JSON-serializable objects). Since this is a pretty commonly used basic data type, I've added some special measures to inform the users that it's not supported by Preferences. I did not do the same for Short and Byte though.
  • I've split the code examples in the README into small sections.

You're welcome to review the changes.

@czyzby
Copy link
Member

czyzby commented Mar 25, 2020

@Quillraven I think there were quite a few useful utilities added since the last release, and this module is a major contribution. We might publish a new stable version this week. Let me know if you think something is missing or if you want to set up any more pull requests soon.

@czyzby czyzby mentioned this pull request Mar 25, 2020
3 tasks
@Quillraven
Copy link
Contributor Author

looks good to me and I like the way you tested the flush extension. I always learn something from your changes, thanks!

Found a small typo but other than that it looks great.

I have no plans for other PRs as I am very busy at work for the next couple of weeks haha.

Looking forward to the new release - thank you a lot!

@czyzby
Copy link
Member

czyzby commented Mar 25, 2020

Found a small typo but other than that it looks great.

Which file and line?

@Quillraven
Copy link
Contributor Author

Quillraven commented Mar 25, 2020 via email

czyzby added a commit that referenced this pull request Mar 25, 2020
@czyzby
Copy link
Member

czyzby commented Mar 25, 2020

@Quillraven Thanks, good catch. (Also, Intel probably hates me.)

@czyzby
Copy link
Member

czyzby commented Mar 25, 2020

@Quillraven By the way, snapshot release should already be out. Assuming you're using the preferences extensions in a personal project, I'd appreciate it if you switched to KTX and tested the API - especially around the JSON serialization. I'll probably do the release during the weekend.

@Quillraven
Copy link
Contributor Author

will work on the private project over the weekend and will update to ktx preferences.

If something comes up, I will let you know :)

czyzby added a commit that referenced this pull request Mar 25, 2020
@czyzby
Copy link
Member

czyzby commented Mar 26, 2020

@Quillraven Just wanted to let you know that I added some extra utilities from the Cyberpunk framework (#183), including taking screenshots, moving camera and profiling. Snapshots with the changes should already be out. If you have some free time for testing, I'd appreciate it if you looked through the code and tried it out in your project. :)

@Quillraven
Copy link
Contributor Author

@czyzby I updated now to the snapshot, kotlin 1.3.71 and gradle 6.3 (to use the exclusiveContent functionality for the snapshot ;) )

Looked into the Cyberpunk extensions but I cannot use any of them, sorry:

  1. Screenshot: sounds useful but I don't use screenshots and no plans to support that for the time being
  2. Profiling: same as above and my code is anyway perfectly optimized :P
  3. Camera: from what I can tell it would be nice to also support a version without Vector2 and simply pass targetX and targetY
  4. Text helpers: I wanted to use that for my floating texts but there is only a "String" version and I have a stringbuilder. Maybe there would be an option to also use stringbuilder? Second, and more important to me, it creates a Vector2 object. Since my texts are floating (=moving) I would create a vector2 object per frame. I would either need to redesign my code for this function or there should be an option to pass in a Vector2 object which will then contain the result.

This is just my personal opinion from my current project. I think in general they are useful extensions but at the moment I cannot use them, sorry.

@czyzby
Copy link
Member

czyzby commented Mar 27, 2020

@Quillraven Thanks for the feedback, it's actually very helpful.

By the way, I'm currently working on #182 (about time). I still have a bunch of tests to write and refactor, but the basic functionality is there. I should be done by tomorrow, unless there's some major bug that I'll have to fix. I've written some basic mock-up apps loading and using some assets, but if you're currently using AssetManager, I'd appreciate if you tested this one as well. I'll let you know when I'm done.

@Quillraven
Copy link
Contributor Author

sure just let me know. I am using assetmanager in the loading screen that actually loads all assets currently since it is still a small game and does not need load/unload mechanics.

@czyzby
Copy link
Member

czyzby commented Apr 11, 2020

@Quillraven Just wanted to let you know that 1.9.10-b5 was released with your preferences module and the new AssetStorage implementation.

@Quillraven
Copy link
Contributor Author

thank you! will update my project over the weekend :)

czyzby added a commit that referenced this pull request Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences Issues of the ktx-preferences module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants