Bug 261417

Eric, 

It still valid with digiKam 2.x serie ?

Gilles Caulier

Eric,

This file still valid using last digiKam 4.2.0 ?

Gilles Cauleir

I can confirm that one both in 5.3 and master branch. I looked at the code on the weekend. It seems like the deletion is not communicated to the TreeView since I found no Signal/Slot-communication concerning this one. I will take a deeper look at it and try to fix that when I have some time.

Created attachment 102157 [details]
Patch for the bug.

Tested this patch. Seems to work quite well. Please review it.

Thanks for the patch Mario. Delete to trash over right click on context menu does not work here. The Item counter remains the same. Del key works. I'll look at it tomorrow.

Maik

Hey, I just tested again. Deleting multiple pictures (selection) at the same time
does not work properly, too. The counter is only decremented by 1, not by the count of deleted images. Seems that the signal imagedeleted is only emitted once even if multiple images are deleted. I can take a look at the code again and try to fix both problems.

I can't check the patch actually but I suspect that you fired a signal from icon view item context menu to duplicates list view to decrease count of items. This is not the right way. You must fired a signal to database iOS job to refresh search album for duplicates. This will update properties the virtual album in database. 

THE process is similar that to refresh all duplicates virtual album from button on left sidebar, but it must only to be processed on current selected duplicates virtual search album.

Gilles caulier

Okay. In fact I fired a signal to the AlbumManager and then to the FindDuplicatesView. I will take a look at the problem more closely. 
Is there an overview concerning wirings? And is there an UI definition that can be used in QTDesigner to get an overview of which component is used in which scope?

Cheers,
Mario

What d'où mean by wiring ?
About qtdesigner : no definition exists yet, because we use mostly Kate editor

I just discovered that there is also an issue with the duplicate count ordering. It is done lexicographically not mathematically (e.g. 10 is before 2). I thought this is probably related, if I should open a separate bug, please say so.

I do not know how the developers opinions are. But I think what you see is part of the problem described in bug 372217 . I already adressed the lexicographical ordering of the simmilarity. I will provide a patch for your problem at the site for bug 372217.

Cheers,
Mario

yes a separate file will be fine...

Gilles Caulier

Hey Gilles,
by the wiring (of components), I mean the connection of signals to slots. The lack on information about which class is the correct view in which situation makes it complicated for me to get find out where to set a hook and trigger specific updates. Is there some dev documentation going into the details of the implementation?

Hey Simon,

I can open a bug then and submit the patch.

Cheers,
Mario

A good start is to explore the DigiKam namespace and the doxygen documentation relevant.

https://api.kde.org/extragear-api/graphics-apidocs/digikam/html/namespaceDigikam.html

The class names are identical than cpp file names everywhere.

Each classes are hosted in subdirectory by functions, as image container, database interface, filters, widgets, etc...

Please give me more info about what you search exactly in source code.

Gilles Caulier

Hey Gilles,
there are obviously 3 ways to delete an image:
1 pressing the delete key
2 klicking the right mouse button on the image and move to trash
3 moving to trash with the main menu (entry->move to trash)
The variant 1 seems to trigger variant 3 since it is a shortcut.
There is a signal from stackedView to DigikamView (imageDeleted) that
works for Variant 1 and 3 but not for variant 2.

What I am looking for is the spot where I can catch all three variant events.

Moreover, I am not sure what you mean with the database iOS job. I did not find any definition comparable.

>there are obviously 3 ways to delete an image:
>1 pressing the delete key
>2 klicking the right mouse button on the image and move to trash
>3 moving to trash with the main menu (entry->move to trash)
>The variant 1 seems to trigger variant 3 since it is a shortcut.

yes it is.

>There is a signal from stackedView to DigikamView (imageDeleted) that
>works for Variant 1 and 3 but not for variant 2.

exact. In this case, the contextual menu is managed by a common class :

https://quickgit.kde.org/?p=digikam.git&a=blob&f=app%2Futils%2Fcontextmenuhelper.cpp

Look this method :

void ContextMenuHelper::addStandardActionItemDelete(QObject* recv, const char* slot, int quantity)

... which permit to plug an action to the menu to delete and item from icon view.

Moreover, I am not sure what you mean with the database iOS job. I did not find any definition comparable.

Since 5.0.0, we have drop all KIOSlave and replaced by 2 multithreaded interfaces, one for file management (rename, copy, delete, etc...), and one to query the database. The last one is located here :

https://quickgit.kde.org/?p=digikam.git&a=blob&f=libs%2Fdatabase%2Fdbjobs%2Fdbjob.cpp

Look class SearchesJob which will manage all background Duplicates search queries in database.

Typically, at end, when you process a duplicates search album to get result, this DBJob is called. 

Typically, to refresh one duplicates search album, you must to call this DBJob to process iface.rebuildDuplicatesAlbums() with right arguments to only handle albums where you have removed duplicates items (from icon-view your can remove more than one at the same time...)

Gilles Caulier

The DBJob for duplicates search album must be called through DuplicatesFinder maintenance tool, as i commented here :

