[digikam] [Bug 374191] New: Unusable context menu entries in tags view

[bugzilla_noreply]

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

--- Comment #15 from Simon <[hidden email]> ---
Hi Mario,

I justed tested it and it worked as expected.
I have a question/proposition: The existing slotNewDuplicatesSearch
funciton accepts a generic album pointer and then determines what type
(physical/tag) it is. You made two distinct functions for lists as
arguments that still accept generic album pointers but named them to
make it clear that either physical or tag albums are expected. Wouldn't
it be cleaner and less error prone to use one function name
(slotNewDuplicatesSearch) and overload it with different parameters
(PAlbum*, TAlbum*, QList<PAlbum*> and QList<TAlbum*>)?

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

[bugzilla_noreply]

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

--- Comment #16 from Mario Frank <[hidden email]> ---
(In reply to Simon from comment #15)

> Hi Mario,
>
> I justed tested it and it worked as expected.
> I have a question/proposition: The existing slotNewDuplicatesSearch
> funciton accepts a generic album pointer and then determines what type
> (physical/tag) it is. You made two distinct functions for lists as
> arguments that still accept generic album pointers but named them to
> make it clear that either physical or tag albums are expected. Wouldn't
> it be cleaner and less error prone to use one function name
> (slotNewDuplicatesSearch) and overload it with different parameters
> (PAlbum*, TAlbum*, QList<PAlbum*> and QList<TAlbum*>)?

Hi Simon,

I am not completely happy with this solution.
But the AlbumTreeView from which the TagFolderView is derived only provides me
access to the list of selected tags as list of Album pointers. Using overloaded
methods is also my preferred way. But to do this, I would either have to
transform the list of Album pointers to a list of TAlbum pointers which is an
unnecessary complexity. Or I will have to adopt the AlbumTreeView for this.
The latter is the way to go, I think.

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

[bugzilla_noreply]

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

--- Comment #17 from Simon <[hidden email]> ---
(In reply to Mario Frank from comment #16)

> [...]
>
> Hi Simon,
>
> I am not completely happy with this solution.
> But the AlbumTreeView from which the TagFolderView is derived only provides
> me access to the list of selected tags as list of Album pointers. Using
> overloaded methods is also my preferred way. But to do this, I would either
> have to transform the list of Album pointers to a list of TAlbum pointers
> which is an unnecessary complexity. Or I will have to adopt the
> AlbumTreeView for this.
> The latter is the way to go, I think.

I agree. Even from the naming it is weird that in TagTreeView the selectedAlbum
and albumForIndex methods return TAlbum, while selectedTags returns Album. So
changing this seems natural. And selectedTags isn't used so widely, so not much
adaption should be necessary.

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

[bugzilla_noreply]

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

--- Comment #18 from Mario Frank <[hidden email]> ---
(In reply to Simon from comment #17)

> (In reply to Mario Frank from comment #16)
> > [...]
> >
> > Hi Simon,
> >
> > I am not completely happy with this solution.
> > But the AlbumTreeView from which the TagFolderView is derived only provides
> > me access to the list of selected tags as list of Album pointers. Using
> > overloaded methods is also my preferred way. But to do this, I would either
> > have to transform the list of Album pointers to a list of TAlbum pointers
> > which is an unnecessary complexity. Or I will have to adopt the
> > AlbumTreeView for this.
> > The latter is the way to go, I think.
>
> I agree. Even from the naming it is weird that in TagTreeView the
> selectedAlbum and albumForIndex methods return TAlbum, while selectedTags
> returns Album. So changing this seems natural. And selectedTags isn't used
> so widely, so not much adaption should be necessary.

I already adopted the code and found some other effectless context menu entry.
In people-sidebar, the find duplicates contect menu item does not have any
effect. I wired the context menu to the same functionality as for tags, i.e.
selected people tags are set as tags in the find duplicates view.

This should be the expected behaviour, right?

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

[bugzilla_noreply]

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

Mario Frank <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Unusable context menu       |Unusable context menu
                   |entries in tags view        |entries in tags and people
                   |                            |view

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

[bugzilla_noreply]

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

--- Comment #19 from Simon <[hidden email]> ---
(In reply to Mario Frank from comment #18)
>
> In people-sidebar, the find duplicates contect menu item does not have any
> effect. I wired the context menu to the same functionality as for tags, i.e.
> selected people tags are set as tags in the find duplicates view.
>
> This should be the expected behaviour, right?

Yes.

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

[bugzilla_noreply]

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

--- Comment #20 from [hidden email] ---
The difference between Tags and face-tags is to name just face in the image,
not the whole image.

So search similarity from face-tags, as similarity is implemented will search
for same similar whole image not face.

We need to be be clear for that. I recommend to plan to write some words about
this topic in handbook for end users, if this implementation is add to digiKam.

Gilles

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

[bugzilla_noreply]

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

--- Comment #21 from [hidden email] ---
To comment #13 :

"Moreover, I found some function calls in the code that seem to be superfluous.
I did not remove them, but only set a comment @ODD."

I take a look to the patch #103144. The commented code with @ODD :

+    // @ODD : Why is singleton set to true? resetAlbumsAndTags already clears
the selection.
     d->albumSelectors->setPAlbumSelected(album, true);

+    // @ODD : Why is singleton set to true? resetAlbumsAndTags already clears
the selection.
     d->albumSelectors->setTAlbumSelected(album, true);

I think this kind of singleton selection exist due to some problem seen while
porting from Qt4 to Qt5. I don't remember exactly.

Check if this argument need to be set to true really, as the model have been
adjusted previously.

Check also where these methods are used outside similarity code to see if this
argument need really to 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=374191

--- Comment #22 from Mario Frank <[hidden email]> ---
(In reply to caulier.gilles from comment #21)

> To comment #13 :
>
> "Moreover, I found some function calls in the code that seem to be
> superfluous. I did not remove them, but only set a comment @ODD."
>
> I take a look to the patch #103144. The commented code with @ODD :
>
> +    // @ODD : Why is singleton set to true? resetAlbumsAndTags already
> clears the selection.
>      d->albumSelectors->setPAlbumSelected(album, true);
>
> +    // @ODD : Why is singleton set to true? resetAlbumsAndTags already
> clears the selection.
>      d->albumSelectors->setTAlbumSelected(album, true);
>
> I think this kind of singleton selection exist due to some problem seen
> while porting from Qt4 to Qt5. I don't remember exactly.
>
> Check if this argument need to be set to true really, as the model have been
> adjusted previously.
>
> Check also where these methods are used outside similarity code to see if
> this argument need really to exists.
>
> Gilles Caulier

Hey Gilles,

I would implement the context menu item Find Duplicates for face tags to lead
to the selection of normal tags. Introducing a face similarity search is far
more complex and could be added to the wishlist. I agree that this should be
included in the handbook.

Concerning the superfluous code: Since the code currently works, I would
postpone the cleanup for after the release of 5.4.
I still have two patches in my working branch which I want to merge today.
After that, I want to make some more tests under different conditions.

Cheers,
Mario

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

[bugzilla_noreply]

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

--- Comment #23 from Mario Frank <[hidden email]> ---
A new patch is available here: https://bugs.kde.org/show_bug.cgi?id=320666

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

[bugzilla_noreply]

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

Wolfgang Scheffner <[hidden email]> changed:

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

--- Comment #24 from Wolfgang Scheffner <[hidden email]> ---
(In reply to Mario Frank from comment #22)

> ...
>
> I would implement the context menu item Find Duplicates for face tags to
> lead to the selection of normal tags. Introducing a face similarity search
> is far more complex and could be added to the wishlist. I agree that this
> should be included in the handbook.
>
> ...
>
> Cheers,
> Mario

I understood Gilles in the way that he just want to make clear in the handbook
that if one chooses "Find Similar..." from a face tag icon in People View
digiKam will compare the whole image, not just the face.
And about the wishlist: what is the point for a similarity search on faces?
What would be the difference to face recognition? I'm not very keen on
explaining that to the users.
And by the way: face recognition is still not very satisfying to me. I don't
even really know how to test it because I don't understand how I can see the
result of a scan. Insofar I would hat to complicate matters, at least right
now.

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

[bugzilla_noreply]

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

--- Comment #25 from Mario Frank <[hidden email]> ---
(In reply to Wolfgang Scheffner from comment #24)

> (In reply to Mario Frank from comment #22)
> > ...
> >
> > I would implement the context menu item Find Duplicates for face tags to
> > lead to the selection of normal tags. Introducing a face similarity search
> > is far more complex and could be added to the wishlist. I agree that this
> > should be included in the handbook.
> >
> > ...
> >
> > Cheers,
> > Mario
>
> I understood Gilles in the way that he just want to make clear in the
> handbook that if one chooses "Find Similar..." from a face tag icon in
> People View digiKam will compare the whole image, not just the face.
> And about the wishlist: what is the point for a similarity search on faces?
> What would be the difference to face recognition? I'm not very keen on
> explaining that to the users.
> And by the way: face recognition is still not very satisfying to me. I don't
> even really know how to test it because I don't understand how I can see the
> result of a scan. Insofar I would hat to complicate matters, at least right
> now.

Hi Wolfgang,
I understood it the same way. It should be made clear. But the functionality
that leads to the selection of the people tags as usual tags in duplicates
search was not present. The context menu item "Find Duplicates" in people
sidebar currently does nothing.
Thus, I proposed to implement this functionality.

With face similarity search, I mean that duplicates are only searched on images
that share a specific face tag.

The problem with face recognition is that it either internally tags a detected
face with a people tag or with nothing.
But there is no way to get a list of faces to which a recognised face is
similar. Filtering detected faces which were not recognised is possible with
the "Unknown" people tag. But filtering detected faces where digikam thinks to
know who it is,
is not possible since the thumbnails are shown in the specific people tag but
not in Unknown. And there are only odd workarounds to get these potentially
wrongly recognised faces which are no good user experience.
Thus, searching for similar faces can be a big improvement. I would not propose
to use HAAR search for this. Instead, The face recognition algorithm could be
used to return faces that seem to be similar to a tagged face.

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

[bugzilla_noreply]

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

Simon <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #103144|0                           |1
        is obsolete|                            |

--- Comment #26 from Simon <[hidden email]> ---
Created attachment 103228
  --> https://bugs.kde.org/attachment.cgi?id=103228&action=edit
Patch for triggering duplicates search for multiple tags with context menu and
refactor.

I took the sections of https://bugs.kde.org/attachment.cgi?id=103203 that are
relevant to this bug and extended the changes. My changes just extend the
refactoring, they don't introduce any change in behaviour:
 - Use the new function selectedTagAlbums instead of selctedTags where
appropriate.
 - Remove any remaining usages of ...FindDuplicatesInAlbum(Album*...
(albumselectiontreeview), such that everything related to duplicates sends
either PAlbums or TAlbums.
 - Rename methods: s/Item/Album/. Items is usually used in the context of
indexes and other functions (currentAlbum) also use this terminology.
I played around with it and in album, tree and people view it worked as
expected. So from my side this is good to merge.

Regarding the similarity search of faces: I think the functionality as it is
currently is fine. Similarity search only on a face area would be more or less
the same as the recognize faces function, wouldn't it?
I absolutely agree that the people interface is still lacking in terms of
usability. It would be great having a way to separate confirmed from
unconfirmed faces. Then searching for similar face would work like: recognize
faces -> select a face tag -> only show unconfirmed faces

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

[bugzilla_noreply]

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

--- Comment #27 from Mario Frank <[hidden email]> ---
(In reply to Simon from comment #26)

> Created attachment 103228 [details]
> Patch for triggering duplicates search for multiple tags with context menu
> and refactor.
>
> I took the sections of https://bugs.kde.org/attachment.cgi?id=103203 that
> are relevant to this bug and extended the changes. My changes just extend
> the refactoring, they don't introduce any change in behaviour:
>  - Use the new function selectedTagAlbums instead of selctedTags where
> appropriate.
>  - Remove any remaining usages of ...FindDuplicatesInAlbum(Album*...
> (albumselectiontreeview), such that everything related to duplicates sends
> either PAlbums or TAlbums.
>  - Rename methods: s/Item/Album/. Items is usually used in the context of
> indexes and other functions (currentAlbum) also use this terminology.
> I played around with it and in album, tree and people view it worked as
> expected. So from my side this is good to merge.
>
> Regarding the similarity search of faces: I think the functionality as it is
> currently is fine. Similarity search only on a face area would be more or
> less the same as the recognize faces function, wouldn't it?
> I absolutely agree that the people interface is still lacking in terms of
> usability. It would be great having a way to separate confirmed from
> unconfirmed faces. Then searching for similar face would work like:
> recognize faces -> select a face tag -> only show unconfirmed faces

Hey Simon,

thanks for the factoring. I will test the two patches and if everything works
as expected give you a confirmation. Can you, too test the
duplicates/sketch/drop/fuzzy search and look whether it conforms to the
description in
https://bugs.kde.org/show_bug.cgi?id=320666#c22 ?

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

[bugzilla_noreply]

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

--- Comment #28 from Mario Frank <[hidden email]> ---
(In reply to Mario Frank from comment #27)

> (In reply to Simon from comment #26)
> > Created attachment 103228 [details]
> > Patch for triggering duplicates search for multiple tags with context menu
> > and refactor.
> >
> > I took the sections of https://bugs.kde.org/attachment.cgi?id=103203 that
> > are relevant to this bug and extended the changes. My changes just extend
> > the refactoring, they don't introduce any change in behaviour:
> >  - Use the new function selectedTagAlbums instead of selctedTags where
> > appropriate.
> >  - Remove any remaining usages of ...FindDuplicatesInAlbum(Album*...
> > (albumselectiontreeview), such that everything related to duplicates sends
> > either PAlbums or TAlbums.
> >  - Rename methods: s/Item/Album/. Items is usually used in the context of
> > indexes and other functions (currentAlbum) also use this terminology.
> > I played around with it and in album, tree and people view it worked as
> > expected. So from my side this is good to merge.
> >
> > Regarding the similarity search of faces: I think the functionality as it is
> > currently is fine. Similarity search only on a face area would be more or
> > less the same as the recognize faces function, wouldn't it?
> > I absolutely agree that the people interface is still lacking in terms of
> > usability. It would be great having a way to separate confirmed from
> > unconfirmed faces. Then searching for similar face would work like:
> > recognize faces -> select a face tag -> only show unconfirmed faces
>
> Hey Simon,
>
> thanks for the factoring. I will test the two patches and if everything
> works as expected give you a confirmation. Can you, too test the
> duplicates/sketch/drop/fuzzy search and look whether it conforms to the
> description in
> https://bugs.kde.org/show_bug.cgi?id=320666#c22 ?

Oh, I just saw that I cannot apply the remaining part of my patch after I apply
your extended patch. I will upload a new patch for my remaining part. There is
some collision in leftsidebarwidgets.

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

[Simon Frei]

On 06/01/17 14:34, Mario Frank wrote:
> https://bugs.kde.org/show_bug.cgi?id=374191
>
> --- Comment #28 from Mario Frank <[hidden email]> ---
> (In reply to Mario Frank from comment #27)
> Oh, I just saw that I cannot apply the remaining part of my patch after I apply
> your extended patch. I will upload a new patch for my remaining part. There is
> some collision in leftsidebarwidgets.
>
The two patches I just uploaded should both be applicable to master and
then merged without conflict. Doesn't that work for you? That would
indicate that I messed up when creating patches.
That is one reason why I like branching and merging much better than
sending around patches, it can so easily create confusion...

[bugzilla_noreply]

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

--- Comment #29 from Simon <[hidden email]> ---
On 06/01/17 14:34, Mario Frank wrote:
> https://bugs.kde.org/show_bug.cgi?id=374191
>
> --- Comment #28 from Mario Frank <[hidden email]> ---
> (In reply to Mario Frank from comment #27)
> Oh, I just saw that I cannot apply the remaining part of my patch after I apply
> your extended patch. I will upload a new patch for my remaining part. There is
> some collision in leftsidebarwidgets.
>
The two patches I just uploaded should both be applicable to master and
then merged without conflict. Doesn't that work for you? That would
indicate that I messed up when creating patches.
That is one reason why I like branching and merging much better than
sending around patches, it can so easily create confusion...

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

[bugzilla_noreply]

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

--- Comment #30 from Mario Frank <[hidden email]> ---
(In reply to Simon from comment #29)

> On 06/01/17 14:34, Mario Frank wrote:
> > https://bugs.kde.org/show_bug.cgi?id=374191
> >
> > --- Comment #28 from Mario Frank <[hidden email]> ---
> > (In reply to Mario Frank from comment #27)
> > Oh, I just saw that I cannot apply the remaining part of my patch after I apply
> > your extended patch. I will upload a new patch for my remaining part. There is
> > some collision in leftsidebarwidgets.
> >
> The two patches I just uploaded should both be applicable to master and
> then merged without conflict. Doesn't that work for you? That would
> indicate that I messed up when creating patches.
> That is one reason why I like branching and merging much better than
> sending around patches, it can so easily create confusion...

I just reset hard to master, applied your dupes patch here, after that your
variant of my patch (https://bugs.kde.org/attachment.cgi?id=103229 ).
It broke in leftsidebarwidgets. I also tried it the other way around. Problem
in the same file but different line. I resolved it on my system. I'm just
compiling digikam. When everything is fine, I will upload my part again.

Branching is easier. But this conflict just occured because I worked on
multiple problems at the same time because they are somehow connected. If we
work on smaller chunks, this should be no problem.

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

[bugzilla_noreply]

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

--- Comment #31 from Simon <[hidden email]> ---
Created attachment 103231
  --> https://bugs.kde.org/attachment.cgi?id=103231&action=edit
Combines patch for this bug and 320666

As I wanted to separate them, both patches are against current master. So to
check both together you would have to create two branches, apply both patches
and then merge these two branches. Or you apply the new patch which is the
combination of the both that I just added.

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

[bugzilla_noreply]

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

--- Comment #32 from Mario Frank <[hidden email]> ---
(In reply to Simon from comment #31)
> Created attachment 103231 [details]
> Combines patch for this bug and 320666
>
> As I wanted to separate them, both patches are against current master. So to
> check both together you would have to create two branches, apply both
> patches and then merge these two branches. Or you apply the new patch which
> is the combination of the both that I just added.

I'll try with the patch you just added. Compiling now.

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