PICARD-2743: Add support for custom post tagging actions #374
Conversation
| PLUGIN_API_VERSIONS = ["2.10", "2.11"] | ||
| PLUGIN_LICENSE = "GPL-2.0" | ||
| PLUGIN_LICENSE_URL = "https://www.gnu.org/licenses/gpl-2.0.html" | ||
| PLUGIN_USER_GUIDE_URL = "https://github.com/twodoorcoupe/picard-plugins/tree/user_guides/user_guides/post_tagging_actions/guide.md" |
There was a problem hiding this comment.
I note that the documentation is NOT included in this PR - and IMO it is essential that it should be included in order to ensure that the availability and versioning of the documentation matches the plugin.
Also you are creating a new top-level sub-directory and have chosen user_guides. I would personally prefer to see this being docs as this seems to me to be a bit of a defacto standard - however others may think differently.
When you start to include this in the PR I think this URL will need to assume that this PR has been merged.
There was a problem hiding this comment.
That makes sense. I also saw other plugins like Additional Artist Details and Format Performer Tags have their docs included in the plugin's folder, so I did the same rather than creating a new top-level sub-directory.
| # run again. Do not edit this file unless you know what you are doing. | ||
|
|
||
|
|
||
| from PyQt5 import QtCore, QtGui, QtWidgets |
There was a problem hiding this comment.
Codacy error - unused QtGui reference.
| """ | ||
| self.action_options = [] | ||
| loaded_options = zip(*[config.setting[name] for name in OPTIONS]) | ||
| for options in loaded_options: |
There was a problem hiding this comment.
Why not iterate on for name in OPTIONS rather than create a new object?
There was a problem hiding this comment.
I think I used a confusing name for options and loaded_options here. I iterate over the options for each "action" rather than each option. Each options is a tuple made of all the options for that action (command, wait_for_exit, etc...), so I need to use the zip function to do this. I changed the name of both options and loaded_options to be hopefully less confusing.
|
|
||
| # Settings. | ||
| CANCEL = "pta_cancel" | ||
| OPTIONS = ["pta_command", "pta_wait_for_exit", "pta_execute_for_tracks", "pta_refresh_tags"] |
There was a problem hiding this comment.
Would this be better as an immutable set (i.e. curly brackets) like the ALBUM_SPECIAL_VARIABLES above?
There was a problem hiding this comment.
The order of the options in this list is the same as the order of the columns of the table in the options page. I use the order when saving or loading the options. This way I can just iterate over the columns without having to explicitly save or load the values in each column.
There was a problem hiding this comment.
If set doesn't preserve order, this is a sound reason.
| loaded_options = zip(*[config.setting[name] for name in OPTIONS]) | ||
| for options in loaded_options: | ||
| command = options[0] | ||
| other_options = [eval(option) for option in options[1:]] |
There was a problem hiding this comment.
The options here seem to be booleans, so avoiding use of eval and the consequent Codacy waarning shouldn't be difficult.
But if use of eval cannot be avoided, if you are able to annotate this to avoid this Codacy warning that would be helpful.
| """ | ||
| variables = [variable[1:-1] for variable in re.findall(r'%.*?%', command)] | ||
| variables = [parser.normalize_tagname(variable) for variable in variables] | ||
| command = re.sub(r'%.*?%', '{}', command) |
There was a problem hiding this comment.
Would it make more sense to create a pre-compiled regex as a pre-initialised object?
| PLUGIN_API_VERSIONS = ["2.10", "2.11"] | ||
| PLUGIN_LICENSE = "GPL-2.0" | ||
| PLUGIN_LICENSE_URL = "https://www.gnu.org/licenses/gpl-2.0.html" | ||
| PLUGIN_USER_GUIDE_URL = "https://github.com/twodoorcoupe/picard-plugins/tree/user_guides/user_guides/post_tagging_actions/guide.md" |
There was a problem hiding this comment.
On this documentation page, I am not sure that the COV integration tool is a good example for this plugin even though it is a valid example - because any Cover Art integration would ideally be done by creating a Cover Art plugin.
| """Finds the variables in the command and adds the options to the action options list. | ||
| """ | ||
| variables = [variable[1:-1] for variable in re.findall(r'%.*?%', command)] | ||
| variables = [parser.normalize_tagname(variable) for variable in variables] |
There was a problem hiding this comment.
variables = [parser.normalize_tagname(variable[1:-1]) for variable in re.findall(r'%.*?%', command)]?
| """ | ||
|
|
||
| def __init__(self): | ||
| self.action_thread_pool = futures.ThreadPoolExecutor() |
There was a problem hiding this comment.
According to the Python futures documentation:
If
max_workersis None or not given, it will default to the number of processors on the machine, multiplied by 5
So this is going to be the number of subprocesses that are run - and depending on the commands the user is choosing to run and the specifications of their PC this may be fine or it may cause major issues.
I think there needs to be a UI option with a sensible default to set the max-workers for this thread-pool.
|
Thank you @Sophist-UK! I made the number of worker threads an option and made it so that the number of pending requests is displayed in the status bar like you asked. |
There was a problem hiding this comment.
Thank you for taking the time to make all these changes.
Only a few follow-on comments.
I haven't tested this - nor am I likely to have time to do so. And in any case I can't approve this PR. So perhaps @zas or @outsidecontext could test this?
| {"pending_requests": action_queue.qsize()}, | ||
| timeout = 3000 | ||
| ) | ||
|
|
There was a problem hiding this comment.
This wasn't what I had in mind (which was to increment/decrement Picard's existing outstanding requests value), and I have no idea whether it will cause any issues (threading, excessive overhead etc.).
There was a problem hiding this comment.
Ah, sorry for misinterpreting. I think that value is used for webservice requests. Would it make sense to use it also for pending actions? For example, if I have some action running in the background and I load a new album, then the number of pending requests would include both?
There was a problem hiding this comment.
Its not a problem - you got the overall intent (that the user needs some sort of indicator about progress).
Yes - that value is intended for web-service requests. Maybe we need another similar indicator for local requests that can also be used for acoustid scans or volume levelling scans or re-encoding. Or perhaps you could look see what the which indicator is used by the native acoustid scan code and use the same indicator?
There was a problem hiding this comment.
The acoustid scans use the Pending Files indicator right next to the Pending Requests one. I could make it so that the files being "used" by an action are put in the pending state, meaning the counter is updated and the tracks are greyed out until the action finishes. This way, however, Pending Files would indicate the number of files being "used" by an action, rather than the number of actions that still need to be completed. I don't know if this is what you meant.
In the meantime, I made the other changes you suggested.
There was a problem hiding this comment.
I cannot be certain because I haven't tried to use the plugin, but this description seems to make sense.
| options = [config.BoolOption("setting", CANCEL, True), *action_options] | ||
| options = [ | ||
| config.BoolOption("setting", CANCEL, True), | ||
| config.IntOption("setting", MAX_WORKERS, 4), |
There was a problem hiding this comment.
Why 4?
Shouldn't this at least be based on the number of cores/hyperthreads that the CPU has?
There was a problem hiding this comment.
I made it so that the default value is the same as the one used by futures module.
There was a problem hiding this comment.
That sounds as good a default as we can get given that the tasks for a file could be heavy (reencoding the file) or light (doing some sort of web lookup), and the number of parallel tasks that can be supported would be very different for these two situations.
| register_album_action(ExecuteAlbumActions()) | ||
| register_track_action(ExecuteTrackActions()) | ||
| register_options_page(PostTaggingActionsOptions) | ||
| QObject.tagger.register_cleanup(action_runner.stop) |
There was a problem hiding this comment.
Can this be moved into the initialisation of the ActionRunner object?
|
@twodoorcoupe Giorgio - We are getting pretty close as far as I am concerned. Keep going.... |
There was a problem hiding this comment.
Thanks a lot for this. This definitely looks good and is something I wanted to have for a long time myself.
I'll give this a local test run, but from reading the PR this already looks really good.
And also thanks a lot @Sophist-UK for all the review work here. Much appreciated.
|
|
||
| # Settings. | ||
| CANCEL = "pta_cancel" | ||
| OPTIONS = ["pta_command", "pta_wait_for_exit", "pta_execute_for_tracks", "pta_refresh_tags"] |
|
@Sophist-UK, I tried using the Therefore, I opted for adding another indicator in the status bar to keep track of the pending actions. It look like this:
It is only visible when there are actions pending, otherwise it stays hidden. The icon is the same as the one used for I think this solution is better since, besides avoiding issues with other functions, it also makes the meaning of the indicator more clear. |
|
Thanks @phw! I saw your comment a minute too late. I changed it now. |
I think this is a great solution. Does this new status icon have a tooltip? If not can you add one please. |
Yes, it says "Pending actions", keeping the "style" of the other icons' tooltips. |
|
Great! As far as my desk review goes, this seems like it is good to go. |
There was a problem hiding this comment.
@twodoorcoupe I actually used this now and it is great. This will be useful for me, I already integrated it into my workflow. There are a couple of things that can be improved for the UI, e.g. editing existing actions. But all that can be done in a future update, I'd rather see that we get this merged first.
But also encountered some actual issues:
- I could not see the status bar icon show up. I haven't digged deeper, yet,and I don't see any error message, but something is no working for me here.
- When closing Picard without an action currently running there is an exception:
Traceback (most recent call last):
File "/usr/bin/picard", line 5, in <module>
main('/usr/share/locale', False)
File "/usr/lib/picard/picard/tagger.py", line 1583, in main
exit_code = tagger.run()
^^^^^^^^^^^^
File "/usr/lib/picard/picard/tagger.py", line 751, in run
self.exit()
File "/usr/lib/picard/picard/tagger.py", line 738, in exit
self.run_cleanup()
File "/usr/lib/picard/picard/tagger.py", line 635, in run_cleanup
f()
File "/home/phw/.config/MusicBrainz/Picard/plugins/post_tagging_actions/__init__.py", line 284, in stop
self.update_widget.join()
File "/usr/lib/python3.11/threading.py", line 1114, in join
raise RuntimeError("cannot join thread before it is started")
RuntimeError: cannot join thread before it is started
Actually this might be related to the first issue (widget not being instantiated)
|
Thanks for testing it @phw. |
|
@twodoorcoupe Found the issue, not yet sure how to fix. This happens if the plugin is already installed, like it was for me, since I checked out your branch and placed a symlink in That's because registering all the hooks happens when Picard is started and loads the plugins, regardless on whether the plugin is enabled or not. That means the code where it decided whether to initialize the widget immediately or use the The But for now I think the best solution would be to live with the fact that showing the status widget requires a restart and just make sure that the call It's all really a limitation of the plugin system currently, something we want to improve with Picard 3 by having plugins provide explicit hooks for lifecycle events like "enable" and not running anything if a plugin is disabled (more on that soon). |
|
Thank you so much for finding the issue @phw! I guess I have the habit of always enabling plugins as soon as I install them, so I would have never figured it out without your help. |
This plugins tries to solve PICARD-2743.
It adds the ability to specify "actions" that run with a context menu click. Basically, it can run external programs.
Besides what was initially proposed, I took inspiration from what other taggers offer, like mp3tag tools.
The commands can be set in the plugin's option page, the UI looks like this:
The commands run in the order they appear in the table. It lets you choose:
Also, it lets you specify variables to use in the command. These are taken either from the metadata like
%title%or%artist%, or from extra "special" variables like%filepath%or%is_complete%.I uploaded a user guide here where everything is hopefully explained more thoroughly, it includes a list of all the special variables as well. There's a link to it both in the options page and in the plugin's metadata.