Bug 374225

Created attachment 103026 [details]
Possibility to remove face identity along with face tag

Forgot the patch.

Why forgot the patch ???

Gilles Caulier

> --- Comment #2 from caulier.gilles@gmail.com ---
> Why forgot the patch ???
>
> Gilles Caulier
>
Sorry, that was misleading, everything is fine.
I forgot to add the patch to the initial comment, the same as with the
usual follow up email when you forget your attachments. However with the
KDE bug tracker it is normal to add attachments in an additional
comment, so my remark was completely unnecessary.

Hi Simon,

I would go even further. Removing the identity when deleting the tag is definitely a good idea. But I see another use-case that hit me multiple times.
I assigned a tag as face erroneously to some image. But removing the face from the image did not delete the ImageProperty that states that this tag is a person tag. The bug https://bugs.kde.org/show_bug.cgi?id=262168 is somehow connected to this.

Thus, deleting a person tag or a list of person tags but preserving them as ordinary tags is helpful to clean the DB. Otherwise, users would have to dig into the DB data which is not really nice for most of them.

I introduced a new context menu item "Delete People Tag" which works for singleton people tags and also for lists.
This action removes the autodetected face and region property for all images for the people tags and if wished (by confirmation) also the connection between the tag and the image, i.e. untaggs the image.
Also, the person, faceEngineId and faceEngineUuid property of the tags is removed and if existent, the Identity from the recognition.db. The recognition db entry is found by the uuid.

By the way, I had some TagProperties entries with property kfaceId and kfaceUuid, but did not find that in source code. May it be that the properties were renamed?

Created attachment 103331 [details]
The patch - includes Simons patch.

>By the way, I had some TagProperties entries with property kfaceId and >kfaceUuid, but did not find that in source code. May it be that the properties >were renamed?

Yes exactly. kface* is the legacy of libkface. These properties are obsolete and come certain from DK4. These value can be dropped if they exists.

Gilles Caulier

Okay, I thought so.
Should I commit the changes?

I don't yet tested the patch. I will do it after to complete my external similarity search test.

Maik, Simon, Can you test this patch please ?

Gilles

Patch looks good, people are removed from the DB. I think the 2. QMessageBox should have Yes and No as a possibility, instead of Yes and Cancel. Commit this, small code polishes make we later.

Maik

Can you hold of from committing for tonight?
My part is not suitable and I have some almost finished changes and
questions still. I will detail it later this evening.

Created attachment 103361 [details]
Mario's patch with changes and minus my patch

I should have written that I don't consider my patch suitable anymore, for the reasons you stated, because it prompts the user for confirmation for every deleted tag when multiple face tags are deleted and it doesn't remove the actual tag regions from faces.

The attached patch removes my part, as Mario's part is self contained and mine not ready to be pushed. I also introduced the following changes:
The slotMultipleFaceTagDel and slotFaceTagDelete methods are mostly redundant, so I replaced the latter with a call to the former. This introduces a tiny overhead, which is IMO worth it for less duplicity and thus easier maintenance.
I also did some purely cosmetic changes (mostly line breaks). If you actually prefer long lines, please tell me and I will stop doing this in the future.

Some conceptual questions:
The naming "Delete person tag" could be misunderstood as actually deleting the tag, not just its "affiliation" to a face. Something like "Delete face (identity)"/"Remove person from tag" would IMO be clearer, but it doesn't completely convince me either. I think it is necessary to clarify this, any ideas?
To implement the part I already implemented (incompletely): My preferred option would be to extend the existing function that removes a tag to also remove the face identity and regions when the deleted tag is a person tag, also if the people sidebar is not active. This seems the cleanest way, as otherwise we have information "hanging around" in the db and metadata, that is not accessible from the UI (the user can still keep this stuff). The downside: It adds even more potential popup questions the user has to answer. Opinions?

Some conceptual questions:
>The naming "Delete person tag" could be misunderstood as actually deleting the >tag, not just its "affiliation" to a face. Something like "Delete face
>(identity)"/"Remove person from tag" would IMO be clearer, but it doesn't >completely convince me either. I think it is necessary to clarify this, any >ideas?

In all case, the action title must be short in all case. Sometime it's complex to found the right words. By chance tooltip ans whatthis feature can help to describe with long phrase an action not well defined.

In this case, i vote for "Remove Face Tag" or something like that. "Face Tags" is used everywhere in GUI, not identity. In fact we ifentify a persone by a face, so the term is the most right to use here.  

>To implement the part I already implemented (incompletely): My preferred >option would be to extend the existing function that removes a tag to also >remove the face identity and regions when the deleted tag is a person tag, >also if the people sidebar is not active. This seems the cleanest way, as >otherwise we have information "hanging around" in the db and metadata, that is >not accessible from the UI (the user can still keep this stuff). The downside: >It adds even more potential popup questions the user has to answer. Opinions?

Yes this is the right way. But take a care this have side efeect with images metadata to sync with DB contents :

1/ The user must do it himself trough Maintenance tool
2/ The user enable to "Use Lazy Synchronisation" option from metadata settings panel to place items to synchronize in a queue. This one is processed when user use Caption sidebar tab and options on the bottom or at end of digiKam session.
3/ All items are all processed when face tag is removed.

In all cases, processing can take a while.

Gilles

(In reply to caulier.gilles from comment #12)
> Yes this is the right way. But take a care this have side efeect with images
> metadata to sync with DB contents :
> 
> 1/ The user must do it himself trough Maintenance tool
> 2/ The user enable to "Use Lazy Synchronisation" option from metadata
> settings panel to place items to synchronize in a queue. This one is
> processed when user use Caption sidebar tab and options on the bottom or at
> end of digiKam session.
> 3/ All items are all processed when face tag is removed.
> 

I didn't think about that. This patch leaves the database and metadata out of sync. This should never happen on purpose, right?
Actually the existing slotTagDelete method does that too. It uses AlbumManager's deleteTAlbum method and I don't see any connection to file metadata and testing confirms, that the tag is only removed from database. This means the user has to do your point 1): manually sync through maintenance.

This is a bug, isn't it? No matter for what reason a tag is altered on an image, if digikam is configured to write tags to metadata this should always be done.

>I didn't think about that. This patch leaves the database and metadata out of >sync. This should never happen on purpose, right?

yes

>This is a bug, isn't it? No matter for what reason a tag is altered on an >image, if digikam is configured to write tags to metadata this should always >be done.

yes it is.

Gilles

Simon, polishing my patches is completely okay.
Just to be safe. Deleting the region, the identity and the connection from tag to image is okay?
Should I sync the metadata or not? What should I adopt in my patch?

> --- Comment #15 from Mario Frank <mario.frank@uni-potsdam.de> ---
> Simon, polishing my patches is completely okay.
> Just to be safe. Deleting the region, the identity and the connection from tag
> to image is okay?
I think deleting the region and identity by default, and asking about
the tag is a sane behaviour.
> Should I sync the metadata or not? What should I adopt in my patch?
>
Yes, metadata should be synced (always). However this is not done in
many (if not all, I didn't check) methods of tagmodificationhelper. The
other methods delete tags via AlbumManager and looking at the right tag
sidebar, syncing metadata involves some fileworker interfaces. So this
seems quite a big and separate fix that is not directly related to this.
I discuss/solve that part in a new issue.

Do you agree on Gilles proposition to change "Delete person tag" to
"Remove Face Tag"?

Another question: The patch introduced a new method with an sql query.
In the existing code such queries are sometimes done directly via
execSql, at other places via get-/exec- DBAction, so from
dbconfig.xml.cmake.in .
What decides whether a query gets into dbconfig.xml.cmake.in or whether
it doesn't?

And only for my education, so ignore at will:
In the tagmodificationhelper methods, arguments are sometimes passed by
reference (lists), sometimes not (single album) while both underlying
types are pointers. Any particular reason for that?

(In reply to Simon from comment #16)
> > --- Comment #15 from Mario Frank <mario.frank@uni-potsdam.de> ---
> > Simon, polishing my patches is completely okay.
> > Just to be safe. Deleting the region, the identity and the connection from tag
> > to image is okay?
> I think deleting the region and identity by default, and asking about
> the tag is a sane behaviour.
Okay.
> > Should I sync the metadata or not? What should I adopt in my patch?
> >
> Yes, metadata should be synced (always). However this is not done in
> many (if not all, I didn't check) methods of tagmodificationhelper. The
> other methods delete tags via AlbumManager and looking at the right tag
> sidebar, syncing metadata involves some fileworker interfaces. So this
> seems quite a big and separate fix that is not directly related to this.
> I discuss/solve that part in a new issue.
Okay, I will look how to sync the metadata.
> 
> Do you agree on Gilles proposition to change "Delete person tag" to
> "Remove Face Tag"?
Yes, I agree. I will adopt that.
> 
> Another question: The patch introduced a new method with an sql query.
> In the existing code such queries are sometimes done directly via
> execSql, at other places via get-/exec- DBAction, so from
> dbconfig.xml.cmake.in .
> What decides whether a query gets into dbconfig.xml.cmake.in or whether
> it doesn't?
I cannot give a statement to this one.
> 
> And only for my education, so ignore at will:
> In the tagmodificationhelper methods, arguments are sometimes passed by
> reference (lists), sometimes not (single album) while both underlying
> types are pointers. Any particular reason for that?
That's not easy to explain since there may be many reasons. Some are technical, some
ideological.

The difference is subtile. While complex (big) structures like single albums are handled in heap a list of 
X (e.g. pointers) is usually located in the stack.

Consider that.
A list of pointers with each pointer being 64 bit aka 8 byte does not take much space.
Even if you store 100000, it's only 800000 byte aka 800 kilobyte. that's okay.
But passing a list of this size by copy is expensive. Thus, you need a reference, to 
reduce the overhead. Also, you can modify the list in the called function. 
Whether you take a pointer or a ref does not make a difference (both collapse to 8 byte).
But refs cannot be null, while pointers can. And if you know that you have an at least empty list,
pointers do not make sense here.
Passing a const pointer only forbids the manipulation of the pointer, but not the contents of the object.
Passing a const ref list forbids altering the list. This is far more secure than passing a const pointer.

Also, using pointers may indicate for other developers that the passed object is located in heap which may not
be always the case. The developer could try to deallocate the object stored in stack which will impose errors.
Though I do not think that this would compile.

Created attachment 103373 [details]
New patch for removing face tags

Okay, I had to fix the dialog texts and some other stuff.
Now, the actions are called "Remove Face Tag" and "Remove Face Tags", respectively.
The dialog for untagging the tagged images after removing the face property has now Yes and No as options. 
The dialogs now explicitely state which face tags are still connected to some images and which face tags have subtags that are face tags, too. 
I also set What's this texts but they are not shown which is weird.

Furthermore, in people sidebar, only "Remove Face Tag" and "Remove Face Tags" can be selected as actions. 
I removed "Delete Tag" and "Delete Tags" since these actions should be located only in tags sidebar. 
Otherwise, users may be confused about the semantics. And I think we should keep functionality where it fits best semantically.
Thus, delete tags in tags sidebar and remove faces in people sidebar.

Finally, I sync the tags metadata to the files - only the tags metadata.
I get the image info for the files, load the metadata with metadata hub and use the function writeTags.
I hope this is an appropriate way.
I tested this functionality with showFoto and it worked.

So,
Please take a look.

To Simon, from comment #16

"Another question: The patch introduced a new method with an sql query.
In the existing code such queries are sometimes done directly via
execSql, at other places via get-/exec- DBAction, so from
dbconfig.xml.cmake.in .
What decides whether a query gets into dbconfig.xml.cmake.in or whether
it doesn't?"

==> SQL code can be portable between SQL engines. Currently mysql, mariadb, and sqlite. This is true for simple queries in tables.

For complex queries, especially which touch the dB table structures, data join, or other SQL love, we need to wrap this code in a XML list with versioning rules, depending of DB structure version. 

So, if your SQL code is simple, and work everywhere, it can still in source code as well, else, it's more complex...

To Mario, from comment #18:

"Finally, I sync the tags metadata to the files - only the tags metadata.
I get the image info for the files, load the metadata with metadata hub and use the function writeTags.
I hope this is an appropriate way.
I tested this functionality with showFoto and it worked."

This want mean that tag are always sync in DB, and in file metadata if option is turned on, through MetadataHub ? If yes, it's the right way.

I supose that by "testing with Showfoto" want mean to check metadata contents after to change tags inside digiKam. Showfoto do not support database and tags from digiKam.

Gilles

(In reply to caulier.gilles from comment #20)
Hey Gilles,

> To Mario, from comment #18:
> 
> "Finally, I sync the tags metadata to the files - only the tags metadata.
> I get the image info for the files, load the metadata with metadata hub and
> use the function writeTags.
> I hope this is an appropriate way.
> I tested this functionality with showFoto and it worked."
> 
> This want mean that tag are always sync in DB, and in file metadata if
> option is turned on, through MetadataHub ? If yes, it's the right way.
When the user removes some face tag from people sidebar, I ask him if the association from the images
to the tag shall also be removed (if existent). If he confirms that, I delete the association of the
tag to the image and sync the new tags for the images from database to the image.

Like this:
1) Unassign the tag:
imageTagAssociation.unAssignTag();

2) get the modified image info, i.e. the new metadata:

metadataHub.load(info);

3) write the new tags to the image file:
metadataHub.writeTags(info.filePath())

> 
> I supose that by "testing with Showfoto" want mean to check metadata
> contents after to change tags inside digiKam. Showfoto do not support
> database and tags from digiKam.
Exactly. After modifying the image in digiKam, i opened the Image in Showfoto
and check the metadata. Result: everything as expected.
> 
> Gilles

Git commit 511ee541a76d22cef63ac9138dbe6f2cd037f808 by Mario Frank.
Committed on 16/01/2017 at 09:20.
Pushed by mfrank into branch 'master'.

It is now possible to remove tags from people sidebar. This actions are called "Remove Face Tag" and "Remove Face Tags", respectively.
The adoptions check if multiple face tags in the potential subtrees would be removed and asks the user for confirmation.
Also, the adoptions tell the user which face tags are connected to images.
The user can choose to remove not only the face tags but also the connection from the underlying tag to the affected images (untagging).
If he confirms, the metadata is synced to the image files (with writeToMetadata) directly (if lazy sync is off). If lazy sync is activated, the metadata
changes are enqueued.

Furthermore, in people sidebar, only "Remove Face Tag" and "Remove Face Tags" can be selected as actions.
I removed "Delete Tag" and "Delete Tags" since these actions should be located only in tags sidebar.
Otherwise, users may be confused about the semantics. And I think we should keep functionality where it fits best semantically.
Thus, delete tags in tags sidebar and remove faces in people sidebar.

M  +1    -1    NEWS
M  +22   -0    app/utils/contextmenuhelper.cpp
M  +6    -0    app/utils/contextmenuhelper.h
M  +1    -0    app/views/leftsidebarwidgets.cpp
M  +31   -0    libs/database/coredb/coredb.cpp
M  +10   -0    libs/database/coredb/coredb.h
M  +18   -0    libs/facesengine/facedb/facedb.cpp
M  +1    -0    libs/facesengine/facedb/facedb.h
M  +24   -2    libs/tags/tagfolderview.cpp
M  +9    -0    libs/tags/tagfolderview.h
M  +232  -0    libs/tags/tagmodificationhelper.cpp
M  +39   -0    libs/tags/tagmodificationhelper.h

https://commits.kde.org/digikam/511ee541a76d22cef63ac9138dbe6f2cd037f808

(In reply to Mario Frank from comment #21)

> > This want mean that tag are always sync in DB, and in file metadata if
> > option is turned on, through MetadataHub ? If yes, it's the right way.

To make it more precise: The tags are always in sync with DB.
Now I write the metadata after confirmation with writeToMetadata.
I think this is cleaner than with writeTags since writeTags does not evaluate
the lazySync option.
If lazy sync is off, the metadata is written directly to the file.
I tested this with loading the image with showFoto.
If lazySync option is turned on, the affected files are enqueued for later
sync.

I did not close this file since the deletion of metadata in tags sidebar
is still open and part of this file.

Cheers,
Mario

Git commit 2f8ddd42ef62d7aea9e490cdb05ffcc644810c81 by Mario Frank.
Committed on 22/02/2017 at 15:05.
Pushed by mfrank into branch 'master'.

Merged the current state of the garbage collection branch which improves the database cleanup stage of the maintenance
and improves the reactiveness of the maintenance overall. We ported the way items are processed to a queue based method
that can use the CPUs more effectively and does not create thousands of threads.
Related: bug 283062, bug 216895, bug 351658, bug 362023, bug 329353
FIXED-IN: 5.5.0

M  +17   -12   NEWS

https://commits.kde.org/digikam/2f8ddd42ef62d7aea9e490cdb05ffcc644810c81