Version: (using KDE Devel) Installed from: Compiled sources Strange(?) decisions in Light table. 1. Open LT with 4 images. 2. First image will be inserted in right panel. This is IMO first bug. First image should be inserted in left panel (as "master" image), second image in second panel. 3. Drag 2nd image into left panel. It will become BOTH "Master" image AND "Slave" - will be inserted automatically in right panel. I understand what happens (you are setting Master and Slave to the same image) but it is not intuitive - took me about 5 minutes to grok what is going on... Especially with default putting of image only in Slave panel. When user will put 2nd image into Master panel also automatically it will change contents of Slave panel.
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: