Bug 146083

Created attachment 21138 [details]
ensure visible items for both left and right panel

The attached patch addresses point 2. by putting the current
image into the left panel and the next (if it exists, or the very first)
as the right image.
The only thing which I did not succeed in is to make the currently
selected image of the thumb-bar visible (somehow the ensureItemVisible does
not work?).
Mikolaj, could you try this patch and see if it works for you
(there are different ways to populate and use the light-table, so it
is not unlikely, that one case slipped through).
Please also check point 3 of your post, if this problem is still true.

> The attached patch addresses point 2. by putting the current
> image into the left panel and the next (if it exists, or the very first)
> as the right image.


Many, many thanks.

> The only thing which I did not succeed in is to make the currently
> selected image of the thumb-bar visible (somehow the ensureItemVisible
> does not work?).


Sorry, don't understand that.

> Mikolaj, could you try this patch and see if it works for you
> (there are different ways to populate and use the light-table, so it
> is not unlikely, that one case slipped through).
> Please also check point 3 of your post, if this problem is still true.


Problem here becomes even stranger: when dragging image N to the Master
position, image in Slave position is replaced with image N+1 (when N is
last image in thumb-bar N+1=1). IMO Slave image should be left
untouched.

One thing: when removing Master image its panel is left blank. IMO it is
not good solution. When someone is removing Master image from LT it is
usually because he found that Slave image is much better. I think Slave
image should be promoted to Master position and right panel should be
filled with next image.

> > The only thing which I did not succeed in is to make the currently
> > selected image of the thumb-bar visible (somehow the ensureItemVisible
> > does not work?).
>
> Sorry, don't understand that.


It is a minor useability issue:
When you add new images the first and second will be placed
in left and right panel, but the item which is hightlighted in
the thumb-bar need not be visible (if there are sufficiently many
entries). I.e., I would like that the slider moves to the
position so that the hightlighted item is visible.
Normally this should work with ``ensureItemVisible'', but
it does not here ...

Problem A)
> Problem here becomes even stranger: when dragging image N to the Master
> position, image in Slave position is replaced with image N+1 (when N is
> last image in thumb-bar N+1=1). IMO Slave image should be left
> untouched.


Yes, I agree.

Problem B): Moreover, trying to replace the slave, it ends up
on both images!

Problem C):
> One thing: when removing Master image its panel is left blank. IMO it is
> not good solution. When someone is removing Master image from LT it is
> usually because he found that Slave image is much better. I think Slave
> image should be promoted to Master position and right panel should be
> filled with next image.


Makes perfect sense to me.

Created attachment 21142 [details]
patch which also addresses problem C)

This patch also tries to address problem C).
There are some corner cases, where it behaves non-optimal.

Warning: it may crash digikam (don't test it on an important image collection!)


Concerning problems A) and B) I have no idea what is causing this
behaviour (I don't see how my patch should be responsible, but
it has to be, because it is different from the behaviour without patch,
where fore example trying to replace the master leads to the same image
in both panels, while exchanging the slave works perfectly fine there ...)

> Problem B): Moreover, trying to replace the slave, it ends up
> on both images!

That is big turn off. While there is some (questionable IMO) logic
behind setting both images to Master it is unacceptable. :(

> This patch also tries to address problem C).
> There are some corner cases, where it behaves non-optimal.

Yes, works nicely for me. And yes, there are situations where new Slave
is strange. Here is my shot: it replaces slave with image next to old
Master. But usually this is image which was already "discarded" when
going from beginning image to image. New Slave should be get from image
next to new Master. This is all according to my comparison habits,
please comment if you (not only Arnd ;) have other experiences.
> Warning: it may crash digikam (don't test it on an important image
> collection!)

Works for me without crashes.

Created attachment 21156 [details]
patch to address A) and B)

approach to C) is taken out at the moment - the logic
got too complicated and needs to be checked again.
Will tackle that later.
Concerning drag-and-drop (and adding via F6), with
this patch it  behaves pretty "logic" to me...

> ------- Created an attachment (id=21156)

This patch takes care about core of bug report: no automagical replacing
of images + putting first two images of LT into panels. I am quite happy
with that and you can feel free to close this entry after applying patch
to SVN. For replacing of deleted images I will open another entry (Bug
147889).

