Repository: karlch/vimiv-qt (language: python)

Issue:
Read Metadata Using pyexiv2 - continued
Continuation of #301

All this does is fix CI and refactor the test suite to run with both pyexiv2 and piexif along with some minor cleanup/personal preference commits.

@jcjgraf would you like to take a look?

In addition to everything done here, while refactoring I came in doubt if actually deprecating piexif is the correct step to take at the current point. It is easier to install than pyexiv2 and also easier to use in the simple cases. Maintaining both should not be too difficult given the nice infrastructure you created. Together with some functionality to log messages once, e.g. inform that piexif does not support metadata formatting on the first call of `:metadata`, this could be the best option for the user. What do you think @jcjgraf? Do we really have to get rid of piexif from our perspective or can we leave the choice for now and only deprecate once there are major issues (like piexif not supporting a new python version or so)?


Discussion / hints:
This looks really good, thanks a lot! Sorry for all the minor flaws, inconstancies and careless mistakes I have left behind in my work :upside_down_face: 

I do not get the working of the pytest so I cannot really comment on the tests.

Concerning supporting both libraries, I think your concern is really adequate. Especially since all work is already done and vimiv runs on both libraries. As you say, maintaining both should not really be an issue. The only thing which may be a little troublesome is the different indexing of the metadata. Here some thoughts I have made about that:
- I think we should get rid of the part in `imutils.exif.ExifHander` where we assume that piexif tags have the prefix `Exif.Image.` or `Exif.Photo.` in pyexiv2. This trial and error check is not very clean but rather hacked.
- The default metadata tags in `api.settings.metadata.defaults` are designed for piexif. With the prefix hack (the one I have talked about in the bullet point above) it may work but maybe also not. I would solve this by adding two separate defaults and load the right one depending on the available exif plugin. These defaults could be added to `ExifHander` and `_ExifHanderPiexif` respectively and loaded from there.

