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

Implement menu options for opening the install file path and Wine prefix in the default file manager #4285

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

luluco250
Copy link

Implements #4037

This was a personal nitpick of mine after migrating to Heroic from Lutris since I often need to open the game folder or prefix (changing/installing mods, editing save files in the prefix etc).

I have added menu options for Browse Files and Browse Wine Prefix to game cards context menus on the game list page as well as in the three dot menu of the game details page.

When clicked they will open the default file manager using Electron's shell.openPath from the backend. The first option opens the install path of the game (the directory portion of the executable path) while the second option opens the path to the Wine prefix used by the game.

I've added checks to ensure the options only show up if the game is installed and if it's a non-native game for the browse prefix option.

I've added English entries for both options in the gamepage translation file so that it could be pushed, not sure if that was the correct way.

Let me know if there are any issues with the PR and thanks for your work on Heroic!

Screenshots

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)

@flavioislima
Copy link
Member

flavioislima commented Jan 16, 2025

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@luluco250
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@arielj
Copy link
Collaborator

arielj commented Jan 16, 2025

Sorry, to be sure, is there a reason to not use the options we already have by clicking the paths in the installed information? or is it that it's not easy to find? or at least to use the same backend calls

image

Also, some feedback, I don't know if we want to add more actions to the library card context menu, the idea for that menu is to have common actions that most users may need, most users don't need to open those paths in normal use of heroic

@luluco250
Copy link
Author

luluco250 commented Jan 17, 2025

@arielj My bad, I didn't even know you could click that "Installed Information" text, it doesn't look like a button to me at all.

I would still personally prefer having those options be more accessible though, I don't often open the game detail page when I'm launching games but I do mod games often so having quick access to the game folder is nice.

Maybe the current UI/UX is a little too obscure?

As for using the same backend APIs, sure, this is the first time I've looked at the code and I wasn't aware of the existing feature.

PS.: The pipeline is complaining about the lock file, probably due to pnpm install having modified it, could I just revert it to how it looks in main? I didn't add or remove any dependencies, just installed them after cloning the repository.

@arielj
Copy link
Collaborator

arielj commented Jan 17, 2025