Hi Mikolaj,

thanks a lot for testing!
Before the code is fine for inclusion,
some of the DWarning()'s have to be removed.
Also I need to think about whether the new
setLeftRightItems is necessary or can be integrated into
loadImageInfos.

Arnd

Created attachment 21234 [details]
patch to address A) and B), clean-up.

Cleaned up version of the patch. Looks ok from my side now.

This patch solves A and B note however C is once again completely broken. When removing Master image it remains empty but Slave is replaced by image next to former Master. I understand Slave should be promoted to Master but it doesn't happen. It is removed from panels - of course remaining in thumbbar.

SVN commit 707040 by abaecker:

ensure that left and right view of the lighttable are populated properly on F6
BUG: 146083
TODO:KDE4PORT


 M  +2 -0      digikam/albumiconview.cpp  
 M  +76 -9     utilities/lighttable/lighttablewindow.cpp  
 M  +2 -1      utilities/lighttable/lighttablewindow.h  


--- branches/extragear/kde3/graphics/digikam/digikam/albumiconview.cpp #707039:707040
@@ -1065,6 +1065,8 @@
     ltview->raise();
     ltview->setFocus();
     ltview->loadImageInfos(list, current);
+    if (list.count()>1)
+        ltview->setLeftRightItems(list);
 }
 
 // ------------------------------------------------------------------------------
--- branches/extragear/kde3/graphics/digikam/utilities/lighttable/lighttablewindow.cpp #707039:707040
@@ -497,13 +497,26 @@
                     false, true);
 }
 
+// Deal with items dropped onto the thumbbar (e.g. from the Album view)
 void LightTableWindow::slotThumbbarDroppedItems(const ImageInfoList& list)
 {
     loadImageInfos(list, 0);
+    if (list.count()>1)
+        setLeftRightItems(list);
 }
 
+
+// We get here either
+// - via F6 (from the albumview)
+//     a) digikamapp.cpp:  key_F6 leads to slotImageLightTable())
+//     b) digikamview.cpp: void DigikamView::slotImageLightTable()
+//          calls d->iconView->insertToLightTable(list, info);
+//     c) albumiconview.cpp: AlbumIconView::insertToLightTable
+//          calls ltview->loadImageInfos(list, current);
+// - via drag&drop, i.e. calls issued by the ...Dropped... routines
 void LightTableWindow::loadImageInfos(const ImageInfoList &list, ImageInfo *imageInfoCurrent)
 {
+
     ImageInfoList l = list;
 
     if (!imageInfoCurrent) 
@@ -535,11 +548,7 @@
         }
     }   
     d->barView->blockSignals(false);
-
-    LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(imageInfoCurrent));
-    if (ltItem) 
-        d->barView->setSelectedItem(ltItem);
-
+    
     // if window is iconified, show it
     if (isMinimized())
     {
@@ -722,30 +731,88 @@
     d->previewView->checkForSelection(info);
 }    
 
+// Deal with one (or more) items dropped onto the left panel
 void LightTableWindow::slotLeftDroppedItems(const ImageInfoList& list)
 {
     ImageInfo *info = *(list.begin());
     loadImageInfos(list, info);
 
     // We will check if first item from list is already stored in thumbbar
-    // Note than thumbbar store all ImageInfo reference in memory for preview object.
+    // Note that the thumbbar stores all ImageInfo reference 
+    // in memory for preview object.
     LightTableBarItem *item = d->barView->findItemByInfo(info);
-    if (item)
+    if (item) 
+    {
         slotSetItemOnLeftPanel(item->info());
+        // One approach is to make this item the current one, via
+        //    d->barView->setSelectedItem(item);
+        // However, this is not good, because this also sets 
+        // the right thumb to the same image.
+        // Therefore we use setLeftRightItems if there is more than 
+        // one item in the list of dropped images.        
+    }
+    if (list.count()>1)
+        setLeftRightItems(list);
+
 }
 
