Update app id on edit by brad-richardson · Pull Request #670 · LizardByte/Sunshine

brad-richardson · 2023-01-02T03:17:08Z

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

Fixessunshine caches box images #551

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

I want maintainers to keep my branch updated

brad-richardson · 2023-01-02T03:25:12Z

@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

brad-richardson · 2023-01-02T03:28:03Z

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.

cgutman · 2023-01-02T04:58:15Z

+ // 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);


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.

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?

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

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?

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

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.

cgutman · 2023-01-02T05:18:24Z

Any ideas about why app list refreshes don't seem to load properly on the qt client?

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

I think those were to avoid using app ID 0.

ReenigneArcher · 2023-01-02T18:07:59Z

Can you also replace the ./src_assets/common/assets/box.png with this image? https://discord.com/channels/804382334370578482/1055290596304105572/1059455062243541082

brad-richardson · 2023-01-02T20:07:32Z

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

brad-richardson · 2023-01-02T20:33:23Z

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

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.

brad-richardson · 2023-01-02T03:17:08Z

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

Fixessunshine caches box images #551

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

I want maintainers to keep my branch updated

brad-richardson · 2023-01-02T03:25:12Z

@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

brad-richardson · 2023-01-02T03:28:03Z

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.

cgutman · 2023-01-02T04:58:15Z

+ // 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);


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.

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?

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

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?

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

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.

cgutman · 2023-01-02T05:18:24Z

Any ideas about why app list refreshes don't seem to load properly on the qt client?

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

I think those were to avoid using app ID 0.

ReenigneArcher · 2023-01-02T18:07:59Z

Can you also replace the ./src_assets/common/assets/box.png with this image? https://discord.com/channels/804382334370578482/1055290596304105572/1059455062243541082

brad-richardson · 2023-01-02T20:07:32Z

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

brad-richardson · 2023-01-02T20:33:23Z

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

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.

brad-richardson · 2023-01-02T03:17:08Z

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

Fixessunshine caches box images #551

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

I want maintainers to keep my branch updated

brad-richardson · 2023-01-02T03:25:12Z

@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

brad-richardson · 2023-01-02T03:28:03Z

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.

cgutman · 2023-01-02T04:58:15Z

+ // 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);


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.

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?

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

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?

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

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.

cgutman · 2023-01-02T05:18:24Z

Any ideas about why app list refreshes don't seem to load properly on the qt client?

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

I think those were to avoid using app ID 0.

ReenigneArcher · 2023-01-02T18:07:59Z

Can you also replace the ./src_assets/common/assets/box.png with this image? https://discord.com/channels/804382334370578482/1055290596304105572/1059455062243541082

brad-richardson · 2023-01-02T20:07:32Z

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

brad-richardson · 2023-01-02T20:33:23Z

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

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.

brad-richardson · 2023-01-02T03:17:08Z

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

Fixessunshine caches box images #551

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

I want maintainers to keep my branch updated

brad-richardson · 2023-01-02T03:25:12Z

@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

brad-richardson · 2023-01-02T03:28:03Z

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.

cgutman · 2023-01-02T04:58:15Z

+ // 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);


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.

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?

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

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?

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

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.

cgutman · 2023-01-02T05:18:24Z

Any ideas about why app list refreshes don't seem to load properly on the qt client?

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

I think those were to avoid using app ID 0.

ReenigneArcher · 2023-01-02T18:07:59Z

Can you also replace the ./src_assets/common/assets/box.png with this image? https://discord.com/channels/804382334370578482/1055290596304105572/1059455062243541082

brad-richardson · 2023-01-02T20:07:32Z

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

brad-richardson · 2023-01-02T20:33:23Z

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

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.

brad-richardson · 2023-01-02T03:17:08Z

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

Fixessunshine caches box images #551

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

I want maintainers to keep my branch updated

brad-richardson · 2023-01-02T03:25:12Z

@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

brad-richardson · 2023-01-02T03:28:03Z

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.

cgutman · 2023-01-02T04:58:15Z

