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

Add an Option to Redownload Winetricks #31

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Add an Option to Redownload Winetricks #31

merged 1 commit into from
Jan 5, 2024

Conversation

thw26
Copy link
Collaborator

@thw26 thw26 commented Jan 2, 2024

This commit fixes #4.

It also places the TUI app in a while loop and moves the quick launch code behind an if GUI check.

Depends on #29.

LogosLinuxInstaller.py Outdated Show resolved Hide resolved
LogosLinuxInstaller.py Outdated Show resolved Hide resolved
installer.py Outdated Show resolved Hide resolved
@n8marti
Copy link
Collaborator

n8marti commented Jan 3, 2024

I need to figure out how to place my comments so the correct lines are previewed with them. Sorry about the mismatches above. They're easier to follow if you open the "Files Changed" tab.

Maybe we need to think through the logic of main a little bit more at a bigger-picture level. The CLI options in some cases preselect choices that would be found later in the TUI or GUI (e.g. SKIP_FONTS), but in other cases they completely override the need for the UI (e.g. --run-winetricks). This is why I added the ACTIONS variable. It served to short-circuit the UIs when this kind of non-interface "action" was requested from the CLI. So my knee-jerk reaction is to say "just put it back", but then that would mess up being able to put the TUI menu into a loop, which is a good idea.

But maybe the if config.DIALOG is None or config.DIALOG == 'tk': block (L197-L201) could be moved up and out of the if config.GUI is False block, along with the "actions" block (L167-L180), then the if config.GUI is True|False parts could be reverted while the curses block could just start with:

if config.DIALOG == 'curses':
    while True:
        [...]

This was an elif statement originally. Would something like that work?

@thw26
Copy link
Collaborator Author

thw26 commented Jan 3, 2024

Thanks for the review. I will rebase and then try to fix up what you've shared.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 4, 2024

Rebased. Now need to review and fix up your comments.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 4, 2024

@n8marti, this should be ready for a new review.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 4, 2024

Also, perhaps we can do a voice call to rethink the optargs/logic of main. I will open a discussion for this, see #34.

LogosLinuxInstaller.py Outdated Show resolved Hide resolved
LogosLinuxInstaller.py Outdated Show resolved Hide resolved
@n8marti
Copy link
Collaborator

n8marti commented Jan 5, 2024

I think this PR is dependent on the outcome of #34, unless the scope is reduced to only add the new --get-winetricks and --run-winetricks options but not yet implement them.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 5, 2024

I think this PR is dependent on the outcome of #34, unless the scope is reduced to only add the new --get-winetricks and --run-winetricks options but not yet implement them.

Yes, agreed. I will push the syntax fix, and then we can discuss how we want to split optargs that are for runtime config and optargs that are actionable.

@thw26
Copy link
Collaborator Author

thw26 commented Jan 5, 2024

Squashed and pushed these modifications.

I will merge this change, then we can merge the reinstall dependencies code, then we can discuss how best to handle the variable optargs and the action optargs.

@thw26 thw26 merged commit 787b60a into main Jan 5, 2024
@thw26 thw26 deleted the issue4 branch January 5, 2024 13:49
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.

Add an option to reinstall winetricks
2 participants