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

Unification of 'open-folder' function, should fix 'xdg-open' path error #727

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

BigBoyBarney
Copy link

@BigBoyBarney BigBoyBarney commented Jan 12, 2024

Should fix #726 as long as xdg-open is in $PATH, which can be assumed for every distro. I tested it for GTK, Qt and CLI on Fedora 39.

Caution

As my other PR (#722), this requires python 3.10. Do not merge before #722 or without updating python dep.

Changes

Code

  • Unified all 'open folder' functions in utils.py. Can take an error_callback param so error reporting can still be done uniquely for every UI.
  • Fixed OSError not being raised when the folder was in the library, but was changed after scanning. I.e:
    1. Start trackma
    2. 'Show X' gets scanned and found
    3. Rename folder of 'show X'
    4. Open folder
    5. Error is raised correctly

- Moved main open function to `utils.py`
- Changed `usr/bin/xdg-open` to `xdg-open`
- Now correctly passes ID
- OSError is raised even if the folder was initially successfully read into the librbary, but is no longer available during runtime
Copy link
Collaborator

@FichteFoll FichteFoll left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

Which part of this PR requires Python 3.10? I'm not seeing it …

Edit: Yes, I am blind.

Comment on lines +527 to +537
except OSError as e:
if error_callback:
error_callback(f'Could not open folder.\n{e}')
else:
raise e

except Exception as e:
if error_callback:
error_callback(e)
else:
raise e
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an anti-pattern. Either the open_folder function should handle errors on its own, wrap errors with custom errors or let them bubble, but making this depend on a callback parameter is an unnecessary level of indirection.

With respect to the function providing some custom handling for annotating OSErrors with "Could not open folder", I suggest to wrap only OSErrors with a custom exception and otherwise let the callers handle the exceptions with their respective UI options.

Copy link
Author

Choose a reason for hiding this comment

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

I initially didn't add the string for the error callback, but it was in the original report so I added it back. I think it might have been due to how the CLI handled errors. I can't remember exactly, but I'm fairly sure there was a reason for it. (I re-added it in this commit)

Either the open_folder function should handle errors on its own, wrap errors with custom errors or let them bubble, but making this depend on a callback parameter is an unnecessary level of indirection.

I'm not sure what you mean by this. Previously, each UI had its own entire open_folder equivalent, where the error handling was done. How would you handle errors for each UI from utils.py alone without the error_callback?

Copy link
Collaborator

@FichteFoll FichteFoll Feb 9, 2024

Choose a reason for hiding this comment

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

try: open_foolder(…); catch: …
Or to answer your question: You wouldn't handle it in utils.py.

Copy link
Author

Choose a reason for hiding this comment

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

Regarding the OSError message.
image
I added the string for clarity, but it's not really necessary if you're opposed to it.

If you want to do it through try: catch: then you'd have to add that everywhere, where you call open_folder(). Since the function is super simple and its usage doesn't vary in the slightest, using the callback function saves time. I can remove if it and wrap the calls in try: if you prefer that though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrapping the error is a valid use case (that I also mentioned in my initial comment). It shouldn't depend on whether an error callback was provided, however, since that's what we have try-catch for. Please just remove the callback parameter, raise a new exception (with the added context) and handle thown exceptions at the call site.

(In general, I prefer error handling like it is provided by Rust (or even Golang to some extent), but Python has exceptions and we should use them as they are intended since the rest of the ecosystem is built around them.)

Copy link
Author

Choose a reason for hiding this comment

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

Just to make sure I understand what you mean.

Like this, for all UIs?
in utils.py

def open_folder(engine, show_id):
    """
    Open the folder containing the show of the passed ID.
    """

    try:
        show_to_open = engine.get_show_info(show_id)
        filename = engine.get_episode_path(show_to_open)

        with open(os.devnull, 'wb') as DEVNULL:
            command = []

            match sys.platform:
                case 'darwin':
                    command = ["open", os.path.dirname(filename)]
                case 'win32':
                    command = ["explorer", os.path.dirname(filename)]
                case _:
                    command = ["xdg-open", os.path.dirname(filename)]

            process = subprocess.Popen(command,
                                       stdout=DEVNULL,
                                       stderr=subprocess.PIPE,
                                       universal_newlines=True)

            _, stderr = process.communicate()

            if process.returncode != 0:
                # Command failed.
                raise OSError(stderr)

    except OSError as e:
        raise OSError(f'Could not open folder: {e}')

    except Exception as e:
        raise RuntimeError(f'An error occurred while trying to open the folder: {e}')

in (gtk) window.py

    def _open_folder(self, show_id):
        try:
            utils.open_folder(self._engine, show_id)
        except Exception as e:
            self._error_dialog_idle(e)

Or do you want me to create a new

class FolderError(TrackmaError):
    pass

Copy link
Collaborator

@FichteFoll FichteFoll Mar 3, 2024

Choose a reason for hiding this comment

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

Yes, that is basically what I mean.

Either would be fine imo, but a custom OpenFolderError(TrackmaError) would be most appropriate since we're interpreting the underlying errors already when re-throwing a wrapped exception. Additionally, you could directly throw a OpenFolderError when the subprocess's return code is 0.

@BigBoyBarney
Copy link
Author

BigBoyBarney commented Feb 9, 2024

Thanks for the changes.

Which part of this PR requires Python 3.10? I'm not seeing it …

match sys.platform:
            case 'darwin':
                command = ["open", os.path.dirname(filename)]
            case 'win32':
                command = ["explorer", os.path.dirname(filename)]
            case _:
                command = ["xdg-open", os.path.dirname(filename)]

Match was introduced in Python 3.10

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.

"Open containing folder" action uses a hard-coded path for xdg-open
2 participants