+ // 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);


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.

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?

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

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?

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

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.

cgutman · 2023-01-02T05:18:24Z

Any ideas about why app list refreshes don't seem to load properly on the qt client?

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

I think those were to avoid using app ID 0.

ReenigneArcher · 2023-01-02T18:07:59Z

Can you also replace the ./src_assets/common/assets/box.png with this image? https://discord.com/channels/804382334370578482/1055290596304105572/1059455062243541082

brad-richardson · 2023-01-02T20:07:32Z

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

brad-richardson · 2023-01-02T20:33:23Z

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

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.

brad-richardson · 2023-01-02T03:17:08Z

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

Fixessunshine caches box images #551

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

I want maintainers to keep my branch updated

brad-richardson · 2023-01-02T03:25:12Z

@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

brad-richardson · 2023-01-02T03:28:03Z

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.

cgutman · 2023-01-02T04:58:15Z

+ // 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);


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.

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?

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

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?

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

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.

cgutman · 2023-01-02T05:18:24Z

Any ideas about why app list refreshes don't seem to load properly on the qt client?

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

I think those were to avoid using app ID 0.

ReenigneArcher · 2023-01-02T18:07:59Z

Can you also replace the ./src_assets/common/assets/box.png with this image? https://discord.com/channels/804382334370578482/1055290596304105572/1059455062243541082

brad-richardson · 2023-01-02T20:07:32Z

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

brad-richardson · 2023-01-02T20:33:23Z

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

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.

brad-richardson · 2023-01-02T03:17:08Z

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

Fixessunshine caches box images #551

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

I want maintainers to keep my branch updated

brad-richardson · 2023-01-02T03:25:12Z

@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

brad-richardson · 2023-01-02T03:28:03Z

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.

cgutman · 2023-01-02T04:58:15Z

+ // 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);


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.

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?

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

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?

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

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.

cgutman · 2023-01-02T05:18:24Z

Any ideas about why app list refreshes don't seem to load properly on the qt client?

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

I think those were to avoid using app ID 0.

ReenigneArcher · 2023-01-02T18:07:59Z

Can you also replace the ./src_assets/common/assets/box.png with this image? https://discord.com/channels/804382334370578482/1055290596304105572/1059455062243541082

brad-richardson · 2023-01-02T20:07:32Z

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

brad-richardson · 2023-01-02T20:33:23Z

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

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.

brad-richardson · 2023-01-02T03:17:08Z

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

Fixessunshine caches box images #551

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Dependency update (updates to dependencies)
Documentation update (changes to documentation)
Repository update (changes to repository files, e.g. .github/...)

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

I want maintainers to keep my branch updated

brad-richardson · 2023-01-02T03:25:12Z

@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

brad-richardson · 2023-01-02T03:28:03Z

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.

cgutman · 2023-01-02T04:58:15Z

+ // 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);


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.

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?

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

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?

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

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.

cgutman · 2023-01-02T05:18:24Z

Any ideas about why app list refreshes don't seem to load properly on the qt client?

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

I think those were to avoid using app ID 0.

ReenigneArcher · 2023-01-02T18:07:59Z

Can you also replace the ./src_assets/common/assets/box.png with this image? https://discord.com/channels/804382334370578482/1055290596304105572/1059455062243541082

brad-richardson · 2023-01-02T20:07:32Z

Are you relaunching the client? That should reliably update it (and it should poll the app list every 30 seconds after that).

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

brad-richardson · 2023-01-02T20:33:23Z

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

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.

brad-richardson force-pushed the update-app-id branch from 6291786 to 23d7497Compare January 2, 2023 03:18

cgutman reviewed Jan 2, 2023
View reviewed changes

ReenigneArcher force-pushed the update-app-id branch from 23d7497 to 557b46eCompare January 2, 2023 18:10

brad-richardson marked this pull request as ready for review January 2, 2023 20:34

brad-richardson commented Jan 2, 2023
View reviewed changes

Comment threadsrc/process.cpp

