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

[Steam-Deck] Always maximize Heroic if Steam-Deck gamemode #1522

Merged
merged 15 commits into from
Jul 10, 2022

Conversation

CommandMC
Copy link
Collaborator

@CommandMC CommandMC commented Jun 24, 2022

This checks the XDG_CURRENT_DESKTOP=gamescope environment variable to detect if Heroic is running in Steam-Deck gamemode and starts Heroic in fullscreen.


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@CommandMC CommandMC added the pr:testing This PR is in testing, don't merge. label Jun 24, 2022
@CommandMC CommandMC requested a review from Nocccer June 24, 2022 13:37
@CommandMC CommandMC self-assigned this Jun 24, 2022
@CommandMC
Copy link
Collaborator Author

One thing that needs further testing is if we really have access to /run/host/etc/os-release in Flatpak. I was able to access it just fine on my machine, but it would be great if some more people could verify it

@CommandMC
Copy link
Collaborator Author

After further testing, we can indeed not access that file on Flatpak.
I'm not quite sure what I have to change to make it accessible either. @flavioislima could you help out here?

@CommandMC CommandMC added pr:wip WIP, don't merge. and removed pr:testing This PR is in testing, don't merge. labels Jun 24, 2022
@CommandMC
Copy link
Collaborator Author

On another note, the code does work fine on AppImage. So it's really just that permission issue that's stopping this

Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

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

2 small issues. One is from my eyes necessary. The other is just a way i would write the code.

electron/main.ts Outdated Show resolved Hide resolved
electron/utils.ts Outdated Show resolved Hide resolved
@Nocccer
Copy link
Collaborator

Nocccer commented Jun 24, 2022

I would go with a window-size command line option, instead detecting if we are on steam deck. This works for everyone everywhere.

@CommandMC
Copy link
Collaborator Author

That would be a good addition, but I'd say having a more automatic option like this is also good

@flavioislima
Copy link
Member

Do you need to access /etc from the flatpak? That's not possible. I believe the only way this might work is by checking the kernel version, might be that Steam OS had something specific in the name. I'm not sure though.

Co-authored-by: Niklas <61798668+Nocccer@users.noreply.github.com>
@CommandMC
Copy link
Collaborator Author

Well, it's possible if we change the file access permissions. I just don't know which exact permission this is & where to change it

electron/utils.ts Outdated Show resolved Hide resolved
@flavioislima
Copy link
Member

Well, it's possible if we change the file access permissions. I just don't know which exact permission this is & where to change it

Wdym? Which file?
Flatpaks cannot see the /etc folder even if we give access to the host system. It's a blacklisted directory:
https://docs.flatpak.org/en/latest/sandbox-permissions.html

@CommandMC
Copy link
Collaborator Author

CommandMC commented Jun 24, 2022

Yes, but if we give it access, we can see it at /run/host/etc. I know this because the application that originally implemented this Steam Deck check is also distributed via Flatpak

Not quite sure where this is set, but --filesystem=/etc:ro should be enough to get the file to show up

@CommandMC
Copy link
Collaborator Author

CommandMC commented Jun 24, 2022

Hm, actually, it seems like there's just a /run/host/os-release file. Let me check if that's normal or if I screwed something up

@CommandMC
Copy link
Collaborator Author

CommandMC commented Jun 24, 2022

image
Looks like it's normal (search for os-release here). TIL

@CommandMC CommandMC requested a review from Nocccer June 24, 2022 17:46
@flavioislima
Copy link
Member

image
Looks like it's normal (search for os-release here). TIL

So maybe add the permission specifically to this file with full path.

@CommandMC
Copy link
Collaborator Author

CommandMC commented Jun 24, 2022

You don't need a permission to access that file, as far as I can tell
At least I just reset my permissions locally and it's still there + it's there on a clean install (on a testing VM)

To check for yourself:

flatpak run --command=bash com.heroicgameslauncher.hgl
cat /run/host/os-release

@CommandMC CommandMC added pr:ready-for-review Feature-complete, ready for the grind! :P and removed pr:wip WIP, don't merge. labels Jun 26, 2022
@JakobDev
Copy link
Contributor

It looks like /run/host/os-release is not available on older Flatpak versions, so make sure Heroic doesn't crash if it is not available.

@Nocccer
Copy link
Collaborator

Nocccer commented Jun 27, 2022

We don't need to check if we are on steam deck. What we need todo is see if process.env.inGameMode or what it is called is true. If so we set the windows resolution to the monitor resolution. You can get monitor resolution via electron. I will write a example later.

This will also start heroic in full screen for holoiso users or if someone hooked up a external monitor with a higher resolution

Maybe this idea works also for bigpicturemode

@Nocccer
Copy link
Collaborator

Nocccer commented Jun 27, 2022

I adapt @CommandMC solution and just maximize heroic if we run heroic from steam.
What do you think about that solution? Should we add a option to disable this behavior, if wanted?

@CommandMC
Copy link
Collaborator Author

CommandMC commented Jun 27, 2022

Had some ideas on how to clean this up
I really like just checking for just Steam instead of the Deck, not sure why I wanted to limit it to that

@CommandMC CommandMC changed the title Always maximize Heroic if a Steam Deck in Gaming Mode is detected Always maximize Heroic if it is running through Steam Jun 27, 2022
@Nocccer
Copy link
Collaborator

Nocccer commented Jun 27, 2022

Had some ideas on how to clean this up
I really like just checking for just Steam instead of the Deck, not sure why I wanted to limit it to that

Yes good changes.

Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

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

LGTM. I would still consider a option to disable/enable this behavior. Waht do you think ?

@Nocccer Nocccer changed the title Always maximize Heroic if it is running through Steam [General] Always maximize Heroic if it is running through Steam Jun 30, 2022
electron/main.ts Outdated Show resolved Hide resolved
electron/constants.ts Outdated Show resolved Hide resolved
@Nocccer Nocccer changed the title [General] Always maximize Heroic if it is running through Steam [Steam-Deck] Always maximize Heroic if Steam-Deck gamemode Jul 9, 2022
@Nocccer
Copy link
Collaborator

Nocccer commented Jul 9, 2022

I tested the appiamge on steam deck. It's working.

@CommandMC
Copy link
Collaborator Author

Well, can't approve since it's my PR as far as GitHub is concerned, but looks good!

Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

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

LGTM!

@Nocccer Nocccer merged commit 7cb93f7 into beta Jul 10, 2022
@Nocccer Nocccer deleted the feat/steamdeck-maximize branch July 10, 2022 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants