After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 784717 - Metadata viewer cannot show updated tags from editor.
Metadata viewer cannot show updated tags from editor.
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: Plugins
git master
Other All
: Normal normal
: 2.10
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2017-07-09 11:15 UTC by Ben
Modified: 2017-08-05 13:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch fix to allow metadata viewer to see all available tags. (3.22 KB, patch)
2017-07-09 11:15 UTC, Ben
committed Details | Review

Description Ben 2017-07-09 11:15:30 UTC
Created attachment 355204 [details] [review]
Patch fix to allow metadata viewer to see all available tags.

The metadata viewer cannot show update fields. This requires regressing part of the last patch update from Ell. Including patch fix.
Comment 1 Ell 2017-07-09 16:27:27 UTC
The metadata viewer does show updated fields, it just filters out all tags that exiv2 doesn't have a label for.  The code was supposed to show the label, which is more descriptive, instead of the tag name, but it accidentally shows the tag name anyway.  The idea behind filtering unlabeled tags was that most of these tags are probably of little interest to the user, however, it looks like exiv2 doesn't have labels for quite a few tags, in particular the DICOM ones.  So, getting rid of this filtering is probably in order.

However, I'd still rather show labels when available.  The simple option would be to use the labels when available, and fall back to the tag name when they aren't.  Another option is to have separate tag-name and label columns, if you're up to hacking it.  If you really feel like going the extra mile, adding a toggle to show/hide the unlabeled tags would be great :)
Comment 2 Ben 2017-07-09 16:31:39 UTC
Nope it doesn't, try it. When you update a new image and change xmp fields from the editor none show up with your changes in the viewer. So i have to disagree with your proposal.
Comment 3 Ell 2017-07-09 17:31:05 UTC
I did try, and it works here.  Just to be clear, the viewer currently filters out tags that exiv2 doesn't have a label for, like the DICOM tags, but this has nothing to do with whether you change them with the editor or not, or whether you save and reload;  tags that do have a label, like "address", show up just fine.

The last hunk of your patch removes this filtering, which we probably want to do given there are unlabeled tags we care about, but I'd still like to keep the label around in some form, when available, for user-friendliness.
Comment 4 Ben 2017-07-09 17:33:12 UTC
Doesn't here. I still disagree.
Comment 5 Ell 2017-07-09 17:45:26 UTC
Well, it must be caused by something else, then.  I see a bunch of error messages on the terminal when writing metadata with the editor, which might be related; or maybe your version of exiv2 has fewer labels.  Anyway, I'm not sure what you're disagreeing with.
Comment 6 Ben 2017-07-09 17:49:52 UTC
#1 Those have nothing to do with the issue i described.

#2 I disagree with the filtering i took it out for a reason.

#3 When i do 

File -> New image -> Ok on whater defaults -> Image -> Metadata -> Metadata Editor -> Edit DICOM Fields-> Write metadata -> Imagae -> Metadata -> Metadata Viewer.

None of the xmpMM fields show up until i reload the image, none of the dicom fields show up either. I already had one person tell me they didn't see it either.

So your proposal doesn't work and inhibits what i was trying to do in the first place which is to see what whats there. 

What you did inhibits that. So i call that a  fail.
Comment 7 Ben 2017-07-09 18:06:08 UTC
For the record i work on exiv2 as a developer i use whats in git, i recently did the xmp sdk split for that code base.
Comment 8 Ell 2017-07-09 18:07:57 UTC
#1 They have to do with the patch you're proposing.

#2 I'm really not trying to be snarky, but can you explain how does your patch fix the issue (which is, if I'm understanding correctly, that XMP tags only show up in the viewer after saving & reloading)?

#3 Well, 'round here, the DICOM tags never show up, no matter what, and labeled XMP tags always show up, no matter what, which is exactly what you'd expect to happen, given the current code.  Like I said, if you're seeing something else, then the problem is likely somewhere else.
Comment 9 Ben 2017-07-09 18:17:41 UTC
#1 Nope, you're totally wrong i know what that issue is for those i just need to find the 3 tags causing it. I just don't have the time right now.

#2 You are. It does fix it by not filter anything and showing what is really there no matter who put what tags where. I didn't want filtering in the first place which is why i took it out when i redid part of it. It didn't work for me then and doesn't now. I'm not the only one having the issue which is how i found out about it when they asked my why didn't show up and then i saw you redid the plugin. Removing those lines of code prevents the filter and then everything just works. 

#3 I call BS and it isn't working, you can explain it to the users that will come to complain. One already has, now two with me. I stand by what i said.

#4 I did a full new clean install of GIMP without my patch i still see the issue. If i remove the filtering code then anything that was adding after loading the image shoes up fine and none do with it.
Comment 10 Ell 2017-07-09 19:14:30 UTC
Hey now, we're all on the same team here :)  I didn't mean to undermine your work.  I'm sorry if this is how it appeared.  The viewer code was based on an older version of the metadata plugin that lacked some bug fixes, and all I did was reapply them.

I'm also agreeing that the filtering should go, given the fact that it hides some important tags, although, as I said, I would like to show the more descriptive label, rather than the tag name, when possible (and preferably, show both in some form.)