ReenigneArcher force-pushed the update-app-id branch from 97c315e to e61c527Compare January 2, 2023 21:09

ReenigneArcher mentioned this pull request Jan 2, 2023
update id on application edit #658
Closed
11 tasks

ReenigneArcherand others added 9 commits January 2, 2023 18:20

update id on application edit
0239726
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.

Import id, fixes
b1e78f1

PR fixes, updated default art
d05aa5b

Add rand to target source
01aab8a

Fix
1b244d7

Clang lint, default string fix
7b2d86e

Revert new box art
2ce5ed5

Clang format
1871776

Use signed int32
f589bca

ReenigneArcher force-pushed the update-app-id branch from e61c527 to f589bcaCompare January 2, 2023 23:20

brad-richardson added 3 commits January 2, 2023 19:42

Handle id collision
fe46ef1

Format
d54442c

Set include
76fcb36

ReenigneArcher merged commit 052297a into LizardByte:nightlyJan 3, 2023

brad-richardson force-pushed the update-app-id branch from 6291786 to 23d7497Compare January 2, 2023 03:18

cgutman reviewed Jan 2, 2023
View reviewed changes

ReenigneArcher force-pushed the update-app-id branch from 23d7497 to 557b46eCompare January 2, 2023 18:10

brad-richardson marked this pull request as ready for review January 2, 2023 20:34

brad-richardson commented Jan 2, 2023
View reviewed changes

Comment threadsrc/process.cpp

ReenigneArcher force-pushed the update-app-id branch from 97c315e to e61c527Compare January 2, 2023 21:09

ReenigneArcher mentioned this pull request Jan 2, 2023
update id on application edit #658
Closed
11 tasks

ReenigneArcherand others added 9 commits January 2, 2023 18:20

update id on application edit
0239726
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.

Import id, fixes
b1e78f1

PR fixes, updated default art
d05aa5b

Add rand to target source
01aab8a

Fix
1b244d7

Clang lint, default string fix
7b2d86e

Revert new box art
2ce5ed5

Clang format
1871776

Use signed int32
f589bca

ReenigneArcher force-pushed the update-app-id branch from e61c527 to f589bcaCompare January 2, 2023 23:20

brad-richardson added 3 commits January 2, 2023 19:42

Handle id collision
fe46ef1

Format
d54442c

Set include
76fcb36

ReenigneArcher merged commit 052297a into LizardByte:nightlyJan 3, 2023

brad-richardson force-pushed the update-app-id branch from 6291786 to 23d7497Compare January 2, 2023 03:18

cgutman reviewed Jan 2, 2023
View reviewed changes

ReenigneArcher force-pushed the update-app-id branch from 23d7497 to 557b46eCompare January 2, 2023 18:10

brad-richardson marked this pull request as ready for review January 2, 2023 20:34

brad-richardson commented Jan 2, 2023
View reviewed changes

Comment threadsrc/process.cpp

ReenigneArcher force-pushed the update-app-id branch from 97c315e to e61c527Compare January 2, 2023 21:09

ReenigneArcher mentioned this pull request Jan 2, 2023
update id on application edit #658
Closed
11 tasks

ReenigneArcherand others added 9 commits January 2, 2023 18:20

update id on application edit
0239726
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.

Import id, fixes
b1e78f1

PR fixes, updated default art
d05aa5b

Add rand to target source
01aab8a

Fix
1b244d7

Clang lint, default string fix
7b2d86e

Revert new box art
2ce5ed5

Clang format
1871776

Use signed int32
f589bca

ReenigneArcher force-pushed the update-app-id branch from e61c527 to f589bcaCompare January 2, 2023 23:20

brad-richardson added 3 commits January 2, 2023 19:42

Handle id collision
fe46ef1

Format
d54442c

Set include
76fcb36

ReenigneArcher merged commit 052297a into LizardByte:nightlyJan 3, 2023

brad-richardson force-pushed the update-app-id branch from 6291786 to 23d7497Compare January 2, 2023 03:18

cgutman reviewed Jan 2, 2023
View reviewed changes