https://bugs.kde.org/show_bug.cgi?id=372435#c2

This must automatically start a progress manager task, with progress info and results. It's simple...

Gilles Caulier

Hey,

I found all that spots when I extended the duplicates search to the interval variant. And I did see the contextmenuhelper, too. 

And I traced the signal-slot-connections. The image preview view signals the stacked view that signals the digikam view. This one then signals either the table or image view  which then delete the selected images.
In both cases, an update of the modified albums is necessary. Since now multiple SAlbums can be selected and one image can be part of multiple SAlbums, I can have n SAlbums for update.

Thus, I first have to find out which albums need to be updated. And since I can jump from an SAlbum with the context menu to a physical album and delete the duplicate image there, I am not even in the context of the duplicates search.

Am I overseeing something? Is there a good spot to catch which SAlbums were modified?

>Thus, I first have to find out which albums need to be updated. And since I can >jump from an SAlbum with the context menu to a physical album and delete the >duplicate image there, I am not even in the context of the duplicates search.

Well, it's simple. If one image can be present in more than one SAlbum, all relevant SAlbum must be updated through DuplicatesFinder tool.

CoreDb class can be your friend here. But as i can see the relevant method is missing about search album.

Another question : In avanced search tool, we can show items relevant of complex DB queries. What's happen when i delete an item from a SAlbum in this view ?

Gilles Caulier

