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

Disable setappimage if winebincode != AppImage #56

Merged
merged 6 commits into from
Feb 17, 2024

Conversation

thw26
Copy link
Collaborator

@thw26 thw26 commented Feb 10, 2024

This is a partial fix for #5. It adds conditional logic to the CLI, TUI, and GUI that either removes or disables a function if the config file states that the Logos install was created with something other than an AppImage.

It also creates a new GUI tooltip function. This is applied to the set appimage button, so that when the user hovers over it with his mouse, it will tell him why the button is disabled.

@thw26 thw26 requested a review from n8marti February 10, 2024 15:09
@thw26 thw26 mentioned this pull request Feb 10, 2024
14 tasks
@n8marti
Copy link
Collaborator

n8marti commented Feb 14, 2024

The general logic looks good.

I just started testing with the Tk dialog and ran into this:

Exception in Tkinter callback
Traceback (most recent call last):
  File "/opt/lib/python3.12/tkinter/__init__.py", line 1962, in __call__
    return self.func(*args)
           ^^^^^^^^^^^^^^^^
  File "/home/nate/g/LogosLinuxInstaller/gui_app.py", line 228, in on_wine_check_released
    self.gui.wine_dropdown['values'] = utils.get_wine_options(utils.find_appimage_files(), utils.find_wine_binary_files())
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nate/g/LogosLinuxInstaller/utils.py", line 757, in get_wine_options
    return sorted(wine_binary_options)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'PosixPath' and 'str'

It looks like the list wine_binary_options is a mix between strings and pathlib.Paths. It looks like utils.find_appimage_files() returns pathlib.Paths, while utils.find_wine_binary_files() returns strings. Both of those get passed into utils.get_wine_options(), so we either need to change what type one of them returns, or we need to covert it in utils.get_wine_options() to be the same as the other one.

@thw26
Copy link
Collaborator Author

thw26 commented Feb 15, 2024

It looks like the list wine_binary_options is a mix between strings and pathlib.Paths. It looks like utils.find_appimage_files() returns pathlib.Paths, while utils.find_wine_binary_files() returns strings. Both of those get passed into utils.get_wine_options(), so we either need to change what type one of them returns, or we need to covert it in utils.get_wine_options() to be the same as the other one.

Okay. That should be an easy fix; I'll mod find_appimage_files. If there is a need for strings, I'd rather run a tostring() function.

@thw26 thw26 force-pushed the issue5-conditional-appimage-buttons branch from e5c3254 to 0e3dea1 Compare February 16, 2024 06:42
@thw26
Copy link
Collaborator Author

thw26 commented Feb 16, 2024

I was running my system's default Python version, 3.11, and I hit an error in code. I was about to code around this until I checked docs and saw it was a 3.12 feature added. While I don't plan to do a full-blown version check for every feature, I added this for clarity's sake since the CLI reported an unexpected keyword error. Thus I added this commit: 229bcb8.

This code could easily be added as a top-level function like if root or if running, or even put into its own util function, to make it easier to call.

@thw26 thw26 force-pushed the issue5-conditional-appimage-buttons branch from 2da53b2 to 1649afc Compare February 16, 2024 07:23
@n8marti
Copy link
Collaborator

n8marti commented Feb 17, 2024

Exception in thread Thread-2 (get_logos_releases):
Traceback (most recent call last):
  File "/opt/lib/python3.12/threading.py", line 1073, in _bootstrap_inner
    self.run()
  File "/opt/lib/python3.12/threading.py", line 1010, in run
    self._target(*self._args, **self._kwargs)
  File "/home/nate/g/LogosLinuxInstaller/utils.py", line 702, in get_logos_releases
    filtered_releases = filter_versions(releases, 30, 1)
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nate/g/LogosLinuxInstaller/utils.py", line 671, in filter_versions
    return [version for version in versions if check_logos_release_version(version, threshold, check_version_part)]
                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nate/g/LogosLinuxInstaller/utils.py", line 667, in check_logos_release_version
    return version_parts[check_version_part - 1] < threshold
           ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'map' object is not subscriptable

So it apparently doesn't work to pluck an item by index from the map function object without first converting that into a list, in which case, I'd say just make it a list from the beginning. So L666 could be something like:

version_parts = [int(p) for p in version.split('.')]

or:

version_parts = list(map(int, version.split('.')))

I tested the speed of both of these, just out of curiosity. Here's what I found:

>>> import timeit
>>> timeit.timeit('list(map(int, "30.2.001".split(".")))', number=10000)
0.014728516049217433
>>> timeit.timeit('[int(p) for p in "30.2.001".split(".")]', number=10000)
0.011726382013875991

So the list comprehension option seems to be faster, in case you ever want to do this on a larger number of items.

@thw26
Copy link
Collaborator Author

thw26 commented Feb 17, 2024

Ah, I had simplified that code from an if/elif series/case statement to that. Sounds good. Will test a change and push.

Copy link
Collaborator

@n8marti n8marti left a comment

Choose a reason for hiding this comment

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

Testing a clean install for TUI and GUI both succeed. This looks good to me.

@thw26 thw26 force-pushed the issue5-conditional-appimage-buttons branch from 6b13fcd to fee433d Compare February 17, 2024 17:33
@thw26 thw26 merged commit 9f832c4 into main Feb 17, 2024
@thw26 thw26 deleted the issue5-conditional-appimage-buttons branch February 19, 2024 22:08
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.

None yet

2 participants