Update app id on edit #670
Conversation
6291786 to
23d7497
Compare
|
@cgutman Any ideas about why app list refreshes don't seem to load properly on the qt client? Also, thoughts on the off by ones I've removed here? I had a couple minor issues around current game state after the changes but I haven't been able to consistently reproduce |
|
The commit should fix the iOS scaling issue: brad-richardson/moonlight-ios@33b0307 It's taking a while to install xcode/configure my env so haven't tested it yet. |
src/confighttp.cpp
Outdated
| // Add a property named "id" to the inputTree with a value of the current timestamp | ||
| // Time is in seconds because some Moonlight clients cannot accept more than a 32-bit integer | ||
| // Milliseconds would be a better option | ||
| time_t id = time(nullptr); |
There was a problem hiding this comment.
time_t is 64-bit on modern systems to avoid the Y2038 issue. We should cast it to make sure it's actually 32-bit.
There was a problem hiding this comment.
That's my bad... I knew it would just be safe until 2038... but that would also assume the user has their clock set correctly.
I would really like to use milliseconds instead of seconds anyway, in the event someone is saving multiple apps with the upcoming API. The code I removed here got the value in milliseconds if we want to go back to that. https://github.com/LizardByte/Sunshine/compare/7ebe09223c41b16cb8feb421e64f0902e0a91353..21660ea083e217075102ce0bc9130c69bd358983
Using milliseconds probably over complicates this all though... so maybe stick with the time_t and have some logic to ensure that the id isn't already used?
There was a problem hiding this comment.
I've changed this to just be a random number given the upcoming API. I didn't add any guard logic here because it seems unnecessary. Let me know what you think
There was a problem hiding this comment.
I've changed this to just be a random number given the upcoming API. I didn't add any guard logic here because it seems unnecessary. Let me know what you think
My fear with using a random number is it's possible for them to repeat. I've seen cases where random functions are not really very random at all.
Here's a potential idea. Use an incrementing counter and store the counter as a separate property in the apps.json. Then just use that value and add 1 to it when assigning a new id?
There was a problem hiding this comment.
Given the small chance of collision with this generator vs potential bugs with something like a user-writable counter in a threaded context I would probably lean towards the former?
I could switch to a more precise timestamp with some truncation/casting but this felt a little safer to me
There was a problem hiding this comment.
How about random then, but check if the id is already used? I feel if we allow it to duplicate, it definitely will happen. I'm used to the SQL auto incrementing style that eliminates the chance of duplication.
Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).
I think those were to avoid using app ID 0. |
|
Can you also replace the |
23d7497 to
557b46e
Compare
Yes, I'm noticing the app list on the mac client doesn't seem to refresh at all (even when I add a new app). I'll keep digging, I may have just put it in a bad state |
|
I fixed the issue on the qt client, an ID above max signed 32-bit int was failing to parse. This PR should be ready now |
97c315e to
e61c527
Compare
When an application is saved, the current timestamp in milliseconds is added to as the application id. A timestamp was used instead of a random integer to prevent any chance of repeated values.
e61c527 to
f589bca
Compare
Description
Continuation of #658 to show new app art on edit, tested working on Moonlight QT/iOS.
Screenshot
Note that the scaling is wrong for this image on iOS, I'll make a separate change in the moonlight client to fix.
Issues Fixed or Closed
Type of Change
.github/...)Checklist
Branch Updates
LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.