(In reply to caulier.gilles from comment #19)
> 
> Well, it's simple. If one image can be present in more than one SAlbum, all
> relevant SAlbum must be updated through DuplicatesFinder tool.
> 
> CoreDb class can be your friend here. But as i can see the relevant method
> is missing about search album.
> 
Okay, I'll try to get that done.

> Another question : In avanced search tool, we can show items relevant of
> complex DB queries. What's happen when i delete an item from a SAlbum in
> this view ?

I will have to look at the code.

Thanks for the hints.

(In reply to Mario Frank from comment #20)
> (In reply to caulier.gilles from comment #19)
> > 
> > Well, it's simple. If one image can be present in more than one SAlbum, all
> > relevant SAlbum must be updated through DuplicatesFinder tool.
> > 
> > CoreDb class can be your friend here. But as i can see the relevant method
> > is missing about search album.
> > 
> Okay, I'll try to get that done.
> 
> > Another question : In avanced search tool, we can show items relevant of
> > complex DB queries. What's happen when i delete an item from a SAlbum in
> > this view ?
> 
> I will have to look at the code.
> 
> Thanks for the hints.

Hey Gilles,

The queries of advanced searches do not contain any image ids, the search should not have to be updated. Was this what you wanted to know?

Nevertheless, I crawled through the source code and have a working solution. 
Since I am not sure if it is the best way to solve that point, I will describe the approach more closely to make my thoughts clear.

If some image (or some images) is deleted, I have to find out whether this image is part of a duplicates search. I will come to the way back later.
The easiest way is to get all SAlbums that have DuplicateSearch as type. Filtering them out is easy. Since I wanted to create reusable code, I created a function that is able to filter all SAlbums by type. I could do that in-database. But since the SAlbums are already present in the AlbumManager, the functionality is located best there. Then I get all SAlbums that represent the duplicate search, get all SAlbums that contain the deleted image(s) (by using SearchXmlReader) and buffer the ids of the images contained in those albums (subtracting the deleted ones).

In the next step, I delete those SAlbums. I do that in order to have clean SAlbums after rescanning for duplicates. Normally, if a duplicates search is finished, all SAlbums for duplicates are deleted. But I want to delete only the ones that were updated. And here comes the important spot. If an image is deleted which reduces a SAlbum to size 1, this SAlbum is not created and following this, the old SAlbum would not be deleted and would stay in the duplicates view.

Since I have now all images that need to be rescanned, I can communicate those images to the DuplicatesFinder. A new search is triggered and the updated SAlbums are created.

My solution works for every variant for deleting images (context menu, main menu and delete button). Also, the duplicates search does not need to be in focus. If it is not, the duplicates search runs in background.

Now back to the part on getting notified about the deleted image. After crawling through the code, I found out, that all deletion methods use the imageViewUtilities. There, I have the information which images are deleted.
Thus I send a signal to the AlbumManager to check whether some SAlbums become invalid. Also, I wire the AlbumManager with a signal to the FindDuplicatesView that triggers the DuplicatesSearch on the set of images.

Is this a more fitting way to solve that?

Cheers,
Mario

Okay, I looked for other ways to signal the AlbumManager about the deleted images and I did not find performant ways to do this in another way.

Thus, I will upload the patch. Here is the commit message:
"[PATCH] This commit fixes bug 261417. When images are deleted, the
 ImageViewUtilities give the ImageInfos to the DIO. In the ImageViewUtilities,
 the ids of the deleted images are buffered in a list and given to the
 AlbumManager via Signal-Slot-communication. I do that since in database
 layer, only the URIs of the images are present and getting the image ids is
 unnecessary effort. Since the AlbumManager already has all SAlbums, it is
 easy to first get all SAlbums that represent DuplicateSearches (I implemented
 a method to get all SAlbums by type) and then get all those albums that
 contain the deleted images (SearchXmlReader is needed here).

In order to have a good performance and do no unnecessary work, the AlbumManager unites
all image ids from the SAlbums to update and subtracts the ids of the deleted images (set operations).
This way, I am able to communicate the set of images to rescan to the FindDuplicatesView.
Before notifying the FindDuplicatesView, I delete all SAlbums that should be updated.
The reason: Consider an SAlbum that contained two duplicates - the original image and the now deleted
image. If I start a normal duplicates search, no SAlbum will be created for the original image and
the SAlbum will thus not be updated and should not be in the final result set.

Since I now know the ids of the images to rescan, I notify the FindDuplicatesView.
But now I do not have albums to search in - and in fact this would not really be performant since
uninvolved images would be scanned. Thus, I extended the DuplicatesFinder with a new constructor
for searching for duplicates in a set of images. This could be applicable in other situations, too.
To make this work, I extended the DB-Jobinfo with a switch for update of duplicates where the
set of images is and not the albums and tags is used for duplicates search. This way, I can set the
image ids and the update flag in the job info. The DBJob triggers a new variant of rebuildDuplicates
in haariface where the image ids are taken directly.

With this approach, only the SAlbums that need to be updated are updated. SAlbums are only lost,
if no duplicates exist due to deletion which is appropriate. Previously not existing SAlbums cannot be created
since no new images are involved.
"

By the way, I found some flaw that can lead to an unexpected set of SAlbums. I'll open another file soon.

Cheers,
Mario

Created attachment 102381 [details]
The patch that triggers an update of involved SAlbums on deletion of images.

Mario,

The analysis is excelent and the way to implement the solution sound good.

I will review your patch.

Gilles Caulier

Git commit 2183d3849f857ef1c04cf765fa357a211a8757d8 by Gilles Caulier.
Committed on 22/11/2016 at 12:01.
Pushed by cgilles into branch 'master'.

Next stage to improve FuzzySearch management to apply patch #102381 from Mario Frank

When images are deleted, the ImageViewUtilities give
the ImageInfos to the DIO. In the ImageViewUtilities,
the ids of the deleted images are buffered in a list and given to the
AlbumManager via Signal-Slot-communication. I do that since in database
layer, only the URIs of the images are present and getting the image ids is
unnecessary effort. Since the AlbumManager already has all SAlbums, it is
easy to first get all SAlbums that represent DuplicateSearches (I implemented
a method to get all SAlbums by type) and then get all those albums that
contain the deleted images (SearchXmlReader is needed here).

In order to have a good performance and do no unnecessary work, the AlbumManager unites
all image ids from the SAlbums to update and subtracts the ids of the deleted images (set operations).
This way, I am able to communicate the set of images to rescan to the FindDuplicatesView.
Before notifying the FindDuplicatesView, I delete all SAlbums that should be updated.
The reason: Consider an SAlbum that contained two duplicates - the original image and the now deleted
image. If I start a normal duplicates search, no SAlbum will be created for the original image and
the SAlbum will thus not be updated and should not be in the final result set.

Since I now know the ids of the images to rescan, I notify the FindDuplicatesView.
But now I do not have albums to search in - and in fact this would not really be performant since
uninvolved images would be scanned. Thus, I extended the DuplicatesFinder with a new constructor
for searching for duplicates in a set of images. This could be applicable in other situations, too.
To make this work, I extended the DB-Jobinfo with a switch for update of duplicates where the
set of images is and not the albums and tags is used for duplicates search. This way, I can set the
image ids and the update flag in the job info. The DBJob triggers a new variant of rebuildDuplicates
in haariface where the image ids are taken directly.

With this approach, only the SAlbums that need to be updated are updated. SAlbums are only lost,
if no duplicates exist due to deletion which is appropriate. Previously not existing SAlbums cannot be created
since no new images are involved.
FIXED-IN: 5.4.0

M  +19   -0    app/items/imageviewutilities.cpp
M  +1    -0    app/items/imageviewutilities.h
M  +63   -0    libs/album/albummanager.cpp
M  +10   -0    libs/album/albummanager.h
M  +17   -7    libs/database/dbjobs/dbjob.cpp
M  +21   -0    libs/database/dbjobs/dbjobinfo.cpp
M  +14   -6    libs/database/dbjobs/dbjobinfo.h
M  +36   -6    libs/database/haar/haariface.cpp
M  +16   -0    libs/database/haar/haariface.h
M  +17   -0    utilities/fuzzysearch/findduplicatesview.cpp
M  +1    -0    utilities/fuzzysearch/findduplicatesview.h
M  +20   -2    utilities/maintenance/duplicatesfinder.cpp
M  +3    -0    utilities/maintenance/duplicatesfinder.h

http://commits.kde.org/digikam/2183d3849f857ef1c04cf765fa357a211a8757d8