However, I can't reproduce the behavior that previously missing tags only appear after a reload, nor should the code in question have anything to do with that.  That's all I'm saying.
Comment 11 Ben 2017-07-09 20:30:12 UTC
That was why i only modified those lines of codes. Because of the other fixes i didn't have time to delve into to see which part was which only the part which prevented tags from showing up at all. It shouldn't but it does, when i first started on this months ago that filtering was there. It was a pain in the ass before i realized it was causing issues when entering my data in the editor and reading them back.  It was easier to remove it entirely and i never had any problems whatsoever there-after. Seeing that the same issue is coming is a major pita. I don't have the time this week to go further on this and i'll be entirely offline and deep in the boonies (hiking) the week after. Others have reported the issue and it needs to be removed. As for the implementation you should revert your patch and if you want it you should add it without the filtering because there wasn't that issue without all that code. Also if using Exiv2 v0.25 you will have issues, and should use whats in git or at least v0.26.
Comment 12 Michael Natterer 2017-07-09 20:35:58 UTC
You probably should have mentioned that. We don't have a direct
dependency on evix2 at all, only gexiv2. And the maximum thing we
can require is 0.25...

I guess we should add at least a configure check for the installed
exiv2 version.
Comment 13 Ben 2017-07-09 20:37:05 UTC
I only found out in the last two days
Comment 14 Michael Natterer 2017-07-09 20:38:50 UTC
Anything we can do to mitigate whatever issues there are?

Even debian unstable only has 0.25.
Comment 15 Ben 2017-07-09 20:40:23 UTC
I would need to do more testing but so far it fails to read some of the data properly. It shouldn't cause a crash.
Comment 16 Ben 2017-07-09 20:41:21 UTC
At least i didn't get any so far when i had it on here.
Comment 17 Ben 2017-07-09 21:01:56 UTC
As for the filtering issue here, i had it irrespective of which version was used.
Comment 18 Massimo 2017-07-10 06:57:20 UTC
I played with metadata-editor (opening it and pressing Write Metadata)
and have found 2 issues:

one is that the string returned by gimp_metadata_get_guid is not NUL
terminated

https://git.gnome.org/browse/gimp/tree/plug-ins/metadata/metadata-editor.c#n882

(bake is always < DALLOC in the for body) and address sanitizer reports a buffer
overflow when later is used in a strcat call.


the other is that here:

https://git.gnome.org/browse/gimp/tree/plug-ins/metadata/metadata-editor.c#n882

'default_metadata_tags[loop].tag' does not correspond to the widget used
for creatorContactInfoTags[loop], so often there are warnings for trying
to convert a GtkEntry pointer to a GtkTextView one.

Either 'loop' should be incremented by 78 or
'creatorContactInfoTags[loop].id' should be used in place of
'default_metadata_tags[loop].tag'
Comment 19 Massimo 2017-07-10 07:01:09 UTC
(In reply to Massimo from comment #18)
> I played with metadata-editor (opening it and pressing Write Metadata)
> and have found 2 issues:
> 
> one is that the string returned by gimp_metadata_get_guid is not NUL
> terminated
> 
> https://git.gnome.org/browse/gimp/tree/plug-ins/metadata/metadata-editor.
> c#n882
> 

the meaningful link is:
https://git.gnome.org/browse/gimp/tree/libgimpbase/gimpmetadata.c#n256
Comment 20 Ben 2017-07-10 10:47:52 UTC
(In reply to Massimo from comment #18 & #19)

Can start a new issue for those that i can track separately, i have other commitments this week i don't think i can fit it in right now and i'll be gone next week on my first vacation time in over a decade.
Comment 21 Massimo 2017-07-11 08:35:56 UTC
(In reply to Ben from comment #20)
> (In reply to Massimo from comment #18 & #19)
> 
> Can start a new issue for those that i can track separately, i have other
> commitments this week i don't think i can fit it in right now and i'll be
> gone next week on my first vacation time in over a decade.

My comment was meant just to help understanding how sometimes
the metadata viewer does not show tags updated from editor, regardless
of exiv2 version or whatsoever filtering.

If the text written for a metadata tag is obtained from the wrong widget
(possibly empty) it wont appear when later viewing modified metadata.


I'm not going to open reports for issues that are visible
just running GIMP from the console.

To stop the debugger at the first warning, I ran gimp like this:

>$ GIMP_PLUGIN_DEBUG_WRAP=metadata-editor GIMP_PLUGIN_DEBUG_WRAPPER="gdb -ex r -ex bt -ex q --args" G_DEBUG=fatal-warnings LC_ALL=C gimp-2.9

Happy holidays
Comment 22 Ell 2017-08-05 13:22:13 UTC
Review of attachment 355204 [details] [review]:

Pushed the patch to master:

commit 97c9a99339fd21927fe4aa4cb8934df96c5b4490
Author: Ben Touchette <draekko.software@gmail.com>
Date:   Sun Jul 9 07:11:50 2017 -0400

    Regression fix for metadata viewer allows to view all tags.
    
    Partially revert commit 8c2ca36ac76fc5fc9aa7aa9055d2c6e4772f93b7, so
    that unlabeled tags are not hidden.

 plug-ins/metadata/metadata-viewer.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)
Comment 23 Ell 2017-08-05 13:27:44 UTC
I'm closing the bug, since no tags should be missing anymore due to filtering.  If some tags used to only appear after a reload, there may be another problem somewhere else.  Feel free to reopen or file a new bug if something pops up :)