ReenigneArcher force-pushed the update-app-id branch from 23d7497 to 557b46eCompare January 2, 2023 18:10

brad-richardson marked this pull request as ready for review January 2, 2023 20:34

brad-richardson commented Jan 2, 2023
View reviewed changes

Comment threadsrc/process.cpp

ReenigneArcher force-pushed the update-app-id branch from 97c315e to e61c527Compare January 2, 2023 21:09

ReenigneArcher mentioned this pull request Jan 2, 2023
update id on application edit #658
Closed
11 tasks

ReenigneArcherand others added 9 commits January 2, 2023 18:20

update id on application edit
0239726
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.

Import id, fixes
b1e78f1

PR fixes, updated default art
d05aa5b

Add rand to target source
01aab8a

Fix
1b244d7

Clang lint, default string fix
7b2d86e

Revert new box art
2ce5ed5

Clang format
1871776

Use signed int32
f589bca

ReenigneArcher force-pushed the update-app-id branch from e61c527 to f589bcaCompare January 2, 2023 23:20

brad-richardson added 3 commits January 2, 2023 19:42

Handle id collision
fe46ef1

Format
d54442c

Set include
76fcb36

ReenigneArcher merged commit 052297a into LizardByte:nightlyJan 3, 2023

brad-richardson force-pushed the update-app-id branch from 6291786 to 23d7497Compare January 2, 2023 03:18

cgutman reviewed Jan 2, 2023
View reviewed changes

ReenigneArcher force-pushed the update-app-id branch from 23d7497 to 557b46eCompare January 2, 2023 18:10

brad-richardson marked this pull request as ready for review January 2, 2023 20:34

brad-richardson commented Jan 2, 2023
View reviewed changes

Comment threadsrc/process.cpp

ReenigneArcher force-pushed the update-app-id branch from 97c315e to e61c527Compare January 2, 2023 21:09

ReenigneArcher mentioned this pull request Jan 2, 2023
update id on application edit #658
Closed
11 tasks

ReenigneArcherand others added 9 commits January 2, 2023 18:20

update id on application edit
0239726
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.

Import id, fixes
b1e78f1

PR fixes, updated default art
d05aa5b

Add rand to target source
01aab8a

Fix
1b244d7

Clang lint, default string fix
7b2d86e

Revert new box art
2ce5ed5

Clang format
1871776

Use signed int32
f589bca

ReenigneArcher force-pushed the update-app-id branch from e61c527 to f589bcaCompare January 2, 2023 23:20

brad-richardson added 3 commits January 2, 2023 19:42

Handle id collision
fe46ef1

Format
d54442c

Set include
76fcb36

ReenigneArcher merged commit 052297a into LizardByte:nightlyJan 3, 2023

brad-richardson force-pushed the update-app-id branch from 6291786 to 23d7497Compare January 2, 2023 03:18

cgutman reviewed Jan 2, 2023
View reviewed changes

ReenigneArcher force-pushed the update-app-id branch from 23d7497 to 557b46eCompare January 2, 2023 18:10

brad-richardson marked this pull request as ready for review January 2, 2023 20:34

brad-richardson commented Jan 2, 2023
View reviewed changes

Comment threadsrc/process.cpp

ReenigneArcher force-pushed the update-app-id branch from 97c315e to e61c527Compare January 2, 2023 21:09

ReenigneArcher mentioned this pull request Jan 2, 2023
update id on application edit #658
Closed
11 tasks

ReenigneArcherand others added 9 commits January 2, 2023 18:20

update id on application edit
0239726
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.

Import id, fixes
b1e78f1

PR fixes, updated default art
d05aa5b

Add rand to target source
01aab8a

Fix
1b244d7

Clang lint, default string fix
7b2d86e

Revert new box art
2ce5ed5

Clang format
1871776

Use signed int32
f589bca

ReenigneArcher force-pushed the update-app-id branch from e61c527 to f589bcaCompare January 2, 2023 23:20

brad-richardson added 3 commits January 2, 2023 19:42