Besides the already mentored things, I think it would be really helpful for the user if we would add information about metadata support to the [docs](https://karlch.github.io/vimiv-qt/documentation/index.html). We should list:
- Advantages/disadvantages of both options
- Where to find the available exif keys to extend to config
- Installation instruction
- How to move from piexif to pyexiv2

Absolutely no worries, I also just noticed them while reading through the code `N > 5` times when thinking about testing.

I fully agree with all of your thoughts and have some further questions / suggestions I would love to hear your opinion on :blush: 

- I love the idea of storing the defaults in the corresponding class. Thanks to the class decorator approach we should be able to just grab them in the settings api via `ExifHandler.default_keys` or so. Would you then keep the full path for all pyexiv2 keys?
- As we plan to keep support for both libraries and will possibly continue to add more exif-related features, would it make sense to slightly change the base class behaviour? I am thinking of raising an exception in the base class for unsupported features instead of declaring the methods "pure virtual". Like this we don't have to more or less duplicate empty code, once for declaring the pure virtual methods in the base class, once for overriding them in `_ExifHandlerNoExif`. In addition we could decide to support some methods only with one of the exif libraries and just not override the method in the other exif library implementation.
- All of your documentation proposals make a lot of sense. I suppose we can just add another page for exif support, will look into this after the implementation details are done. We could possibly link to some exif library documentation to find all available keys, but i guess the nice option for this is #220, right?
- I suppose you are talking about the key paths for the defaults, right? The simplest solution is to just list all the keys as we are currently doing. However, this may clutter the code quite badly. Maybe we can clean it up using formatted strings? Or what have you thought?
- Yeah, I agree that it makes sense to design these exifhandlers in a way that they can easily be extended without much trouble and tedious copying. Also, having the possibility to add a method just for one library sounds great. Do you think we can create a super class (instead of abstract class), which is used if there is no exif support, and two child classes depending on which library is in use, or what do you think?
- For pyexiv2 is is certainly [this](https://www.exiv2.org/metadata.html). For piexif, maybe one can use the same list and drop the whole prefix, but I have not tested it. Besides that, #220 would certainly be helpful.

- Yes and no. The problem you describe is actually one I haven't thought about, I was just going to list them as you describe. But thinking of a smarter solution would certainly make sense :laughing: I was just wondering if you would require the key option in the ini to be the full path for pyexiv2, i.e. `Exif.Photo.Whatever` instead of `Whatever`. As mentioned I am not sure I like the verbosity of this, on the other hand it is certainly the cleanest option. I will go with whatever you suggest :blush:
- This is exactly what I thought of, yes. I have tried an implementation in the latest commit 69c058920c2476c2c0d72c2e3a3365b631af569b.
- Thanks for hinting me to the documentation!
I am still not sure if I have got you concerning the fist point. `pyexiv2.ImageMetadata` is a set-like structure (I have not looked at the source feels like a set) and can only be indexed using the full path, i.e. `Exif.Photo.Whatever`. Given only `Whatever`, pyexiv2 cannot give any data. The metadata tag does not always start with `Exif.Photo`, so we cannot just assume it to be a fixed prefix. In the list, which I have sent you in the previous comment, you can see all possible prefixes.

But as said, `Exif.Photo` as well as `Exif.Image` are quite frequent prefixes, therefore we have assumed that given `Whatever`, it has one of the two prefixes.

Either we leave it as it is, or remove this prefix assumption mechanism. I do not really mind. Leaving it in is more handy since it allows for shorter configuration/less cluttered config. But, it may be also confusing for the users that some keys require the full key path while some only required the last part of the key. But if we well document that, that should not really be an issue I belief.

Sure, I somehow feel like we mean the same thing but are somehow talking past each other :laughing:

I am still trying to understand your point:
> I think we should get rid of the part in imutils.exif.ExifHander where we assume that piexif tags have the prefix Exif.Image. or Exif.Photo. in pyexiv2.

as I really like the convenience of being able to refer to keys with only the suffix. Maybe we could allow this for all keys, e.g. by grabbing the available prefixes in the `__init__` via something like
```
            available_prefixes = {
                f"{elem.rpartition('.')[0]}."
                for elem in self._metadata
            }
            self._prefixes = ("", *available_prefixes)
```
and later iterate over all available prefixes
```
        for base_key in desired_keys:
            # For convenience allow referring to a key without its prefix
            for prefix in self._prefixes:
            ...
```

I understand that some of the keys are ambiguous if you only pass the suffix, but as long as we document this and allow using the full path as well I don't see much of an issue. Having it only for two of the prefixes, `Exif.Photo` and `Exif.Image`, does seem a bit random, I agree.

The trial-and-error approach is certainly not the most beautiful, but it does seem the most convenient to me at the moment.

Ok, now I get what what you meant.

I am for removing this functionality mainly because it seems a bit tinkered and may behave unexpectedly. But to come out in favour/against it I would have to research and figure out: how many prefixes are there? how many clashes happen?. I can do that, but probably not in the next few days.

So, I propose that we leave this functionality with the two current prefixes in and that I will do that research some time. Thereupon we can then decide if we want to extend that functionality with more prefixes or if we remove it altogether.

Took me some time, but I tried to make myself a little picture and think I understand your fears a bit better. I wrote myself a little script to print out possible duplicates, and some of the ones that I discovered and don't like are e.g.
- ApertureValue: Exif.Photo.ApertureValue, Exif.CanonSi.ApertureValue
- ShutterSpeedValue: Exif.Photo.ShutterSpeedValue, Exif.CanonSi.ShutterSpeedValue
- XResolution: Exif.Image.XResolution, Exif.Thumbnail.XResolution

But in general these clashes would not occur with the current implementation where we allow `Exif.Image` and `Exif.Photo` as the only "short-cuts". So I think I agree with you to keep the current implementation as-is, possibly still improve the defaults, document everything and hopefully get this done :smile:

In case you are ever interested, here is the little script I used:
<details><summary>find_exifkey_duplicates.py</summary><p>

```python
import itertools
import pprint

import pyexiv2


def find_similar_keys(data):
    def keyfunc(elem):
        key, value = elem
        return key.rpartition(".")[-1]

    groups = {
        k: lg
        for k, g in itertools.groupby(sorted(data.items(), key=keyfunc), key=keyfunc)
        if len(lg := list(g)) > 1
    }
    pprint.pprint(groups)


def check_image(path: str):
    def get_value(v):
        try:
            return v.human_value
        except AttributeError:
            return v.raw_value

    metadata = pyexiv2.ImageMetadata(path)
    metadata.read()

    data = {k: get_value(v) for k, v in metadata.items()}
    find_similar_keys(data)


def main():
    path = "..."
    check_image(path)


if __name__ == "__main__":
    main()
```

</p></details>
Alright, I made some hopefully final touches in the last few commits:
- We now use the full exif path for our default key sets and only strip those in the piexif implementation.
- The exif handling should be documented in `docs/documentation/exif.rst`. If you would like to take a look beforehand, you should be able to build the docs using `tox -e docs -- path/to/website` and then `$BROWSER path/to/website/documentation/exif.html`.

What do you think? Ready to merge and destroy everything? :laughing: 

Thank you for you extending the docs, final improvements and cleanup. I was unable to run `tox -e docs -- path/to/website` (it failed with `ERROR: Could not open requirements file: [Errno 2] No such file or directory: 'vimiv-qt/misc/requirements/requirements_optional.txt'`. I do not know the setup well enough to debug that) so I just commented on the docs based on the raw file.

Besides my few comments above, I think this PR is ready to be merged. I have thought about reworking the default keysets but I will open a separate PR for that.

Nice little script. I wanted to do something similar but extract all available keys directly from the website. But currently, time is a little limited...
Thanks for catching this, I missed this a while ago when I renamed the requirements, but it should be fixed now :blush: 

I included all of your comments as-is or with very minor rewording, thanks a bunch! In case you have no further comments, I would go ahead and merge this, and then we celebrate, one way or the other :smile: 

Using the webpage for this sounds great, I am just not very good at web-scraping and probably won't put more effort into this.. If you ever end up doing this, I would certainly be interested in the results.
I am good, from my side this PR is ready to be merged :+1: 


Fix patch:
diff --git a/vimiv/api/settings.py b/vimiv/api/settings.py
index 6d183345e..9bd762033 100644
--- a/vimiv/api/settings.py
+++ b/vimiv/api/settings.py
@@ -495,9 +495,11 @@ class metadata:  # pylint: disable=invalid-name
 
     # Default sets
     defaults = [
-        "Exif.Image.Make, Exif.Image.Model, Exif.Image.DateTime, Exif.Photo.ExposureTime, Exif.Photo.FNumber, Exif.Photo.IsoSpeedRatings, Exif.Photo.FocalLength, Exif.Photo.LensMake, Exif.Photo.LensModel, Exif.Photo.ExposureBiasValue",  # pylint: disable=line-too-long,useless-suppression
-        "Exif.Photo.ExposureTime, Exif.Photo.FNumber, Exif.Photo.IsoSpeedRatings, Exif.Photo.FocalLength",  # pylint: disable=line-too-long,useless-suppression
-        "Exif.Image.Artist, Exif.Image.Copyright",
+        "Exif.Image.Make,Exif.Image.Model,Exif.Photo.LensModel,Exif.Image.DateTime,Exif.Image.Artist,Exif.Image.Copyright",  # pylint: disable=line-too-long,useless-suppression
+        "Exif.Photo.ExposureTime,Exif.Photo.FNumber,Exif.Photo.ISOSpeedRatings,Exif.Photo.ApertureValue,Exif.Photo.ExposureBiasValue,Exif.Photo.FocalLength,Exif.Photo.ExposureProgram",  # pylint: disable=line-too-long,useless-suppression
+        "Exif.GPSInfo.GPSLatitudeRef,Exif.GPSInfo.GPSLatitude,Exif.GPSInfo.GPSLongitudeRef,Exif.GPSInfo.GPSLongitude,Exif.GPSInfo.GPSAltitudeRef,Exif.GPSInfo.GPSAltitude",  # pylint: disable=line-too-long,useless-suppression
+        "Iptc.Application2.Caption,Iptc.Application2.Keywords,Iptc.Application2.City,Iptc.Application2.SubLocation,Iptc.Application2.ProvinceState,Iptc.Application2.CountryName,Iptc.Application2.Source,Iptc.Application2.Credit,Iptc.Application2.Copyright,Iptc.Application2.Contact",  # pylint: disable=line-too-long,useless-suppression
+        "Exif.Image.ImageWidth,Exif.Image.ImageLength,Exif.Photo.PixelXDimension,Exif.Photo.PixelYDimension,Exif.Image.BitsPerSample,Exif.Image.Compression,Exif.Photo.ColorSpace",  # pylint: disable=line-too-long,useless-suppression
     ]
 
     # Store the keys as a comma separated string


Test patch:
diff --git a/tests/end2end/features/image/conftest.py b/tests/end2end/features/image/conftest.py
index 41075c6f7..cf7c2ec2a 100644
--- a/tests/end2end/features/image/conftest.py
+++ b/tests/end2end/features/image/conftest.py
@@ -24,6 +24,7 @@ def exif_content():
             piexif.ImageIFD.Copyright: b"vimiv-AUTHORS-2020",
         },
         "Exif": {piexif.ExifIFD.ExposureTime: (1, 200)},
+        "GPS": {piexif.GPSIFD.GPSAltitude: (1234, 1)},
     }
 
 
diff --git a/tests/end2end/features/image/metadata.feature b/tests/end2end/features/image/metadata.feature
index 590cc0c5e..14911292f 100644
--- a/tests/end2end/features/image/metadata.feature
+++ b/tests/end2end/features/image/metadata.feature
@@ -30,8 +30,8 @@ Feature: Metadata widget displaying image exif information
         When I add exif information
         And I run 3metadata
         Then the metadata widget should be visible
-        And the metadata text should contain 'Copyright'
-        And the metadata text should contain 'vimiv-AUTHORS-2020'
+        And the metadata text should contain 'GPSAltitude'
+        And the metadata text should contain '1234'
 
     Scenario: Error message on invalid metadata key set number
         Given I open any image