Introduce TagVar & TagVars classes #2637
Conversation
|
There's room for improvements but that's the idea. |
|
Of course, we can add Instead of |
There was a problem hiding this comment.
My long-term vision was to also allow plugins to register their tags and variables for autocompletion and tooltips. The initial thinking was to hold the information in a class like:
class PluginTagsAndVariables():
_tags_and_variables = {}
"""Dictionary containing the tags and/or variables for a plugin along with
the tooltip description by plugin identifier."""
@classmethod
def get_plugins(cls):
"""Returns a list of the names of the plugins with tags and/or variables
registered for autocompletion.
Return:
list: List of registered plugin names.
"""
return [x for x in cls._tags_and_variables.keys()]
@classmethod
def get_plugin_tags(cls):
"""Yields tag and variable names for all plugins registered for autocompletion.
Yields:
str: Tag or variable name.
"""
for items in cls._tags_and_variables.values():
yield from items.keys()
@classmethod
def get_plugin_tags_and_descriptions(cls):
"""Yields tuples of tag and variable names along with their descriptions for all plugins
registered for autocompletion.
Yields:
tuple: Tuple of (tag or variable name, description).
"""
for tags in cls._tags_and_variables.values():
for tag, desc in tags.items():
yield (tag, desc)
@classmethod
def add_tag(cls, plugin: str, tag: str, desc: str):
"""Add a tag or variable name to a plugin for autocompletion.
Args:
plugin (str): ID of the plugin. This must be the name of the plugin python file (or the
plugin zip file) without the extension, not the PLUGIN_NAME constant within the plugin.
tag (str): Tag or variable name to add. Cannot contain any spaces.
desc (str): Description of the tag or variable suitable for a tooltip.
"""
_plugin = plugin.strip()
_tag = tag.strip()
_desc = desc.strip()
if not _plugin or not _tag or ' ' in _tag:
return
if not _desc:
_desc = _('No description provided')
if _plugin not in cls._tags_and_variables:
cls._tags_and_variables[_plugin] = set()
cls._tags_and_variables[_plugin][_tag] = f"{desc} [plugin]"
# TODO: Consider adding the plugin name to the description.
@classmethod
def register_plugin(cls, plugin: str, tags=None):
"""Register a plugin and its tags and variables for autocompletion.
Args:
plugin (str): ID of the plugin. This must be the name of the plugin python file (or the
plugin zip file) without the extension, not the PLUGIN_NAME constant within the plugin.
tags (iterable, optional): Iterable of tuples (tag/variable, description) register. Defaults to None.
"""
if not plugin.strip():
return
if not tags:
tags = set()
if isinstance(tags, str):
tags = [(tags, '')]
try:
for tag in tags:
cls.add_tag(plugin, tag[0], tag[1])
except TypeError:
pass
@classmethod
def de_register_plugin(cls, plugin: str):
"""De-register a plugin and remove all of its related tags and variables from the scripting
autocompletion.
Args:
plugin (str): ID of the plugin. This must be the name of the plugin python file (or the
plugin zip file) without the extension, not the PLUGIN_NAME constant within the plugin.
"""
cls._tags_and_variables.pop(plugin.strip(), None)When a plugin was enabled, it could either register its tags and variables directly (which I had working in a copy of the 2.x branch), or this could be part of the registration process (perhaps through another constant in the plugin header).
when a plugin is disabled, I planned to have some extra code in the picard/ui/options/plugins.py module's save() method to de-register any plugins not in the enabled_plugins setting.
There's probably a cleaner way of handling this, but these were my initial thoughts.
|
What would be really nice (in my opinion) would be to provide a description (perhaps the short description) along with the tag/variable name in the autocompletion list. Trouble is, I suspect that would be a huge effort and may not be worth doing. |
This PR makes it actually much easier because it makes things more dynamic. We could even have plugin tags/vars marked with a plugin prefix (something like |
We could at least provide a dynamic help for tags & variables as we do with functions. |
Suggested by @rdswift
…TAGVAR` This class maintains an internal dict object for display names. Also it doesn't allow to add a TagVar of the same name more than once.
This sounded good initially, but the more I think about it I'm not sure I fully understand what you mean. Are you suggesting that users will need to prefix any variables from a plugin with the plugin ID when using them in scripts? I hope not, because that would break any scripts using plugins like "Additional Artists Variables". Further, I don't think a plugin should change the definition of an existing tag or variable (although it could change the value). I do like the idea of storing the new tags and variables created by a plugin in the |
|
When this change is merged, are there plans to also include it in the |
Well, I didn't really think about it, but I was thinking about a way for plugins to add variables without risking to be in conflict with another plugin doing the same, they would still be able to modify any existing tags. |
Perhaps not easy to do, but if it's possible why not. |
There was a problem hiding this comment.
@zas This is a good move. The ta definition requires a bit more code, but usage is simplified. Some comments for discussion, but otherwise this looks good.
I think it would be good to update VariableScriptToken.get_tooltip in scripttextedit.py already to make use of the new longdesc (even if that is not yet filled for any tag). This requires some helper to get the long description from a tag name, which would make the API more complete.
@phw, I actually have this coded up in a local branch waiting for this PR to be merged so I can rebase and update #2636 (or open a new PR). The approach I took was to add another method called The code looks like: TEXT_NOTES = N_('**Notes:**')
TEXT_READ_ONLY = N_('read-only')
TEXT_FROM_FILE = N_('provided from the file')
TEXT_NAMING_ONLY = N_('available in naming scripts only')
TEXT_NOT_FOR_SCRIPTING = N_('not available for use in scripts')
TEXT_CALCULATED = N_('calculated variable')
TEXT_NOT_FROM_MB = N_('not provided from MusicBrainz data')
TEXT_NO_DESCRIPTION = N_('No description available.')
# This is the new method in the TagVars class.
def display_tooltip(self, name):
tagdesc = None
if name and name.startswith('_'):
name = name.replace('_', '~', 1)
if ':' in name:
name, tagdesc = name.split(':', 1)
item: TagVar = self._name2item.get(name, None)
title = _(item.longdesc) if item and item.longdesc else _(TEXT_NO_DESCRIPTION)
notes = []
if item:
if item.is_preserved:
notes.append(_(TEXT_READ_ONLY))
if item.is_calculated:
notes.append(_(TEXT_CALCULATED))
if item.is_file_info:
notes.append(_(TEXT_FROM_FILE))
if not item.is_script_variable:
notes.append(_(TEXT_NOT_FOR_SCRIPTING))
if not item.is_from_mb and not item.is_file_info:
notes.append(_(TEXT_NOT_FROM_MB))
if notes:
title += f"\n\n{_(TEXT_NOTES)} {'; '.join(notes)}."
if markdown is not None:
title = markdown(title)
if name and name.startswith('~'):
name = name.replace('~', '_', 1)
if tagdesc:
return f"<p><em>%{name}%</em> [{tagdesc}]</p>{title}"
else:
return f"<p><em>%{name}%</em></p>{title}" |
|
@rdswift That is great. I guess in this case we could also separate it and wait for this PR to merge first. Just that we don't forget about this part :) |
After spending a few days adding all the long descriptions, updating the tooltips displayed, and writing tests, it is really unlikely I'm going to forget about it. 😜 |
Suggested by @phw in metabrainz#2637 (comment)
Suggested by @phw in metabrainz#2637 (comment)
… if hidden Suggested by @phw in metabrainz#2637 (comment)
|
@phw I implemented your suggestions (I think), most notably the |
I think this PR will be ready to merge soon, so you'll be able to work on your PR for long descriptions :) |
3 participants
Summary
Problem
Solution
Action
Additional actions required: