[digikam] [Bug 374225] New: Add possibility to remove face identities by removing tag

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

            Bug ID: 374225
           Summary: Add possibility to remove face identities by removing
                    tag
           Product: digikam
           Version: 5.4.0
          Platform: Other
                OS: Linux
            Status: UNCONFIRMED
          Severity: wishlist
          Priority: NOR
         Component: Faces-Management
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

When deleting a face-tag in the people part of the left sidebar a new dialog
asks whether the connected face identity should be removed from the database.

Use case:
If a person was face-tagged by mistake or is no longer of interest, removing
the face tags from images and deleting the tag itself does not remove this
person from the face database. Thus when face recognition is done again, there
will be new proposed face-tags for the delete person. The only way to get rid
of it is to remove all training data. With this patch only a specific identity
(and its training data) can be removed.

Detail about implementation:
The methods identityForTags and tagForIdentity of FaceUtils should probably be
static, shouldn't it?
They don't use any other class members. I had to create an instance of
FaceUtils just to call one of these methods. The only place one of these
methods is used in existing codes the same has been done. That seems
unnecessary and wasteful. Or is this a normal practice in c++?

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #1 from Simon <[hidden email]> ---
Created attachment 103026
  --> https://bugs.kde.org/attachment.cgi?id=103026&action=edit
Possibility to remove face identity along with face tag

Forgot the patch.

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]
            Summary|Add possibility to remove   |Add possibility to remove
                   |face identities by removing |face identities by removing
                   |tag                         |tag [patch]

--- Comment #2 from [hidden email] ---
Why forgot the patch ???

Gilles Caulier

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #3 from Simon <[hidden email]> ---
> --- Comment #2 from [hidden email] ---
> 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.

--
You are receiving this mail because:
You are the assignee for the bug.

[Simon Frei]

> --- Comment #2 from [hidden email] ---
> 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.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

Mario Frank <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #4 from Mario Frank <[hidden email]> ---
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?

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

Mario Frank <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Add possibility to remove   |Add possibility to remove
                   |face identities by removing |face identities by removing
                   |tag [patch]                 |tag and remove person tags
                   |                            |but preserving the tags
                   |                            |themselves [patch]

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #5 from Mario Frank <[hidden email]> ---
Created attachment 103331
  --> https://bugs.kde.org/attachment.cgi?id=103331&action=edit
The patch - includes Simons patch.

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #6 from [hidden email] ---
>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

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #7 from Mario Frank <[hidden email]> ---
Okay, I thought so.
Should I commit the changes?

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #8 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

Maik Qualmann <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #9 from Maik Qualmann <[hidden email]> ---
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

--
You are receiving this mail because:
You are the assignee for the bug.

[Simon Frei]

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.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #10 from Simon <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #11 from Simon <[hidden email]> ---
Created attachment 103361
  --> https://bugs.kde.org/attachment.cgi?id=103361&action=edit
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?

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #12 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #13 from Simon <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #14 from [hidden email] ---
>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

--
You are receiving this mail because:
You are the assignee for the bug.

[bugzilla_noreply]

https://bugs.kde.org/show_bug.cgi?id=374225

--- Comment #15 from Mario Frank <[hidden email]> ---
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?

--
You are receiving this mail because:
You are the assignee for the bug.

[Simon Frei]

> --- Comment #15 from Mario Frank <[hidden email]> ---
> 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?