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.

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

"Internal" Metadata Support Plugin #676

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jcjgraf
Copy link
Contributor

@jcjgraf jcjgraf commented Aug 4, 2023

This PR deals with the long-standing #324 and is an adoption of #325 to the refactored metadata support (c.f. #467).

The code is taken 1:1 from #325 and adopted to the new plugin-based metadata support schema, by making it a standalone plugin. No changes to vimiv are required to make it work.

First and foremost, I do not think that internal is appropriate anymore. This naming was chosen, as the information provided was coming from vimiv infernally, opposed to by an external library. I called it general, but that is also not really a good name.

Secondly, I am not sure if it really makes sense to make this an official plugin, or whether I should maintain it myself. I do not really mind, it is up to you to decide.

Code-wise this should be done. Due to #650 the extration of the filetype does not work as before. Need to figure out a new way to implment that. But if we decide to merge, then I need to adopt the docs and potentially add tests first.

implements #324
replaces #325

@jcjgraf jcjgraf force-pushed the feature/internalmetadata_plugin branch from bd74e76 to fcf629c Compare August 4, 2023 19:17
@karlch
Copy link
Owner

karlch commented Aug 5, 2023

Thanks for looking into this again! 😊

I won't be able to run a detailed analysis today, but I have a few random thoughts.

I would not include this as a default plugin, but either:

  1. Have it always loaded and part of the regular codebase. There is really not much harm as the code is simple, no new dependencies, ...
  2. Have it as your user-plugin, if you think the code is not generally useful.

Personally, I am happy with 1.

No matter what we do, I am not sure if the code as-is can deal with the current structure. Especially the metadata.has_metadata_support is then always True, when the "general" plugin is loaded, but this may not be what we "mean". Especially for the other "default" plugin loading, but also haven't dug into the other places like copying exif data, etc.

@jcjgraf
Copy link
Contributor Author

jcjgraf commented Aug 16, 2023

Why would you not have it as a default plugin but rather as part of the regular codebase? (Just to make sure we are on the same page, print and imageformats is what I consider as default plugins) And what exactly do you mean with part of the regular codebase? What would be required to do that?

No matter what we do, I am not sure if the code as-is can deal with the current structure.

I think it is no issue with the exception that no "real" metadata plugin (piexif or pyexiv2) is automatically loaded when "general" is loaded. But we recommend to explicitly specify the desired metadata backend anyways. So, I do not see an issue here.
Writing (:write) is also no issue, as "general" does not implement that method, and hence does not interfere with that process (except if the user expect a "real" metadata plugin to be loaded, which is not due to loading "general").

I think it is less effort if I have it as my user-plugin. In case it should ever get really popular (😏), we can still think about "merging" it into vimiv.

@karlch
Copy link
Owner

karlch commented Aug 17, 2023

Just not in the plugins part, but, e.g. under imutils and always loaded. I actually think print should also not be there, as it is always loaded anyway, and there is no real way to stop that. Having default plugins that always load kind of does not make them a plugin, not sure why it was designed that way in hindsight.

Now imageformats is different, as it is not loaded by default, and can be configured nicely.

In case we put this under imutils and always load it, then the check in the "autoloading plugin" could take care of this accordingly, knowing one will always be there.

I think it is quite useful, and don't see why it couldn't just sit in the codebase, but feel free to make it a plugin on your side of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants