Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix setting game path for games with executable in subfolder. #1351

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

Holt59
Copy link
Member

@Holt59 Holt59 commented Jan 10, 2021

Fixes #1346

@Holt59 Holt59 requested a review from isanae January 10, 2021 19:59
@isanae
Copy link
Contributor

isanae commented Jan 11, 2021

That works fine, but I have two problems with it:

  1. update() is called after the dialog is closed and doesn't handle failure. Either put the code in an editingFinished handler for the text box (which sucks because it pops a dialog), or add a new checkValid() member function in SettingsTab that's called from SettingsDialog::exec() and stops the process if one returns false.

  2. The message is going to be confusing, some people will ask why you're trying to unzip the game folder. I'd say something really specific for cases where the binary name contains a directory, because it's not obvious that we're trying to find a parent folder that contains a specific path. You can also use a task dialog, put more info in the details and also log them.

@Holt59
Copy link
Member Author

Holt59 commented Jan 11, 2021

  1. update() is called after the dialog is closed and doesn't handle failure. Either put the code in an editingFinished handler for the text box (which sucks because it pops a dialog), or add a new checkValid() member function in SettingsTab that's called from SettingsDialog::exec() and stops the process if one returns false.

The game path edit is read-only so I just moved the code in the "click" event, and I only update the edit if the path is valid. I'm using a member to avoid recomputing the path to save it when saving the settings.

  1. The message is going to be confusing, some people will ask why you're trying to unzip the game folder. I'd say something really specific for cases where the binary name contains a directory, because it's not obvious that we're trying to find a parent folder that contains a specific path. You can also use a task dialog, put more info in the details and also log them.

I've changed the message, let me know what you think, I'm not really good at writing user warnings.

@Al12rs Al12rs added this to the 2.4.0alpha6 milestone Jan 13, 2021
@isanae
Copy link
Contributor

isanae commented Jan 13, 2021

Can you add a short comment explaining what the cdUp loop is doing and why it has to do it?

@Holt59
Copy link
Member Author

Holt59 commented Jan 13, 2021

Can you add a short comment explaining what the cdUp loop is doing and why it has to do it?

Done.

@isanae
Copy link
Contributor

isanae commented Jan 13, 2021

Looks good to me.

@Holt59 Holt59 merged commit c3334e7 into ModOrganizer2:master Jan 13, 2021
@Holt59 Holt59 deleted the fix-set-gamepath branch February 7, 2021 19:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect game path update for game plugins with executable in subfolders
3 participants