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

Add application image #36

Merged
merged 17 commits into from Feb 6, 2022
Merged

Add application image #36

merged 17 commits into from Feb 6, 2022

Conversation

PapyKahan
Copy link
Contributor

Description

Application picture feature added.

Screenshot

image

image

Issues Fixed or Closed

N/A

Type of Change

  • Application image path into *_apps.json.

  • Modification of GFE appimage route to send the picture to the client.

  • A field has been added to the WebUI to configure image path.

  • 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)

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 documentation blocks for new or existing components

@github-actions
Copy link

Pull requests must be made to the nightly branch. Thanks.

@PapyKahan PapyKahan changed the base branch from master to nightly January 20, 2022 17:16
Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

Code reviewed, but not tested. Seems mostly okay at first glance. One small change requested.

Have you tested on Linux and Windows? Please check each box if you've tested.

  • Windows
  • Linux

sunshine/process.cpp Outdated Show resolved Hide resolved
@PapyKahan
Copy link
Contributor Author

PapyKahan commented Jan 23, 2022

  • Windows
  • Linux

I don't have a linux setup now. Currently building one.

@TheElixZammuto
Copy link
Contributor

Tested with Windows 11 and Moonlight-qt and Moonlight-ios with both JPG and PNG files, it works

@cgutman
Copy link
Contributor

cgutman commented Jan 24, 2022

I'd be careful returning non-PNG images. They might work but it's only by luck.

All current Moonlight clients assume the image they get back will be a PNG (like GFE does). They will cache the images locally as .png files, which may confuse the APIs consuming those files if they guess the image format based on the file extension.

Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

Based on comments by @cgutman let's force the image to be PNG.

Additionally there was a typo in the word extension wherever it appeared.

sunshine/process.h Outdated Show resolved Hide resolved
sunshine/process.cpp Outdated Show resolved Hide resolved
sunshine/process.cpp Outdated Show resolved Hide resolved
assets/web/apps.html Outdated Show resolved Hide resolved
sunshine/process.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

Reviewed.

sunshine/process.cpp Outdated Show resolved Hide resolved
ReenigneArcher
ReenigneArcher previously approved these changes Jan 25, 2022
@ReenigneArcher
Copy link
Member

@cfajardo can you take a look at this? A user in the moonlight discord was testing and couldn't get it to return the png or even the default image (on Windows). https://discord.com/channels/352065098472488960/667129596088418344/938115637627191417

I'm also going to try to setup a test machine later to test out this PR in both linux and windows.

@PapyKahan
Copy link
Contributor Author

@cfajardo can you take a look at this? A user in the moonlight discord was testing and couldn't get it to return the png or even the default image (on Windows). https://discord.com/channels/352065098472488960/667129596088418344/938115637627191417

I'm also going to try to setup a test machine later to test out this PR in both linux and windows.

Could you invite me to Discord channel ? I'm not able to access it.

@ReenigneArcher
Copy link
Member

@cfajardo can you take a look at this? A user in the moonlight discord was testing and couldn't get it to return the png or even the default image (on Windows). https://discord.com/channels/352065098472488960/667129596088418344/938115637627191417
I'm also going to try to setup a test machine later to test out this PR in both linux and windows.

Could you invite me to Discord channel ? I'm not able to access it.

https://moonlight-stream.org/discord

@PapyKahan
Copy link
Contributor Author

@cfajardo can you take a look at this? A user in the moonlight discord was testing and couldn't get it to return the png or even the default image (on Windows). https://discord.com/channels/352065098472488960/667129596088418344/938115637627191417
I'm also going to try to setup a test machine later to test out this PR in both linux and windows.

Could you invite me to Discord channel ? I'm not able to access it.

https://moonlight-stream.org/discord

Sunshine channel seems to be private. Am I right ?

@ReenigneArcher
Copy link
Member

@cfajardo No... I think you need to select your roles in https://discord.com/channels/352065098472488960/668086277639503874

Also, I can confirm I'm not seeing images on a windows host, haven't tested linux yet.

Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

Small typo in apps_linux.json

assets/apps_linux.json Outdated Show resolved Hide resolved
@ReenigneArcher ReenigneArcher dismissed their stale review February 2, 2022 23:36

Typo needs fixed.

assets/web/apps.html Show resolved Hide resolved
assets/apps_linux.json Show resolved Hide resolved
Copy link
Member

@ReenigneArcher ReenigneArcher left a comment

Choose a reason for hiding this comment

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

I appreciate your work on this PR and apologize for asking for a bunch of changes. One final change and then I'll merge the PR.

CMakeLists.txt Outdated Show resolved Hide resolved
@ReenigneArcher ReenigneArcher merged commit 067214a into LizardByte:nightly Feb 6, 2022
@PapyKahan PapyKahan deleted the add-application-image branch February 10, 2022 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants