Bug 374191

Git commit 762161177b9862601396b53dea64e6037ef391cc by Gilles Caulier.
Committed on 27/12/2016 at 01:09.
Pushed by cgilles into branch 'master'.

remove obsolete Batch Process menu entries in context menus

M  +0    -28   app/utils/contextmenuhelper.cpp
M  +0    -5    app/utils/contextmenuhelper.h
M  +0    -1    libs/album/albumselectiontreeview.cpp
M  +0    -2    libs/tags/tagfolderview.cpp

https://commits.kde.org/digikam/762161177b9862601396b53dea64e6037ef391cc

Regarding the issue with "Find Duplicates": The correct tags are actually set, but for some reason the check whether this selection is valid (FindDuplicatesView::slotCheckForValidSettings) is false and thus "Tags" is not selected in the "Duplicates" view.

"Find Duplicates" -> confirmed. Just a call is missing somewhere to turn on Tags search instead Albums search.

"Export" -> The current behavior is correct. Only the current selected image in Tag album is exported. It work as main application Export menu. If you want to export whole tag contents, just select all with CTRL+A before.

"Batch Process" -> Fixed...

Gilles Caulier

> caulier.gilles@gmail.com changed:
> [...]
> "Export" -> The current behavior is correct. Only the current selected image in
> Tag album is exported. It work as main application Export menu. If you want to
> export whole tag contents, just select all with CTRL+A before.

I understand how to do it using the main view, but I do not understand
why a menu entry on the tag behaves this way. If there was an export
entry in the context menu on a item in the main view, this behaviour
would be expected. However for export on a tag I would expect that
export applies to this tag, i.e. the tag album. If this behaviour is not
desirable, then this option should either be removed or transferred to
the context menu in the main view. To me it makes no sense to select
images in the main view, then right click the tag in the left sidebar
and select an export tool to export all selected images. I would either
use the top menu bar or right click on one of the selected items in the
main view.

> "Batch Process" -> Fixed...
>
> Gilles Caulier
>
As usual, thanks for the quick response!

"Finde Duplicates" sounds like work for me. Should the search only be done on the Tags? I could try to patch that today.

On 27/12/16 11:52, Mario Frank wrote:
> [...]
>
> --- Comment #5 from Mario Frank <mario.frank@uni-potsdam.de> ---
> "Finde Duplicates" sounds like work for me. Should the search only be done on
> the Tags? I could try to patch that today.
>

Yes, I would expect the search only to be done on the tags that are
selected in the tags view (otherwise what is the point of having the
context menu option, you could just select the fuzzy sidebar). Steps to
reproduce:

1. Go to Duplicates in fuzzy sidebar, select "Albums" in "Search in:"
section.
2. Go to Tags sidebar, right click on a tag and select "Find Duplicates..."
Actual result: The correct tag is set to be searched, but "Albums" is
still selected.
Expected result: Switch to "Tags".


One additional, related but different issue:
I addition the tag sidebar allows to select multiple tags at once, so I
would expect all these tags to be passed to the Duplicate tool.
Currently only the one tag that the right click happens on is passed to
the Duplicates tool.

Simon,

About "Export" problem, this require a re-design/factoring/simplification in kipi interface which must be postponed in the future. So typically, i don't want to touch this implementation. Please open a separated bug for that, in digiKam/kipiinterface section.

Gilles Caulier

Git commit 319d66b5062d145ce4e04a23df7ecf68bfbfecfb by Mario Frank.
Committed on 29/12/2016 at 19:26.
Pushed by mfrank into branch 'master'.

Using the context menu in tags and album tree view for finding duplicates now not only selects the selected tag/album for duplicates search
but also switches to the correct search mode.

M  +11   -0    libs/album/albumselectors.cpp
M  +5    -0    libs/album/albumselectors.h
M  +2    -0    utilities/fuzzysearch/findduplicatesview.cpp

https://commits.kde.org/digikam/319d66b5062d145ce4e04a23df7ecf68bfbfecfb

Simon,

your original bug report for "Find Duplicates" is resolved. I do not trigger the duplicates search since the user may want to modify the similarity interval. But the correct section (album/tag) is now selected. I already tested it. Nevertheless, please confirm whether the functionality is correct.

Concerning the selection of multiple tags for context menu: this needs some more work. Should this be also included in the 5.4 milestone? If not, we should open a bug for this specific case.

>Concerning the selection of multiple tags for context menu: this needs some more >work. Should this be also included in the 5.4 milestone? If not, we should open >a bug for this specific case.

You decide. 5.4.0 is postponed to 8 January now. If you have enough time to do it, let's go. Note: it's always better to touch same code at closed time, it's more easy, instead to delay to much changes.

If you need more time, i can again postpone 5.4.0. There is no special reason to publish 5.4.0 in first week of January. Let's me hear.

Gilles

> --- Comment #10 from caulier.gilles@gmail.com ---
>> Concerning the selection of multiple tags for context menu: this needs some more >work. Should this be also included in the 5.4 milestone? If not, we should open >a bug for this specific case.
> You decide. 5.4.0 is postponed to 8 January now. If you have enough time to do
> it, let's go. Note: it's always better to touch same code at closed time, it's
> more easy, instead to delay to much changes.
>
> If you need more time, i can again postpone 5.4.0. There is no special reason
> to publish 5.4.0 in first week of January. Let's me hear.
>
> Gilles
>

