extragear/graphics/digikam/utilities/cameragui

SVN commit 1035366 by aclemens:

Do not refresh the icon view for every single emitted signal from the
CameraController.
This will not only lock the GUI, but also lead to enormous memory consumption.
Why? Because files get reloaded as much as signals are emitted from the
CameraController.

If you open a folder with 200 subfolders, the iconview and its content gets
reloaded 200 times.

For my testfolder this meant that over 2 million images were loaded as
thumbnails, and this is NOT very system friendly :)

The CameraUI is much more responsive now, but still there are issues:

- The do...while loop in cameraui.cpp:1179 will lock the UI while all the
  images are processed. Problem: We can not set the CameraUI window to busy,
  and therefore it looks like the download has already been completed.

- Sometimes the thumbnail generation can not be canceled, but I have not
  figured out why yet.

Maybe it would be best to move the code from the do...while loop into the
CameraController thread and emit signals?

CCMAIL:[hidden email]

 M  +25 -1     cameraui.cpp  
 M  +2 -0      cameraui.h  
 M  +4 -0      cameraui_p.h  


--- trunk/extragear/graphics/digikam/utilities/cameragui/cameraui.cpp #1035365:1035366
@@ -145,6 +145,15 @@
 
     // -------------------------------------------------------------------
 
+    d->refreshIconViewTimer = new QTimer(this);
+    d->refreshIconViewTimer->setInterval(0);
+    d->refreshIconViewTimer->setSingleShot(true);
+
+    connect(d->refreshIconViewTimer, SIGNAL(timeout()),
+            this, SLOT(slotRefreshIconView()));
+
+    // -------------------------------------------------------------------
+
     setupUserArea();
     setupStatusBar();
     setupActions();
@@ -1087,6 +1096,18 @@
     if (fileList.empty())
         return;
 
+    d->filesToBeAdded << fileList;
+    slotRefreshIconView();
+}
+
+void CameraUI::slotRefreshIconView()
+{
+    if (d->busy)
+    {
+        d->refreshIconViewTimer->start();
+        return;
+    }
+
     AlbumSettings* settings = AlbumSettings::instance();
     if (!settings) return;
 
@@ -1117,7 +1138,7 @@
         citem = static_cast<CameraIconItem*>(citem->nextItem());
     }
 
-    foreach(const GPItemInfo& item, fileList)
+    foreach(const GPItemInfo& item, d->filesToBeAdded)
     {
         info.setFile(item.name);
         if (!fileNames.contains(info.fileName().toLower()) &&
@@ -1153,11 +1174,14 @@
     QList<QVariant> itemsList;
 
     it = lastPhotoFirst ? map.end() : map.begin();
+
+    // This can be slow and lock the GUI. Maybe we need to move this into the camera controller thread?
     do
     {
         if (lastPhotoFirst) --it;
 
         item = *it;
+
         // We query database to check if item have been already downloaded from camera.
         switch(DownloadHistory::status(d->controller->cameraMD5ID(), item.name, item.size, item.mtime))
         {
--- trunk/extragear/graphics/digikam/utilities/cameragui/cameraui.h #1035365:1035366
@@ -208,6 +208,8 @@
 
     void slotSidebarTabTitleStyleChanged();
 
+    void slotRefreshIconView();
+
 private:
 
     CameraUIPriv* const d;
--- trunk/extragear/graphics/digikam/utilities/cameragui/cameraui_p.h #1035365:1035366
@@ -116,6 +116,7 @@
         kdeJob                    = 0;
         historyView               = 0;
         lastPhotoFirstAction      = 0;
+        refreshIconViewTimer      = 0;
     }
 
     bool                          deleteAfter;
@@ -130,6 +131,7 @@
     QStringList                   currentlyDeleting;
     QStringList                   cameraFolderList;
     QSet<QString>                 foldersToScan;
+    GPItemInfoList                filesToBeAdded;
 
     QCheckBox                    *autoRotateCheck;
     QCheckBox                    *autoAlbumDateCheck;
@@ -140,6 +142,8 @@
     QLabel                       *formatLabel;
     QLabel                       *folderDateLabel;
 
+    QTimer                       *refreshIconViewTimer;
+
     KMenu                        *downloadMenu;
     KMenu                        *deleteMenu;
     KMenu                        *imageMenu;
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Gilles Caulier-4]

Andi,

This loop query the database. I'm not sure if DB methods are thread
safe. Anyway, i don't think that moving this code in camera ctrl is
the right place. Writting another little QThread wrapper to run this
code will be better i think.

Gilles

2009/10/14 Andi Clemens <[hidden email]>:

> SVN commit 1035366 by aclemens:
>
> Do not refresh the icon view for every single emitted signal from the
> CameraController.
> This will not only lock the GUI, but also lead to enormous memory consumption.
> Why? Because files get reloaded as much as signals are emitted from the
> CameraController.
>
> If you open a folder with 200 subfolders, the iconview and its content gets
> reloaded 200 times.
>
> For my testfolder this meant that over 2 million images were loaded as
> thumbnails, and this is NOT very system friendly :)
>
> The CameraUI is much more responsive now, but still there are issues:
>
> - The do...while loop in cameraui.cpp:1179 will lock the UI while all the
>  images are processed. Problem: We can not set the CameraUI window to busy,
>  and therefore it looks like the download has already been completed.
>
> - Sometimes the thumbnail generation can not be canceled, but I have not
>  figured out why yet.
>
> Maybe it would be best to move the code from the do...while loop into the
> CameraController thread and emit signals?
>
> CCMAIL:[hidden email]
>
>  M  +25 -1     cameraui.cpp
>  M  +2 -0      cameraui.h
>  M  +4 -0      cameraui_p.h
>
>
> --- trunk/extragear/graphics/digikam/utilities/cameragui/cameraui.cpp #1035365:1035366
> @@ -145,6 +145,15 @@
>
>     // -------------------------------------------------------------------
>
> +    d->refreshIconViewTimer = new QTimer(this);
> +    d->refreshIconViewTimer->setInterval(0);
> +    d->refreshIconViewTimer->setSingleShot(true);
> +
> +    connect(d->refreshIconViewTimer, SIGNAL(timeout()),
> +            this, SLOT(slotRefreshIconView()));
> +
> +    // -------------------------------------------------------------------
> +
>     setupUserArea();
>     setupStatusBar();
>     setupActions();
> @@ -1087,6 +1096,18 @@
>     if (fileList.empty())
>         return;
>
> +    d->filesToBeAdded << fileList;
> +    slotRefreshIconView();
> +}
> +
> +void CameraUI::slotRefreshIconView()
> +{
> +    if (d->busy)
> +    {
> +        d->refreshIconViewTimer->start();
> +        return;
> +    }
> +
>     AlbumSettings* settings = AlbumSettings::instance();
>     if (!settings) return;
>
> @@ -1117,7 +1138,7 @@
>         citem = static_cast<CameraIconItem*>(citem->nextItem());
>     }
>
> -    foreach(const GPItemInfo& item, fileList)
> +    foreach(const GPItemInfo& item, d->filesToBeAdded)
>     {
>         info.setFile(item.name);
>         if (!fileNames.contains(info.fileName().toLower()) &&
> @@ -1153,11 +1174,14 @@
>     QList<QVariant> itemsList;
>
>     it = lastPhotoFirst ? map.end() : map.begin();
> +
> +    // This can be slow and lock the GUI. Maybe we need to move this into the camera controller thread?
>     do
>     {
>         if (lastPhotoFirst) --it;
>
>         item = *it;
> +
>         // We query database to check if item have been already downloaded from camera.
>         switch(DownloadHistory::status(d->controller->cameraMD5ID(), item.name, item.size, item.mtime))
>         {
> --- trunk/extragear/graphics/digikam/utilities/cameragui/cameraui.h #1035365:1035366
> @@ -208,6 +208,8 @@
>
>     void slotSidebarTabTitleStyleChanged();
>
> +    void slotRefreshIconView();
> +
>  private:
>
>     CameraUIPriv* const d;
> --- trunk/extragear/graphics/digikam/utilities/cameragui/cameraui_p.h #1035365:1035366
> @@ -116,6 +116,7 @@
>         kdeJob                    = 0;
>         historyView               = 0;
>         lastPhotoFirstAction      = 0;
> +        refreshIconViewTimer      = 0;
>     }
>
>     bool                          deleteAfter;
> @@ -130,6 +131,7 @@
>     QStringList                   currentlyDeleting;
>     QStringList                   cameraFolderList;
>     QSet<QString>                 foldersToScan;
> +    GPItemInfoList                filesToBeAdded;
>
>     QCheckBox                    *autoRotateCheck;
>     QCheckBox                    *autoAlbumDateCheck;
> @@ -140,6 +142,8 @@
>     QLabel                       *formatLabel;
>     QLabel                       *folderDateLabel;
>
> +    QTimer                       *refreshIconViewTimer;
> +
>     KMenu                        *downloadMenu;
>     KMenu                        *deleteMenu;
>     KMenu                        *imageMenu;
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel
>

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

On Wednesday 14 October 2009 23:09:09 Gilles Caulier wrote:
> Andi,
>
> This loop query the database. I'm not sure if DB methods are thread
> safe. Anyway, i don't think that moving this code in camera ctrl is
> the right place. Writting another little QThread wrapper to run this
> code will be better i think.

But if it is not thread-safe, another QThread wrapper will not be useful too,
or am I misunderstanding this now?

Andi
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Gilles Caulier-4]

Excepted if lock mechanism is implemented in DB interface

Gilles

2009/10/14 Andi Clemens <[hidden email]>:

>
> On Wednesday 14 October 2009 23:09:09 Gilles Caulier wrote:
>> Andi,
>>
>> This loop query the database. I'm not sure if DB methods are thread
>> safe. Anyway, i don't think that moving this code in camera ctrl is
>> the right place. Writting another little QThread wrapper to run this
>> code will be better i think.
>
> But if it is not thread-safe, another QThread wrapper will not be useful too,
> or am I misunderstanding this now?
>
> Andi
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel
>

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

Oh I meant "moev the method into the DownloadHistory", not CameraController.
This could be a thread (if safe) and emit signals, and the CameraUI reacts on
them.
Instead of providing a single item like we do now, we could provide the whole
list (map) and then return to the main event loop?

Andi

On Wednesday 14 October 2009 23:16:14 Gilles Caulier wrote:

> Excepted if lock mechanism is implemented in DB interface
>
> Gilles
>
> 2009/10/14 Andi Clemens <[hidden email]>:
> > On Wednesday 14 October 2009 23:09:09 Gilles Caulier wrote:
> >> Andi,
> >>
> >> This loop query the database. I'm not sure if DB methods are thread
> >> safe. Anyway, i don't think that moving this code in camera ctrl is
> >> the right place. Writting another little QThread wrapper to run this
> >> code will be better i think.
> >
> > But if it is not thread-safe, another QThread wrapper will not be useful
> > too, or am I misunderstanding this now?
> >
> > Andi
> > _______________________________________________
> > Digikam-devel mailing list
> > [hidden email]
> > https://mail.kde.org/mailman/listinfo/digikam-devel
>
> _______________________________________________
> Digikam-devel mailing list
> [hidden email]
> https://mail.kde.org/mailman/listinfo/digikam-devel
>

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel