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

[BigBoyBarney]

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

[FichteFoll]

Thanks for the changes.

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

Edit: Yes, I am blind.

[FichteFoll]

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.

[BigBoyBarney]

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?

[FichteFoll]

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

[BigBoyBarney]

Regarding the OSError message.

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

[FichteFoll]

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.)

[BigBoyBarney]

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

[FichteFoll]

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.

[z411]

Alternatively you could just re-raise the OSError as an EngineError and catch it outside, which is what all UIs do right now.

[BigBoyBarney]

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

[FichteFoll]

I implemented this as I described in my comments in #770, which supersedes this PR. Thanks for getting this started either way.