Thanks a lot for the quick fixes!
From my point of view this is a minor issue, I did not report it because
it is important for my use of digikam. It is just from the UI, I would
expect this to work this way as a user (both involved functions allow
multiple tags to be selected, so the link between them should also
handle multiple tags). And as I am just reporting and not fixing, I
certainly don't have any say in when this should be done.
However I would suggest not to postpone 5.4.0 again, at least not for
this. There are for sure some people out there waiting for the release
(even if its just one), and disappointing them needlessly is not so nice.

Hey Gilles,

I would not like to have the release postponed again. There is enough time for testing and this modification for me and I agree with Simon. We should only postpone for stability. This glitch should not be a reason. I'll look at it over the weekend and make some more tests.

Cheers,
Mario

Hi Simon,
hi Gilles,

I got a working solution for initialising a duplicates search (selecting the tags) for multiple selected tags. It works quite well. 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 will append a patch. Although I tested it on my machine, I would like to have someone else having a look over it. Can you test the functionality, too, Simon?

Another glitch for which I will open a bug is: In tags tree view, multiple tags can be selected. But in Albums tree view, this is not possible. Perhaps this should be enabled in DK 5.5.

Created attachment 103144 [details]
Patch for triggering duplicates search for multiple tags with context menu.

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*>)?

(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.

(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.

(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?

(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.

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

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

(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

A new patch is available here: https://bugs.kde.org/show_bug.cgi?id=320666

(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.

(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.

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

(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 ?

(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.

On 06/01/17 14:34, Mario Frank wrote:
> https://bugs.kde.org/show_bug.cgi?id=374191
>
> --- Comment #28 from Mario Frank <mario.frank@uni-potsdam.de> ---
> (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...

(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 <mario.frank@uni-potsdam.de> ---
> > (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.

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.

(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.

Git commit 429fa5fd8e7f53b74c82eb19dffb2e6cf4b4325c by Mario Frank.
Committed on 06/01/2017 at 18:36.
Pushed by mfrank into branch 'master'.

This patch is joint work with Simon. The patch introduces the similarity of images to a specific image
as column in table view. Also, it fixes the sorting by similarity in sketch search.
Moreover, the dysfunctional context menu item "Find duplicates" in people sidebar now leads to the selection
of the people tags as regular tabs in duplicates search. Finally, selecting multiple regular/people tags for
duplicates search is possible. Not to mention some refactoring to make function names more fitting.
Related: bug 320666, bug 302923
FIXED-IN: 5.4.0

M  +1    -1    NEWS
M  +21   -30   app/views/digikamview.cpp
M  +5    -4    app/views/digikamview.h
M  +17   -15   app/views/leftsidebarwidgets.cpp
M  +6    -8    app/views/leftsidebarwidgets.h
M  +34   -2    app/views/tableview/tableview_column_item.cpp
M  +2    -1    app/views/tableview/tableview_column_item.h
M  +1    -1    libs/album/albumselectiontreeview.cpp
M  +1    -1    libs/album/albumselectiontreeview.h
M  +5    -0    libs/album/albumtreeview.cpp
M  +2    -1    libs/album/albumtreeview.h
M  +10   -1    libs/database/dbjobs/dbjob.cpp
M  +45   -30   libs/database/haar/haariface.cpp
M  +5    -5    libs/database/haar/haariface.h
M  +14   -0    libs/database/item/imageinfo.cpp
M  +5    -0    libs/database/item/imageinfo.h
M  +2    -0    libs/database/item/imageinfodata.h
M  +106  -6    libs/database/item/imagelister.cpp
M  +8    -1    libs/database/item/imagelister.h
M  +10   -6    libs/database/item/imagelisterrecord.h
M  +0    -6    libs/models/imagefiltermodel.cpp
M  +0    -1    libs/models/imagefiltermodel.h
M  +5    -10   libs/models/imagesortsettings.cpp
M  +0    -2    libs/models/imagesortsettings.h
M  +0    -2    libs/settings/applicationsettings.cpp
M  +0    -3    libs/settings/applicationsettings.h
M  +0    -10   libs/settings/applicationsettings_miscs.cpp
M  +0    -4    libs/settings/applicationsettings_p.cpp
M  +0    -2    libs/settings/applicationsettings_p.h
M  +3    -1    libs/tags/tagfolderview.cpp
M  +1    -1    libs/tags/tagfolderview.h
M  +3    -3    libs/tags/tagsmanager/tagsmanager.cpp
M  +16   -16   utilities/fuzzysearch/findduplicatesview.cpp
M  +5    -2    utilities/fuzzysearch/findduplicatesview.h
M  +22   -20   utilities/fuzzysearch/fuzzysearchview.cpp
M  +5    -4    utilities/fuzzysearch/fuzzysearchview.h

https://commits.kde.org/digikam/429fa5fd8e7f53b74c82eb19dffb2e6cf4b4325c