search

HereInPlainSight / gshade_installer

GShade Installer / Updater Bash Script for Linux
GNU General Public License v3.0
59 stars 18 forks source link

FFXIV wineprefix install changes #5

Closed Maia-Everett closed 3 years ago

Maia-Everett commented 3 years ago

This pull request improves support for autodetecting FFXIV in a WINEPREFIX.

HereInPlainSight commented 3 years ago

Hi there! You look miles above any experience I've got, so I apologize, I just wanted to ask something as someone who's mostly just a user playing at scripting. I threw together a branch that adapted your changes, and this is just a question so I can learn for the future. Assuming I didn't make any obvious mistakes in my branch, is there a reason I shouldn't seek to compact code like that, and avoid duplication?

I always try to do this, but I'm not sure if I'm just spinning my wheels over something that doesn't matter. The script already feels much bigger than I anticipated at over 800 lines, but I'm not sure I should be worrying about that like I do?

I'm happy to accept both of your pull requests, to be clear, I just want to know for the future, for my own knowledge, if I'm putting too much effort in the wrong direction, and if so, why.

Also, thank you for the pull requests in the first place. I hadn't thought about checking ~/.wine or using wine's own registry to find things.

Maia-Everett commented 3 years ago

I haven't tested them, but your edits look good to me at first glance! They also avoid code duplication.

One caveat, though: unlike my version, yours doesn't seem to prompt to use the dxgi method for a Wine Steam installation (like it does for native Steam). I myself have GShade set up with the dxgi method, as d3d11 interferes with the Steam overlay.

HereInPlainSight commented 3 years ago

Sorry -- got completely eaten for like a week (week and a half? Geez!) there. And I completely didn't realize I'd skipped that prompt, thanks for pointing it out! If my branch has got less code duplication (and since you don't seem to think I'm crazy for trying so hard to avoid it), I updated that and merged the branch in to cover both requests with credit for you. If you think both use-cases are sufficiently covered, you can close them, but if you think I'm in any way missing things, for either, please definitely point them out. :)

Maia-Everett commented 3 years ago

Thanks for the merge! I've closed both pull requests.