Add "Show in Explorer" context menu for download queue #605
Conversation
exislow left a comment
exislow left a comment There was a problem hiding this comment.
I appreciate your effort to solve the issue. The idea is not bad but your GenAI model solved this somehow cumbersome. Please address next to my comments these general topics as well:
- Before committing something please run
make checkand resolve all errors and warnings. Do not usenoqastatements to avoid warnings and errors. - Document ALL of your code with propper docstrings!
- I recommend you to use Claude Sonnet 4.5 for better code quality and solution proposals.
- You put a lot of effort into instructing GenAI to re-create the path, where the downlaoded item was saved. But why? The path is known to the
downloadmethod. Please do not re-create the path, this is super error prone. Instead just emit (or so) the path from the download method to the GUI after the download has finished. This way, you reduce complexity and lines of code.
| Hi @exislow, I made some changes based on your feedback. The only thing left is to fix the false positives that Ruff is flagging so I can avoid using noqa:, especially for S603. If you have any ideas, I’d really appreciate your feedback. |
exislow left a comment
exislow left a comment There was a problem hiding this comment.
Okay, to be honest. I have stopped reviewing this at some point. In general said:
- You still have not document all your changes with proper docstrings.
- You still compute the path in the GUI for artists. I wouldn't recommend this. And especially this method would be a helper and shouldn't be stored in the GUI.
- Instead of keep proper type hints you remove them.
- You also remove proper docstrings
- Your code looks a little bit cumbersome. This idea can be realized way easier then that.
- A lot of functionality you need to realize this idea is already implemented. You do not need to re-implement more code, which basically does the same.
Try to put you in my place: The less code I need to review, the easier it gets for me.
What GenAI do you use?
tidal_dl_ng/model/gui_data.py Outdated
| quality_audio: Quality | ||
| quality_video: QualityVideo | ||
| obj: object | ||
| status_init: InitVar[str] = QueueDownloadStatus.Waiting |
There was a problem hiding this comment.
Why did you rename the variable. This is not good practice, since this class attribute not only shows the initial status but it is supposed to represent the overall status as well.
Also if you like to have thread safe setters in getters in Python please use propery decorators, which is a more pythonic way to do this: https://docs.python.org/3/library/functions.html#property
I would recommend to undo the renaming of this attribute.
tidal_dl_ng/gui.py Outdated
| def on_queue_download_item_finished(self, item: QtWidgets.QTreeWidgetItem) -> None: | ||
| def on_queue_download_item_finished(self, item: QtWidgets.QTreeWidgetItem, path: str) -> None: | ||
| """Update the status of a queue download item to 'Finished'. | ||
| Thread-safe implementation with proper path handling. |
There was a problem hiding this comment.
Unnecessary comment. A method docstring describes, what the method is doing, not how it is implemented.
Rikrdoga force-pushed the show-in-explorer branch from 9ae1d00 to dea23b1 Compare | Hi @exislow Apologies for the late response - work has been keeping me busy. I've refactored the implementation to make it simpler. Let me know what you think |
2 participants
This pull request introduces a new context menu option, "Show in Explorer," for completed items in the download queue. This allows users to quickly navigate to the location of their downloaded media.
This feature is implemented through a robust architectural refactor that ensures stability, thread-safety, and cross-platform compatibility.
Key Changes
Platform-Specific File Manager Logic:
A new FileSystemHelper class has been introduced to abstract all OS-specific logic for opening file explorers.
Implements robust handling for Windows, macOS, and Linux.
Uses os.startfile for directories and subprocess.Popen for files on Windows to ensure reliable operation.
Thread-Safe Path Storage:
The QueueDownloadItem dataclass (gui_data.py) is now thread-safe.
It uses a threading.Lock with set_file_path and get_file_path methods. This prevents race conditions between the download worker thread (writing the path) and the main GUI thread (reading the path).
Download Pipeline Path Resolution:
The download pipeline (on_queue_download and download) has been updated to correctly resolve and propagate the final, absolute file path for all media types.
Implements correct directory path resolution for Tracks, Albums, and Artists.
User Workflow
1.- A user adds one or more items (Track, Album, Artist) to the download queue.
2.-The items download and their status changes to "Finished."
3.-The user can now right-click any "Finished" item in the queue.
4.-A context menu appears with the option "Show in Explorer."
5.- Upon clicking, the user's native file manager (Explorer, Finder, etc.) opens.
If the downloaded item was a Track, the folder opens with the track file selected.
If the downloaded item was an Album or Artist, the main directory for that album or artist opens.
Closes #599