I agree the links are hard to find, and it's not obvious they can be clicked, I think it's a good idea to have those links too in the 3-dots menu (the new design have those paths more visible, but it's still not obvious then can be clicked)

About the card context menu, I understand that it would be good for your use case, but I don't think that's a general use case (if we add that cause one user wants that, we have to add options for anything other users want). I think the argument there should be: is this something the average user needs to have quick access from the library card? without thinking about how one personally uses Heroic. I can see Play/Logs/Settings/Library modifications/Uninstall to be general use for example.

You should revert the package.yml file, yes, in general you don't want to include that file unless you are changing something that would explicitly change node packages.

@luluco250
Copy link
Author

@arielj Thanks for your feedback!

I get your point about not cluttering the context menu for game cards. It's a personal preference of mine, but having in the three dots menu is good enough to improve discoverability IMO.

I wonder if something can be improved about the look of the Installed Information menu, maybe shade it in a lighter color or have a V icon to imply it's a clickable dropdown?

So in summary, here's what I'm planning to do:

  • Browse Files and Browse Wine Prefix are available in the three dots menu.
  • The context menu will not be changed from how it is in main, so no extra options there.
  • Reuse backend code that already exists for the Installed Information dropdown for browsing paths.

Can I go ahead and make those changes?

PS.: I've also manually reverted the lockfile to the one in main and it seems to have fixed the pipeline issues.

@arielj
Copy link
Collaborator

arielj commented Jan 17, 2025

@arielj Thanks for your feedback!

I get your point about not cluttering the context menu for game cards. It's a personal preference of mine, but having in the three dots menu is good enough to improve discoverability IMO.

I wonder if something can be improved about the look of the Installed Information menu, maybe shade it in a lighter color or have a V icon to imply it's a clickable dropdown?

So in summary, here's what I'm planning to do:

  • Browse Files and Browse Wine Prefix are available in the three dots menu.
  • The context menu will not be changed from how it is in main, so no extra options there.
  • Reuse backend code that already exists for the Installed Information dropdown for browsing paths.

Can I go ahead and make those changes?

PS.: I've also manually reverted the lockfile to the one in main and it seems to have fixed the pipeline issues.

yes, those changes sound good

re: the look of installed information, the new design already solves that by displaying the information without having to click that line (you can try it in settings > advanced > experimental features > new design)

@luluco250
Copy link
Author

I just noticed a bug in the logic used to determine if the game has Wine btw:
image

It seems to stem from this part of the code (src/frontend/screens/Game/GamePage/components/InstalledInfo.tsx:108):

{!is.win && !is.native && (
  <>
    <div>
      <b>Wine:</b> {wineName}
    </div>
    {wineType === 'crossover' ? (
      <div>
        <b>{t2('setting.winecrossoverbottle', 'Bottle')}:</b>{' '}
        <div>{wineCrossoverBottle}</div>
      </div>
    ) : (
      <div
        className="clickable"
        onClick={() => window.api.openFolder(winePrefix)}
      >
        <b>{t2('setting.wineprefix', 'WinePrefix')}:</b>{' '}
        <div className="truncatedPath">{winePrefix}</div>
      </div>
    )}
  </>
)}

I think I can fix that, I got it to work on the Browse Wine Prefix though I did it in a more roundabout way:

function isNativeGame(
  gameInfo: GameInfo,
  platform: NodeJS.Platform | 'unknown'
) {
  const isInstallPlatformWindows =
    gameInfo.install.platform?.toLowerCase().startsWith('win') ?? false
  return (
    platform === 'win32' ||
    !isInstallPlatformWindows ||
    ((platform === 'linux') === gameInfo.is_linux_native &&
      (platform === 'darwin') === gameInfo.is_mac_native)
  )
}

That aside I'm not sure why it's reporting the existence of a Wine prefix, there is no such folder in my prefixes folder which can be verified by the fact an error is thrown when I click it:
image

So maybe it's safe to just fix the condition that checks for the presence of Wine and there's no other deep rooted issue with it?

@arielj
Copy link
Collaborator

arielj commented Jan 17, 2025

I'm not sure I understand what's the issue exactly, the prefix path is wrong? or you are saying it shouldn't report that's using wine and it should be native? I think I'm missing some details or context

@luluco250
Copy link
Author

luluco250 commented Jan 17, 2025

It's reporting the presence of a Wine prefix for a native Linux game that I installed on Heroic manually (sideload runner I think).

I should note that the is_linux_native and is_mac_native properties of GameInfo don't seem to exist for games that use the sideload runner and it makes checking for Wine more complicated.

@arielj
Copy link
Collaborator

arielj commented Jan 17, 2025

ok so the issue is the value of is.native, that's happening here https://github.com/Heroic-Games-Launcher/HeroicGamesLauncher/blob/main/src/frontend/screens/Game/GamePage/index.tsx#L323

so I'm guessing the issue is with the isLinuxNative variable a few lines above

EDIT: I think the issue is installPlatform === 'linux', in sideloaded games it looks like it should be Linux with capital L

@luluco250
Copy link
Author

luluco250 commented Jan 17, 2025

I'm assuming the same issue could affect macOS due to the missing properties,

Here's what the gameInfo for the game looks like:
image

I've noticed that gameInfo.install.platform is not consistent, I've seen Windows, windows, Win32 and Linux, hence the slightly odd check I used in my code:

gameInfo.install.platform?.toLowerCase().startsWith('win') ?? false

This obviously assumes that any install platform that doesn't look like Windows is native.

@arielj
Copy link
Collaborator

arielj commented Jan 17, 2025

You can do const isLinuxNative = installPlatform?.toLocaleLowerCase() === 'linux' for now I guess, fixing the installPlatform is probably out of scope for this

@luluco250
Copy link
Author

I guess a better check can be implemented to cover all the possible install platforms, given the InstallPlatform type:
image
image
image

So the full range of platform strings that could appear are Windows, windows, Win32, Linux, linux, Mac, osx and Browser.

Quite tricky in how inconsistent it is, but we can cover our bases properly by just checking thoroughly.

@arielj
Copy link
Collaborator

arielj commented Jan 17, 2025

ideally, we want to make sure that the installPlatform property is normalized at some point so we don't need to know the different possible values

The code is already checking ['osx', 'Mac'].includes(installPlatform ?? '') so we could have ['linux', 'Linux'].includes(installPlatform ?? '') for now to address this problem

I don't know what's the best solution for the whole app, I'd prefer to do something like: when reading the information of the games from disk, we normalize the value, then from that point it's all win linux mac (to say something)

but I think fixing that should be a different issue and a different PR

@luluco250
Copy link
Author

Updated the branch, removing redundant API methods as well as the isNativeGame helper function, instead relying on is.win and is.native from GameContext while fixing the check for native Linux games.

Removed the menu options from the game card context menu, now they show up only on the game details page's submenu.

@arielj arielj mentioned this pull request Jan 18, 2025
@@ -233,6 +233,24 @@ export default function GamesSubmenu({
isInstalled &&
!isThirdPartyManaged

const hasWine = !is.win && !is.native
Copy link
Collaborator

@arielj arielj Jan 22, 2025

Choose a reason for hiding this comment

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

We also need to check that gameSettings.wineVersion.type !== "crossover" cause in that case it uses a crossover bottle and not a prefix

same check is done on the Installed Information paths

Copy link
Author

Choose a reason for hiding this comment

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

In that case would it be appropriate to rename the option to Browse CrossOver Bottle when we're dealing with a game that uses CrossOver? Referencing this string:

"winecrossoverbottle": "CrossOver Bottle"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think users need that, bottles should be handle with Crossover's UI so it's better to not let the user mess with the bottle directly

const hasWine = !is.win && !is.native

const onBrowseFiles = useCallback(() => {
const path = gameInfo.install.install_path || gameInfo.folder_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if gameInfo.folder_name would help here as a fallback, this is just the folder name, not a path that can be opened

Copy link
Author

Choose a reason for hiding this comment

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

I've copied the way it seems to be done here:

const appLocation = install_path || folder_name

Is this incorrect?

Copy link
Collaborator

@arielj arielj Jan 22, 2025

Choose a reason for hiding this comment

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

hmmm that's weird cause we can't really open just a folder name, it wouldn't be a path, maybe when that was first implemented it was not meant to be clicked, I don't really know, I do think it won't work (you can try it, just remove the gameInfo.instlal.install_path || part of that assignment and click the Browse Files link, I imagine it won't open at all, and if that's the case we can ignore the folder_name fallback)

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.

3 participants