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

[UX] Built in widget to install components and fonts from Winetricks #3102

Merged
merged 9 commits into from
Oct 18, 2023

Conversation

arielj
Copy link
Collaborator

@arielj arielj commented Oct 1, 2023

This PR adds a search/install functionality for winetricks built-in into the Winetricks dialog.

This should make it easier for users to install things like vcrun, d3dcompiler, etc with simpler instructions (Winetricks GUI is not user friendly and too complex for what we usually use it).

Now we can point the users to: settings > winetricks > search "XYZ" > install > wait

The original GUI is still available with an Open Winetricks GUI button for more advanced usage if needed.

Some things I changed:

  • SearchBar now is more generic and added LibrarySearchBar and WinetricksSearchBar to reuse some code
  • ProgressDialog can now be extended to add extra content for specific dialog with progress
  • I'll add more comments in the diff

Things I didn't include in this PR:

  • uninstalling components/fonts (winetricks doesn't have any option to do so)
  • changing settings (we could add that, but would probably need some rephrasing on the copy)
  • optimizations like caching available components and installed ones for a given prefix (I want to get the thing working right first, we can improve it in the future)
  • sometimes the Winetricks dialog gets push to the back because of that weird issue we have of the Settings dialog being re-rendered, I didn't try to fix it here to not scope creep this PR
  • I have not tested the built-in virtual keyboard (I don't have the setup to test that currently, and I imagine it will not work properly), but the steamdeck keyboard should work just fine anyway
  • cancellation of the current install
winetricks.mp4

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)

@arielj arielj added the pr:ready-for-review Feature-complete, ready for the grind! :P label Oct 1, 2023
@arielj arielj requested review from a team, flavioislima, CommandMC, Nocccer and imLinguin and removed request for a team October 1, 2023 18:20
@@ -616,35 +616,6 @@ async function runWineCommandOnGame(
})
}

// Calls WineCFG or Winetricks. If is WineCFG, use the same binary as wine to launch it to dont update the prefix
ipcMain.handle('callTool', async (event, { tool, exe, appName, runner }) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved this into tools.tsx to stop adding more callbacks in main.ts, it's becoming too big and it feels like a big bag of everything

@@ -429,7 +432,7 @@ export const Winetricks = {
runner: Runner,
appName: string,
args: string[],
event?: Electron.IpcMainInvokeEvent
returnOutput = false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we didn't need the event for the messages to work so I removed that

I also added a new parameters to make the method return the output instead of sending messages, so we can capture the results of some commands

sendFrontendMessage('progressOfWinetricks', executeMessages)
progressUpdated = false
}
}, 1000)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is just removing the event, the formatter is changed too many lines

src/backend/tools.ts Outdated Show resolved Hide resolved
const { t } = useTranslation()
const navigate = useNavigate()

export default function SearchBar({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made this component more generic, customizable with props to extend it for both LibrarySearchBar and WinetricksSearchBar, most of the code I removed here is now in LibrarySearchBar

async function callTools(tool: Tool, exe?: string) {
if (tool === 'winetricks') {
setWinetricksRunning(true)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we now call winetricks from within the other dialog

@flavioislima
Copy link
Member

flavioislima commented Oct 8, 2023

Testing this one here and looks pretty good so far.
Somethings I think that can be improved, if not on this PR maybe on another iteration:

  • During the install, it could show which component it's being installed instead of just: installing component.
  • Would be nice to have a way of seeing which component is installed on the prefix, I know winetricks shows this information somewhere, if not on the prefix, in some config folder.
  • Instead of a SearchBar (which implies you know what winetricks provides) ideally we could have a screen/modal showing the list of all available components and which ones are installed or not.

Other than that it is a good improvement for what we have right now. :D

@flavioislima
Copy link
Member

  • Would be nice to have a way of seeing which component is installed on the prefix, I know winetricks shows this information somewhere, if not on the prefix, in some config folder.

I am blind, just saw this one is implemented already 👍🏽

@arielj
Copy link
Collaborator Author

arielj commented Oct 8, 2023

  • During the install, it could show which component it's being installed instead of just: installing component.
    I did this initially, but when you close the modal while something is being installed you lose that information and the only way I could think to solve that was to keep something in the GlobalState but it was kinda messy

I've been thinking about that a bit more and I think I have a better solution (basically, keep track of that info in the backend instead and push the tool name also along with the progress)

  • Instead of a SearchBar (which implies you know what winetricks provides) ideally we could have a screen/modal showing the list of all available components and which ones are installed or not.

I wasn't sure what to do here, cause there are A LOT of components and I don't know how to fit all that in the UI. Maybe we can have the search and a few most common listed? like some vcrun, d3dcompiler, corefonts, directx, etc. I feel like listing everything is going to be a UI mess.

One comment though is that if you can still open the original winetricks to see all the available options, my original idea for this is more like a shortcut to help people follow guides that already tell you what you need to search for.

@arielj
Copy link
Collaborator Author

arielj commented Oct 8, 2023

I updated the modal to show the name of the component that is being installed.

I'll leave the addition of some commonly installed components for a future iteration once we decide what makes sense to include in that list.

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.

Good stuff!
That is pretty useful and cleaner for people that knows that to install. ⚔️

@arielj arielj merged commit 69407be into main Oct 18, 2023
13 checks passed
@arielj arielj deleted the built-in-winetricks branch October 18, 2023 22:32
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.

2 participants