Handle id collision
fe46ef1

Format
d54442c

Set include
76fcb36

ReenigneArcher merged commit 052297a into LizardByte:nightlyJan 3, 2023

brad-richardson force-pushed the update-app-id branch from 6291786 to 23d7497Compare January 2, 2023 03:18

cgutman reviewed Jan 2, 2023
View reviewed changes

ReenigneArcher force-pushed the update-app-id branch from 23d7497 to 557b46eCompare January 2, 2023 18:10

brad-richardson marked this pull request as ready for review January 2, 2023 20:34

brad-richardson commented Jan 2, 2023
View reviewed changes

Comment threadsrc/process.cpp

ReenigneArcher force-pushed the update-app-id branch from 97c315e to e61c527Compare January 2, 2023 21:09

ReenigneArcher mentioned this pull request Jan 2, 2023
update id on application edit #658
Closed
11 tasks

ReenigneArcherand others added 9 commits January 2, 2023 18:20

update id on application edit
0239726
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.

Import id, fixes
b1e78f1

PR fixes, updated default art
d05aa5b

Add rand to target source
01aab8a

Fix
1b244d7

Clang lint, default string fix
7b2d86e

Revert new box art
2ce5ed5

Clang format
1871776

Use signed int32
f589bca

ReenigneArcher force-pushed the update-app-id branch from e61c527 to f589bcaCompare January 2, 2023 23:20

brad-richardson added 3 commits January 2, 2023 19:42

Handle id collision
fe46ef1

Format
d54442c

Set include
76fcb36

ReenigneArcher merged commit 052297a into LizardByte:nightlyJan 3, 2023

brad-richardson force-pushed the update-app-id branch from 6291786 to 23d7497Compare January 2, 2023 03:18

cgutman reviewed Jan 2, 2023
View reviewed changes

ReenigneArcher force-pushed the update-app-id branch from 23d7497 to 557b46eCompare January 2, 2023 18:10

brad-richardson marked this pull request as ready for review January 2, 2023 20:34

brad-richardson commented Jan 2, 2023
View reviewed changes

Comment threadsrc/process.cpp

ReenigneArcher force-pushed the update-app-id branch from 97c315e to e61c527Compare January 2, 2023 21:09

ReenigneArcher mentioned this pull request Jan 2, 2023
update id on application edit #658
Closed
11 tasks

ReenigneArcherand others added 9 commits January 2, 2023 18:20

update id on application edit
0239726
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.

Import id, fixes
b1e78f1

PR fixes, updated default art
d05aa5b

Add rand to target source
01aab8a

Fix
1b244d7

Clang lint, default string fix
7b2d86e

Revert new box art
2ce5ed5

Clang format
1871776

Use signed int32
f589bca

ReenigneArcher force-pushed the update-app-id branch from e61c527 to f589bcaCompare January 2, 2023 23:20

brad-richardson added 3 commits January 2, 2023 19:42

Handle id collision
fe46ef1

Format
d54442c

Set include
76fcb36

ReenigneArcher merged commit 052297a into LizardByte:nightlyJan 3, 2023

Uh oh!

Uh oh!

Conversation

brad-richardson commented Jan 2, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Branch Updates

Uh oh!

brad-richardson commented Jan 2, 2023

Uh oh!

brad-richardson commented Jan 2, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cgutman commented Jan 2, 2023

Uh oh!

ReenigneArcher commented Jan 2, 2023

Uh oh!

brad-richardson commented Jan 2, 2023

Uh oh!

brad-richardson commented Jan 2, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

brad-richardson commented Jan 2, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshot

Issues Fixed or Closed

Type of Change

Checklist

Branch Updates

Uh oh!

brad-richardson commented Jan 2, 2023

Uh oh!

brad-richardson commented Jan 2, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cgutman commented Jan 2, 2023

brad-richardson commented Jan 2, 2023•
edited
Loading

brad-richardson commented Jan 2, 2023•
edited
Loading

brad-richardson commented Jan 2, 2023•
edited
Loading