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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3e31d29
Initial unification of open-folder command.
BigBoyBarney Jan 12, 2024
dd4b7cc
Fixed CLI open-folder
BigBoyBarney Jan 12, 2024
183a803
Fixed CLI out of range ID call
BigBoyBarney Jan 12, 2024
f031c43
Simplified errors
BigBoyBarney Jan 12, 2024
9a61d28
Raise errors when folders change during runtime
BigBoyBarney Jan 12, 2024
1d9477a
Added clearer OSError message
BigBoyBarney Jan 12, 2024
b0e4821
Added comments
BigBoyBarney Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 2 additions & 15 deletions trackma/ui/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,21 +531,8 @@ def do_openfolder(self, args):
"""

try:
show = self._get_show(args[0])
filename = self.engine.get_episode_path(show)
with open(os.devnull, 'wb') as DEVNULL:
if sys.platform == 'darwin':
subprocess.Popen(["open",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
elif sys.platform == 'win32':
subprocess.Popen(["explorer",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
else:
subprocess.Popen(["/usr/bin/xdg-open",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
except OSError:
# xdg-open failed.
self.display_error("Could not open folder.")
show_id = self._get_show(args[0])['id']
utils.open_folder(self.engine, show_id, error_callback=self.display_error)
except utils.TrackmaError as e:
self.display_error(e)

Expand Down
22 changes: 1 addition & 21 deletions trackma/ui/curses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,27 +365,7 @@ def do_play(self):

def do_openfolder(self):
item = self._get_selected_item()

try:
show = self.engine.get_show_info(item.showid)
filename = self.engine.get_episode_path(show)
with open(os.devnull, 'wb') as DEVNULL:
if sys.platform == 'darwin':
subprocess.Popen(["open",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
elif sys.platform == 'win32':
subprocess.Popen(["explorer",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
else:
subprocess.Popen(["/usr/bin/xdg-open",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
except OSError:
# xdg-open failed.
raise utils.EngineError("Could not open folder.")

except utils.EngineError:
# Show not in library.
self.error("No folder found.")
utils.open_folder(self.engine, item.showid, error_callback=self.error)

def do_play_random(self):
try:
Expand Down
24 changes: 1 addition & 23 deletions trackma/ui/gtk/window.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,29 +573,7 @@ def _open_website(self, show_id):
Gtk.show_uri(None, show['url'], Gdk.CURRENT_TIME)

def _open_folder(self, show_id):
show = self._engine.get_show_info(show_id)
try:
filename = self._engine.get_episode_path(show)
with open(os.devnull, 'wb') as DEVNULL:
if sys.platform == 'darwin':
subprocess.Popen(["open", os.path.dirname(filename)],
stdout=DEVNULL,
stderr=DEVNULL)
elif sys.platform == 'win32':
subprocess.Popen(["explorer", os.path.dirname(filename)],
stdout=DEVNULL,
stderr=DEVNULL)
else:
subprocess.Popen(["/usr/bin/xdg-open", os.path.dirname(filename)],
stdout=DEVNULL,
stderr=DEVNULL)
except OSError:
# xdg-open failed.
raise utils.EngineError("Could not open folder.")

except utils.EngineError:
# Show not in library.
self._error_dialog_idle("No folder found.")
utils.open_folder(self._engine, show_id, error_callback=self._error_dialog_idle)

def _copy_title(self, show_id):
show = self._engine.get_show_info(show_id)
Expand Down
21 changes: 1 addition & 20 deletions trackma/ui/qt/mainwindow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1183,26 +1183,7 @@ def s_altname(self):
self.ws_changed_show(show, altname=new_altname)

def s_open_folder(self):
show = self.worker.engine.get_show_info(self.selected_show_id)
try:
filename = self.worker.engine.get_episode_path(show)
with open(os.devnull, 'wb') as DEVNULL:
if sys.platform == 'darwin':
subprocess.Popen(["open",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
elif sys.platform == 'win32':
subprocess.Popen(["explorer",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
else:
subprocess.Popen(["/usr/bin/xdg-open",
os.path.dirname(filename)], stdout=DEVNULL, stderr=DEVNULL)
except OSError:
# xdg-open failed.
raise utils.EngineError("Could not open folder.")

except utils.EngineError:
# Show not in library.
self.error("No folder found.")
utils.open_folder(self.worker.engine, self.selected_show_id, error_callback=self.error)

def s_retrieve(self):
queue = self.worker.engine.get_queue()
Expand Down
47 changes: 47 additions & 0 deletions trackma/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,53 @@ def get_terminal_size(fd=1):
return hw


# Default 'open folder' function
def open_folder(engine, show_id, error_callback=None):
"""
Tries to open the folder containing the show of the passed ID.
Takes an optional 'error_callback' parameter that lets the invoker specify an error callback method.
If no error callback is passed, it defaults to simply raising.
"""

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:
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
Comment on lines +527 to +537
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

@BigBoyBarney BigBoyBarney Feb 9, 2024 •
edited
Loading

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 •
edited
Loading

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

@BigBoyBarney BigBoyBarney Feb 9, 2024 •
edited
Loading

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

@FichteFoll FichteFoll Feb 21, 2024 •
edited
Loading

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

@BigBoyBarney BigBoyBarney Mar 2, 2024 •
edited
Loading

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 •
edited
Loading

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.



def show():
return {
'id': 0,
Expand Down