+// Deal with one (or more) items dropped onto the right panel
 void LightTableWindow::slotRightDroppedItems(const ImageInfoList& list)
 {
     ImageInfo *info = *(list.begin());
     loadImageInfos(list, info);
+    if (list.count()>1)
+        setLeftRightItems(list);
 
     // We will check if first item from list is already stored in thumbbar
-    // Note than thumbbar store all ImageInfo reference in memory for preview object.
+    // Note that the thumbbar stores all ImageInfo reference 
+    // in memory for preview object.
     LightTableBarItem *item = d->barView->findItemByInfo(info);
-    if (item)
+    if (item) 
+    {
         slotSetItemOnRightPanel(item->info());
+        // Make this item the current one.
+        d->barView->setSelectedItem(item);
+    }
+
+    if (list.count()>1)
+        setLeftRightItems(list);
+
 }
 
+
+// Set the images for the left and right panel.
+void LightTableWindow::setLeftRightItems(const ImageInfoList &list)
+{
+    ImageInfoList l = list;
+
+    // Make sure that more than just one item is in the list.
+    if (l.count()<=1)
+        return;
+
+    ImageInfo *info = l.first();
+    LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(info));
+
+    if (ltItem) {
+        // The first item is used for the left panel.
+        d->barView->setOnLeftPanel(info);
+        slotSetItemOnLeftPanel(info);
+
+        // The subsequent item is used for the right panel.
+        LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next());
+        if (next)
+        {
+            d->barView->setOnRightPanel(next->info());
+            slotSetItemOnRightPanel(next->info());
+            d->barView->setSelectedItem(next);
+            // ensure that the selected item is visible
+            // FIXME: this does not work:
+            d->barView->ensureItemVisible(next);
+        }
+    }
+}
+
+
 void LightTableWindow::slotSetItemLeft()
 {
     if (d->barView->currentItemImageInfo())
--- branches/extragear/kde3/graphics/digikam/utilities/lighttable/lighttablewindow.h #707039:707040
@@ -58,6 +58,7 @@
     static bool              lightTableWindowCreated();
 
     void loadImageInfos(const ImageInfoList &list, ImageInfo *imageInfoCurrent);
+    void setLeftRightItems(const ImageInfoList &list);
 
 signals: 
 
@@ -100,7 +101,7 @@
     void slotSetItemOnRightPanel(ImageInfo*);
     void slotLeftDroppedItems(const ImageInfoList&);
     void slotRightDroppedItems(const ImageInfoList&);
-
+    
     void slotLeftPanelLeftButtonClicked();
     void slotRightPanelLeftButtonClicked();
 

Arnd,

please, do not close this file until i have backported the code in KDE4. Use CCBUGS in this case, not BUG. Thanks in advance

Gilles

SVN commit 708316 by cgilles:

digiKam from trunk (KDE4) : backport commits #707040 from KDE3 branch
BUG: 146083


 M  +2 -0      digikam/albumiconview.cpp  
 M  +6 -0      project/project.kdevelop  
 M  +69 -8     utilities/lighttable/lighttablewindow.cpp  
 M  +1 -0      utilities/lighttable/lighttablewindow.h  


--- trunk/extragear/graphics/digikam/digikam/albumiconview.cpp #708315:708316
@@ -1057,6 +1057,8 @@
     ltview->raise();
     ltview->setFocus();
     ltview->loadImageInfos(list, current);
+    if (list.count()>1)
+        ltview->setLeftRightItems(list);
 }
 
 // ------------------------------------------------------------------------------
--- trunk/extragear/graphics/digikam/project/project.kdevelop #708315:708316
@@ -150,6 +150,12 @@
     <projectname>project</projectname>
     <projectname>project</projectname>
     <projectname>project</projectname>
+    <projectname>project</projectname>
+    <projectname>project</projectname>
+    <projectname>project</projectname>
+    <projectname>project</projectname>
+    <projectname>project</projectname>
+    <projectname>project</projectname>
   </general>
   <kdevfileview>
     <groups>
--- trunk/extragear/graphics/digikam/utilities/lighttable/lighttablewindow.cpp #708315:708316
@@ -497,11 +497,22 @@
     createGUI("lighttablewindowui.rc");
 }
 
+// Deal with items dropped onto the thumbbar (e.g. from the Album view)
 void LightTableWindow::slotThumbbarDroppedItems(const ImageInfoList& list)
 {
     loadImageInfos(list, ImageInfo());
+    if (list.count()>1)
+        setLeftRightItems(list);
 }
 
