Port from pyexif2 to GExiv2

[infirit]

As stated on the pyexiv2 website [1] it is unmaintained and it's successor is GExiv2. Attached is a patch to use GExiv2.

I removed the try/except for registerting the namespace because when the variety namespace is not registered none of the variety tags will be added/written.

Although not neccesary I left the namespace register for reading the exif data. It does not need it but also does not hurt it. I leave it up to you to decide to keep it or not.

Patch applies to current trunk and the last release version. I tested it against the last released version and I did not find any issues.

[1] http://tilloy.net/dev/pyexiv2/

[infirit (infirit)]

Previous patch was reversed somehow :(

[Peter Levi (peterlevi)]

Thanks for the work. I had also noticed the deprecation warning and thought about porting it, but stopped for two reasons - the less important one was that they say on the site "Formal API documentation is not available at this time.".

The more important one though stopped me: I don't see a way to get GExiv2 on Ubuntu 12.04 - and Quickly also fails to build Variety with a message "ERROR:root:Could not find any typelib for GExiv2". And for now I insist on keeping the support for 12.04 and would also prefer to keep the simple one-version-fits-all deployment approach as long as possible (sidenote: The deployment and packaging procedure is not good as it is now - I rely on running quickly manually, and I haven't had the time and will to fix this, deployment and packaging is not my strong side... Having separate branches for 12.04 and later versions would be a major complication).

Am I missing something here - what dependency do I need to be able to use GExiv2 in Python on 12.04 ? A quick google search Some comments here suggest this is not fixable: https://code.launchpad.net/~robru/nautilus-image-manipulator/gexiv2/+merge/141539

[infirit (infirit)]

I am a gentoo user and don't know enough about Ubuntu. But the last comments in the deban bug report [1] seems to indicate it should work

What I can do is support both and make GExiv2 the first one to try.

* Try to import GExiv2 with fallback to pyexev2 on ImportError
* Add some logic to switch between the 2 libs.

_Untested_ patch attached.

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=697204

[Peter Levi (peterlevi)]

Having both in the code used conditionally still results in errors with my current build process based on Quickly and requires me to set up an alternate build procedure which I want to avoid for now (not enough time to do all the necessary reading for this). Quickly automatically finds dependencies and adds them to the generated Debian package, and it doesn't care whether the dependency is used conditionally in try/catch or not. I.e. the problem happens at build/deploy time, not runtime.

Apart from being deprecated, is pyexiv2 causing problems in Gentoo right now? Is is misbehaving or in the process of being purged out of the repos?

[infirit (infirit)]

The reason I did the patch was becasue pyexiv2 is not in the official portage tree and had to resort to a 3rd party pyexiv2 ebuild (package) taken from the interwebs.

However once I got pyexiv2 installed variety works fine with it, no misbehaviour that I could find.

I use variety as user only (not in official Gentoo) and I am happy to keep applying the patch locally until you can make GExiv2 work on Ubuntu. So you can keep the report open and poke me if you need me to fix/rewrite the patch.

At least you know variety works with GExiv2 on at least one distro =)

[Christopher Meng (cicku)]

Fedora variety packager here.

Looking forward to see the porting.

[James Lu (jlu5)]

Peter, what's the status on this? I wonder if this will also fix https://bugs.launchpad.net/variety/+bug/1550345, which is frequently reported but still an open issue. Ubuntu 12.04 is EOL in a few days as well, so I'm hoping that it isn't a blocker to porting anymore.

[James Lu (jlu5)]

To answer the other question, I believe the Debian/Ubuntu deps for this are gir1.2-gexiv2-0.10 and python-gi. GExiv2 is a wrapper based on GObject introspection, so we'd actually be using those bindings to access it from Python.

[Peter Levi (peterlevi)]

12.04 isn't a blocker anymore, I've not been posting updates for it to the PPA since some incompatible change from a couple of months ago.

However, when I looked at the attached patch and tested it, there were problems with it (which unfortunately I forgot to document then) - if my memory is correct, handling of multi-value fields was somehow broken (for example there may be multiple keywords in an image), also by quickly skimming through GExiv2's API I couldn't quite understand how it is supposed to handle such array/multi-value fields. In short, it looked like it was not an exact drop-in replacement in some aspects that Variety relies on and the patch assumes it is. So at the time I decided that since pyexiv2, while deprecated, works with no issues, I'd rather not push forward on the issue.

Another thing was that GExiv2's documentation was really poor. It still says "Formal API documentation is not available at this time. However, a seasoned developer can work out how to use gexiv2 by looking at its primary header file (C), VAPI (Vala), or GExiv2.py (Python)." But when I look at https://git.gnome.org/browse/gexiv2/tree/GExiv2.py, it's not obvious to me how I am supposed to deal with multi-value fields like keywords.

So, if someone can take a look and help with this, I'll appreciate it.

[infirit (infirit)]

if you can point me to were it broke i'll be happy to update the patch.

fyi, there is documentation for it on the below link. looking at that there is a set_tag_multiple which may be what is required.
https://lazka.github.io/pgi-docs/#GExiv2-0.10/classes/Metadata.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[infirit (infirit)]

Yes, the set/get_tag_multiple functions are what is required. I played around with subclassing from GExiv2.Metadata a bit and came up with the below. This now comes close to how it was under pyexiv2 and imho much better :-). Let me know if this is the way you like to go and I'll send a new patch based on this.

class VarietyMetadata(GExiv2.Metadata):
    def __init__(self, path):
        super(VarietyMetadata, self).__init__(path=path)
        self.register_xmp_namespace("https://launchpad.net/variety/", "variety")

        self.__tag_multiples = [
            "Iptc.Application2.Headline",
            "Iptc.Application2.Keywords",
            "Xmp.dc.creator",
            "Xmp.dc.subject"
            ]

    def __getitem__(self, key):
        if self.has_tag(key):
            if self._tag_is_multiple(key):
                return self.get_tag_multiple(key)
            else:
                return self.get_tag_string(key)
        else:
            raise KeyError('%s: Unknown tag' % key)

    def _tag_is_multiple(self, key):
        return key in self.__tag_multiples

    def __setitem__(self, key, value):
        if self._tag_is_multiple(key):
            self.set_tag_multiple(key, value)
        else:
            self.set_tag_string(key, value)

[infirit (infirit)]

Sorry, couldn't wait and went ahead anyway. Seems to work nicely for me also with multiple value tags.

[Peter Levi (peterlevi)]

To be fair, I don't have a preference for the code organization (e.g.
separate class vs. read_metadata/write_metadata functions), as long as the
resulting code is functionally correct and works for all sorts of metadata
that we record. Flickr is a good source to test with, as it provides all
the extra bits of metadata. A nice-to-have would be also adding a dedicated
unit test that tests specifically reading/writing/reading metadata from a
file actually "preserves" all the bits.

On Tue, Apr 25, 2017 at 8:09 PM, infirit <email address hidden> wrote:

> Yes, the set/get_tag_multiple functions are what is required. I played
> around with subclassing from GExiv2.Metadata a bit and came up with the
> below. This now comes close to how it was under pyexiv2 and imho much
> better :-). Let me know if this is the way you like to go and I'll send
> a new patch based on this.
>
> class VarietyMetadata(GExiv2.Metadata):
> def __init__(self, path):
> super(VarietyMetadata, self).__init__(path=path)
> self.register_xmp_namespace("https://launchpad.net/variety/",
> "variety")
>
> self.__tag_multiples = [
> "Iptc.Application2.Headline",
> "Iptc.Application2.Keywords",
> "Xmp.dc.creator",
> "Xmp.dc.subject"
> ]
>
> def __getitem__(self, key):
> if self.has_tag(key):
> if self._tag_is_multiple(key):
> return self.get_tag_multiple(key)
> else:
> return self.get_tag_string(key)
> else:
> raise KeyError('%s: Unknown tag' % key)
>
> def _tag_is_multiple(self, key):
> return key in self.__tag_multiples
>
> def __setitem__(self, key, value):
> if self._tag_is_multiple(key):
> self.set_tag_multiple(key, value)
> else:
> self.set_tag_string(key, value)
>
> --
> You received this bug notification because you are subscribed to
> Variety.
> https://bugs.launchpad.net/bugs/1280349
>
> Title:
> Port from pyexif2 to GExiv2
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/variety/+bug/1280349/+subscriptions
>

[Peter Levi (peterlevi)]

Also, please do test it well - haven't tested yet, but I see this line, which would break:

m.safe_file()

[Peter Levi (peterlevi)]

Also, I wouldn't remove the list check here:

                 elif k == 'keywords':
- if isinstance(v, list):
- m['Iptc.Application2.Keywords'] = v
- m['Xmp.dc.subject'] = v
+ m['Iptc.Application2.Keywords'] = v
+ m['Xmp.dc.subject'] = v

If we by error somewhere we pass a non-list single string here, wouldn't it get unpacked into individual letters in the metadata? E.g. "word" => ['w', 'o', 'r', 'd']. Which would silently result in files with broken metadata.

Better to check and either raise an exception here, or automatically pack the value into a one-element list. I'm fine with both options.

[Peter Levi (peterlevi)]

Not to miss it - two thumbs up, and special thanks for this contribution :)

[Peter Levi (peterlevi)]

Actually we do have the test already - TestUtil.test_metadata. Please make sure it passes (it should run with variety/tests as cwd).

[infirit (infirit)]

Cool, tests :-). With some more fixes I have it passing the test test_metadata.

I added back the check for list and expanded it to tuples. Theoretically any sequence of strings will be fine but lets stick with these two. It now raises ValueError if we don't get one of the two.

New patch coming up :-)

[Peter Levi (peterlevi)]

Thanks infirit, merged and committed with some small fixes.
Everyone, please test metadata is being read and written correctly (e.g. author info, origin URLs, EXIF rating, etc.).