GeoTecINIT / nativescript-task-dispatcher

NativeScript-based mobile background task scheduler and dispatcher
Apache License 2.0
18 stars 6 forks source link

Fix application settings timestamps #30

Closed matey97 closed 2 years ago

matey97 commented 3 years ago

This PR fixes a bug while storing planning timestamps on ApplicationSettings. The timestamps were stored using the setNumber method which, internally, for Android parses the number (i.e., timestamp) to a float type an storing the value in scientific notation losing precision on the last decimals. To fix the issue, the timestamps are now stored as strings.

agonper commented 3 years ago

Looks great! Many thanks @matey97 for noticing this and implementing a solution.

I'm wondering now. Do you think that calling flush twice can have any impact on performance? Perhaps it would be safer to have a commit() function to flush() the updates just once. What do you think?

Once this gets merged I'll publish it as v2.0.2

agonper commented 2 years ago

Ping @matey97 :)

matey97 commented 2 years ago

Pong @agonper 😉. I'm sorry, I totally forgot about this 😓.

I'm wondering now. Do you think that calling flush twice can have any impact on performance? Perhaps it would be safer to have a commit() function to flush() the updates just once. What do you think?

Well, it's not easy. Of course calling flush() two times will impact on the performance. The main issue here is that the NativeScript ApplicationSettings is doing a really bad wrapping on Android's SharedPreferences. ApplicationSettings uses a different instance of the SharedPreferences.Editor for each method, and from Android Docs:

Note that when two editors are modifying preferences at the same time, the last one to call apply wins.

Then, they are doing a, apply() after each operation. Apply() is asynchronous (in theory), so if an operation is done after another, there would be two editors, and there might be concurrency issues. Thats why I flush() after each operation, to ensure that the change is stored. But again, the flush() itself uses another SharedPreferences.Editor, so it might not be safe to use it along apply(). I don't really know what to do...

agonper commented 2 years ago

Hi @matey97. Don't worry, it's ok. This was not something urgent anyway :) (and I completely forgot about it too hehe)

I must admit that I'm not sure either. My only concern is that these two are sync operations and calling them twice could potentially impact the performance of the scheduler. So I don't have a clear answer :(

Perhaps we can leave it as it is now and, if we notice any negative impact, we can think of a workaround. Because, how about persisting both items in the same key? (using a serialized object or similar). Both of them are always stored and retrieved at the same time, perhaps it makes sense. What do you think?

matey97 commented 2 years ago

Yes, that migth be an option. The less apply() and commit() calls, the better. I'll implement that.

agonper commented 2 years ago

Perfect! Many thanks @matey97 :)