+// We get here either
+// - via F6 (from the albumview)
+//     a) digikamapp.cpp:  key_F6 leads to slotImageLightTable())
+//     b) digikamview.cpp: void DigikamView::slotImageLightTable()
+//          calls d->iconView->insertToLightTable(list, info);
+//     c) albumiconview.cpp: AlbumIconView::insertToLightTable
+//          calls ltview->loadImageInfos(list, current);
+// - via drag&drop, i.e. calls issued by the ...Dropped... routines
 void LightTableWindow::loadImageInfos(const ImageInfoList &list, const ImageInfo &givenImageInfoCurrent)
 {
     ImageInfoList l = list;
@@ -537,10 +548,6 @@
     }
     d->barView->blockSignals(false);
 
-    LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(imageInfoCurrent));
-    if (ltItem) 
-        d->barView->setSelectedItem(ltItem);
-
     // if window is iconified, show it
     if (isMinimized())
     {
@@ -723,30 +730,84 @@
     d->previewView->checkForSelection(info);
 }
 
+// Deal with one (or more) items dropped onto the left panel
 void LightTableWindow::slotLeftDroppedItems(const ImageInfoList& list)
 {
     ImageInfo info = list.first();
     loadImageInfos(list, info);
 
     // We will check if first item from list is already stored in thumbbar
-    // Note than thumbbar store all ImageInfo reference in memory for preview object.
+    // Note that the thumbbar stores all ImageInfo reference 
+    // in memory for preview object.
     LightTableBarItem *item = d->barView->findItemByInfo(info);
-    if (item)
+    if (item) 
+    {
         slotSetItemOnLeftPanel(item->info());
+        // One approach is to make this item the current one, via
+        //    d->barView->setSelectedItem(item);
+        // However, this is not good, because this also sets 
+        // the right thumb to the same image.
+        // Therefore we use setLeftRightItems if there is more than 
+        // one item in the list of dropped images. 
+    }
+    if (list.count()>1)
+        setLeftRightItems(list);
 }
 
+// Deal with one (or more) items dropped onto the right panel
 void LightTableWindow::slotRightDroppedItems(const ImageInfoList& list)
 {
     ImageInfo info = list.first();
     loadImageInfos(list, info);
+    if (list.count()>1)
+        setLeftRightItems(list);
 
     // We will check if first item from list is already stored in thumbbar
-    // Note than thumbbar store all ImageInfo reference in memory for preview object.
+    // Note that the thumbbar stores all ImageInfo reference 
+    // in memory for preview object.
     LightTableBarItem *item = d->barView->findItemByInfo(info);
-    if (item)
+    if (item) 
+    {
         slotSetItemOnRightPanel(item->info());
+        // Make this item the current one.
+        d->barView->setSelectedItem(item);
+    }
+    if (list.count()>1)
+        setLeftRightItems(list);
 }
 
+// Set the images for the left and right panel.
+void LightTableWindow::setLeftRightItems(const ImageInfoList &list)
+{
+    ImageInfoList l = list;
+
+    // Make sure that more than just one item is in the list.
+    if (l.count()<=1)
+        return;
+
+    ImageInfo info            = l.first();
+    LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(info));
+
+    if (ltItem) 
+    {
+        // The first item is used for the left panel.
+        d->barView->setOnLeftPanel(info);
+        slotSetItemOnLeftPanel(info);
+
+        // The subsequent item is used for the right panel.
+        LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next());
+        if (next)
+        {
+            d->barView->setOnRightPanel(next->info());
+            slotSetItemOnRightPanel(next->info());
+            d->barView->setSelectedItem(next);
+            // ensure that the selected item is visible
+            // FIXME: this does not work:
+            d->barView->ensureItemVisible(next);
+        }
+    }
+}
+
 void LightTableWindow::slotSetItemLeft()
 {
     if (!d->barView->currentItemImageInfo().isNull())
--- trunk/extragear/graphics/digikam/utilities/lighttable/lighttablewindow.h #708315:708316
@@ -58,6 +58,7 @@
     static bool              lightTableWindowCreated();
 
     void loadImageInfos(const ImageInfoList &list, const ImageInfo &imageInfoCurrent);
+    void setLeftRightItems(const ImageInfoList &list);
 
 signals: