Version: (using KDE Devel) Installed from: Compiled sources OS: Linux (Describing situation after fixing Bug 146063) When deleting from selection image with Ctrl-K (RMB click, menu, whatever) while image was in panel, panel is left blank. This isn't best solution because we can usually guess what user wanted to do next when removing picture from selection. Situation is really easy with Master image. When this one was removed it means Slave picture was much better and Master shoulnd't compare to other images in selection. Place Slave into Master position. Situation isn't such obvious when removing Slave but people usually are working from left to right (what about R-to-L interfaces?) so when removing Slave picture on the right side of former Slave should be placed into Slave position. There are some corner cases: a) Slave was last item in selection b) Slave was image just before Master - this case is especially ambiguous because in current implementation going through images doesn't wrap around selection; Slave on the left side of Master in thumbbar may indicate user was going rigth-to-left. So, without wrapping we cannot really guess what user wants when removing Slave :/ Best solution would be implement wrapping and then in most cases people would go around selection in continuous left-to-right. In such situation placing image on right side of former Slave into right panel would be satisfying for big majority of users. Thanks for your attention ;)
Note: this is the followup to http://bugs.kde.org/show_bug.cgi?id=146083 (and not 146063;-).
Created attachment 21534 [details] update items properly after delete Attached is a patch, which is an attempt at keeping the light-table populated properly on delete. Unfortunately, the code got quite complicated due to the corner cases.Not sure if there is much simplification possible. At least it has a lot of comments ;-). It will require extensive testing with all the different possibilities! Note that I cannot guarantee that it does not crash (though I did not experience any crashes). Also note that presently there are many DWarnings() for testing which will be removed in the final iteration of this patch.
Mik, Are you tested this patch on your computer ? Gilles
Tested and: When removing Master everything works as I would expect. But when removing Slave... - Image next to Slave becomes Master and image after that becomes Slave. So result is negative. Sorry, Arnd.
Arnd, Well check again your patch. I love like you improve LT. Thanks (:=))) From my viewpoint, this fix must be add to 0.9.3 release... Gilles
How I hate these corner cases ;-) ... before specifically dealing with them it did work - obviously I did not check the "normal" case again. Back to the drawing board - stay tuned!
Created attachment 21563 [details] remove items properly small change with (hopefully) a big difference. Please test thoroughly ... Thanks, Arnd
Big Thanks Arnd :) Now it works as I think most users would expect.
SVN commit 710518 by abaecker: Properly pdate left and right panel items of the lighttable after delete. CCBUGS: 147889 TODO:KDE4PORT M +198 -11 lighttablewindow.cpp --- branches/extragear/kde3/graphics/digikam/utilities/lighttable/lighttablewindow.cpp #710517:710518 @@ -345,12 +345,12 @@ actionCollection(), "lighttable_edititem"); d->editItemAction->setEnabled(false); - d->removeItemAction = new KAction(i18n("Remove item"), "fileclose", + d->removeItemAction = new KAction(i18n("Remove item from LightTable"), "fileclose", CTRL+Key_K, this, SLOT(slotRemoveItem()), actionCollection(), "lighttable_removeitem"); d->removeItemAction->setEnabled(false); - d->clearListAction = new KAction(i18n("Clear all items"), "editshred", + d->clearListAction = new KAction(i18n("Remove all items from LightTable"), "editshred", CTRL+SHIFT+Key_K, this, SLOT(slotClearItemsList()), actionCollection(), "lighttable_clearlist"); d->clearListAction->setEnabled(false); @@ -935,26 +935,213 @@ void LightTableWindow::slotRemoveItem(ImageInfo* info) { - if (d->previewView->leftImageInfo()) + // When either the image from the left or right panel is removed, + // there are various situations to account for. + // To describe them, 4 images A B C D are used + // and the subscript _L and _ R mark the currently + // active item on the left and right panel + + bool leftPanelActive = false; + ImageInfo *curr_linfo = d->previewView->leftImageInfo(); + ImageInfo *curr_rinfo = d->previewView->rightImageInfo(); + ImageInfo *new_linfo = 0; + ImageInfo *new_rinfo = 0; + + Q_LLONG infoId = info->id(); + + // First determine the next images to the current left and right image: + ImageInfo *next_linfo = 0; + ImageInfo *next_rinfo = 0; + + if (curr_linfo) { - if (d->previewView->leftImageInfo()->id() == info->id()) + LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(curr_linfo)); + if (ltItem) { - d->previewView->setLeftImageInfo(); - d->leftSidebar->slotNoCurrentItem(); + LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); + if (next) + { + next_linfo = next->info(); + } + } } - if (d->previewView->rightImageInfo()) + if (curr_rinfo) { - if (d->previewView->rightImageInfo()->id() == info->id()) + LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(curr_rinfo)); + if (ltItem) { - d->previewView->setRightImageInfo(); - d->rightSidebar->slotNoCurrentItem(); + LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); + if (next) + { + next_rinfo = next->info(); + } } } + d->barView->removeItem(info); - d->barView->setSelected(d->barView->currentItem()); + + + // Make sure that next_linfo and next_rinfo are still available: + if (!d->barView->findItemByInfo(next_linfo)) + { + next_linfo = 0; + } + if (!d->barView->findItemByInfo(next_rinfo)) + { + next_rinfo = 0; + } + + + + // removal of the left panel item? + if (curr_linfo) + { + if ( curr_linfo->id() == infoId ) + { + leftPanelActive = true; + // Delete the item A_L of the left panel: + // 1) A_L B_R C D -> B_L C_R D + // 2) A_L B C_R D -> B C_L D_R + // 3) A_L B C D_R -> B_R C D_L + // 4) A_L B_R -> A_L + // some more corner cases: + // 5) A B_L C_R D -> A C_L D_R + // 6) A B_L C_R -> A_R C_L + // 7) A_LR B C D -> B_L C_R D (does not yet work) + // I.e. in 3) we wrap around circularly. + + // When removing the left panel image, + // put the right panel image into the left panel. + // Check if this one is not the same (i.e. also removed). + if (curr_rinfo) + { + if (curr_rinfo->id() != infoId) + { + new_linfo = curr_rinfo; + // Set the right panel to the next image: + new_rinfo = next_rinfo; + } + } + } + } + + // removal of the right panel item? + if (curr_rinfo) + { + if (curr_rinfo->id() == infoId) + { + // Leave the left panel as the current one + new_linfo = curr_linfo; + // Set the right panel to the next image + new_rinfo = next_rinfo; + } + } + + + + // Now we deal with the corner cases, where no left or right item exists. + // If the right panel would be set, but not the left-one, then swap + if (!new_linfo && new_rinfo) + { + new_linfo = new_rinfo; + new_rinfo = 0; + leftPanelActive = true; + } + + + if (!new_linfo) + { + if (d->barView->countItems()>0) + { + LightTableBarItem* first = dynamic_cast<LightTableBarItem*>(d->barView->firstItem()); + new_linfo = first->info(); + } + } + + // no right item defined? + if (!new_rinfo) + { + // If there are at least two items, we can find reasonable right image. + if (d->barView->countItems()>1) + { + // See if there is an item next to the left one: + LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(new_linfo)); + LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); + if (next) + { + new_rinfo = next->info(); + } + else + { + // If there is no item to the right of new_linfo + // then we can choose the first item for new_rinfo + // (as we made sure that there are at least two items) + LightTableBarItem* first = dynamic_cast<LightTableBarItem*>(d->barView->firstItem()); + new_rinfo = first->info(); + } + } + } + + // Check if left and right are set to the same + if (new_linfo && new_rinfo) + { + if (new_linfo->id() == new_rinfo->id()) + { + // Only keep the left one + new_rinfo = false; + } + } + + // If the right panel would be set, but not the left-one, then swap + // (note that this has to be done here again!) + if (!new_linfo && new_rinfo) + { + new_linfo = new_rinfo; + new_rinfo = false; + leftPanelActive = true; + } + + + // set the image for the left panel + if (new_linfo) + { + d->barView->setOnLeftPanel(new_linfo); + slotSetItemOnLeftPanel(new_linfo); + + // make this the selected item if the left was active before + if ( leftPanelActive) + { + LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(new_linfo)); + d->barView->setSelectedItem(ltItem); + } + } + else + { + d->previewView->setLeftImageInfo(); + d->leftSidebar->slotNoCurrentItem(); + } + + // set the image for the right panel + if (new_rinfo) + { + d->barView->setOnRightPanel(new_rinfo); + slotSetItemOnRightPanel(new_rinfo); + // make this the selected item if the left was active before + if (!leftPanelActive) + { + LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(new_rinfo)); + d->barView->setSelectedItem(ltItem); + } + } + else + { + d->previewView->setRightImageInfo(); + d->rightSidebar->slotNoCurrentItem(); + } + refreshStatusBar(); }
SVN commit 710596 by cgilles: polish commits #710518: - never use true/false with a pointer value. - no need to cast again a point which is already casted to the right form. - fix indent a coding style. CCBUGS: 147889 M +25 -34 lighttablewindow.cpp --- branches/extragear/kde3/graphics/digikam/utilities/lighttable/lighttablewindow.cpp #710595:710596 @@ -940,7 +940,7 @@ // To describe them, 4 images A B C D are used // and the subscript _L and _ R mark the currently // active item on the left and right panel - + bool leftPanelActive = false; ImageInfo *curr_linfo = d->previewView->leftImageInfo(); ImageInfo *curr_rinfo = d->previewView->rightImageInfo(); @@ -955,35 +955,32 @@ if (curr_linfo) { - LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(curr_linfo)); + LightTableBarItem *ltItem = d->barView->findItemByInfo(curr_linfo); if (ltItem) { LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); if (next) { - next_linfo = next->info(); + next_linfo = next->info(); } - } } if (curr_rinfo) { - LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(curr_rinfo)); + LightTableBarItem *ltItem = d->barView->findItemByInfo(curr_rinfo); if (ltItem) { LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); if (next) { - next_rinfo = next->info(); + next_rinfo = next->info(); } } } - d->barView->removeItem(info); - // Make sure that next_linfo and next_rinfo are still available: if (!d->barView->findItemByInfo(next_linfo)) { @@ -994,13 +991,11 @@ next_rinfo = 0; } - - // removal of the left panel item? - if (curr_linfo) + if (curr_linfo) { - if ( curr_linfo->id() == infoId ) - { + if ( curr_linfo->id() == infoId ) + { leftPanelActive = true; // Delete the item A_L of the left panel: // 1) A_L B_R C D -> B_L C_R D @@ -1026,7 +1021,7 @@ } } } - } + } // removal of the right panel item? if (curr_rinfo) @@ -1040,21 +1035,18 @@ } } - - // Now we deal with the corner cases, where no left or right item exists. // If the right panel would be set, but not the left-one, then swap if (!new_linfo && new_rinfo) { - new_linfo = new_rinfo; - new_rinfo = 0; + new_linfo = new_rinfo; + new_rinfo = 0; leftPanelActive = true; } - - if (!new_linfo) + if (!new_linfo) { - if (d->barView->countItems()>0) + if (d->barView->countItems() > 0) { LightTableBarItem* first = dynamic_cast<LightTableBarItem*>(d->barView->firstItem()); new_linfo = first->info(); @@ -1065,15 +1057,15 @@ if (!new_rinfo) { // If there are at least two items, we can find reasonable right image. - if (d->barView->countItems()>1) + if (d->barView->countItems() > 1) { // See if there is an item next to the left one: - LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(new_linfo)); - LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); + LightTableBarItem *ltItem = d->barView->findItemByInfo(new_linfo); + LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); if (next) { new_rinfo = next->info(); - } + } else { // If there is no item to the right of new_linfo @@ -1082,7 +1074,7 @@ LightTableBarItem* first = dynamic_cast<LightTableBarItem*>(d->barView->firstItem()); new_rinfo = first->info(); } - } + } } // Check if left and right are set to the same @@ -1091,7 +1083,7 @@ if (new_linfo->id() == new_rinfo->id()) { // Only keep the left one - new_rinfo = false; + new_rinfo = 0; } } @@ -1099,11 +1091,10 @@ // (note that this has to be done here again!) if (!new_linfo && new_rinfo) { - new_linfo = new_rinfo; - new_rinfo = false; + new_linfo = new_rinfo; + new_rinfo = 0; leftPanelActive = true; } - // set the image for the left panel if (new_linfo) @@ -1114,9 +1105,9 @@ // make this the selected item if the left was active before if ( leftPanelActive) { - LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(new_linfo)); + LightTableBarItem *ltItem = d->barView->findItemByInfo(new_linfo); d->barView->setSelectedItem(ltItem); - } + } } else { @@ -1132,9 +1123,9 @@ // make this the selected item if the left was active before if (!leftPanelActive) { - LightTableBarItem *ltItem = dynamic_cast<LightTableBarItem*>(d->barView->findItemByInfo(new_rinfo)); + LightTableBarItem *ltItem = d->barView->findItemByInfo(new_rinfo); d->barView->setSelectedItem(ltItem); - } + } } else {
SVN commit 710598 by cgilles: digiKam from trunk (KDE4): backport commits #710518 BUG: 147889 M +203 -3 lighttablewindow.cpp --- trunk/extragear/graphics/digikam/utilities/lighttable/lighttablewindow.cpp #710597:710598 @@ -370,13 +370,13 @@ connect(d->editItemAction, SIGNAL(triggered()), this, SLOT(slotEditItem())); actionCollection()->addAction("lighttable_edititem", d->editItemAction); - d->removeItemAction = new KAction(KIcon("dialog-close"), i18n("Remove item"), this); + d->removeItemAction = new KAction(KIcon("dialog-close"), i18n("Remove item from LightTable"), this); d->removeItemAction->setShortcut(Qt::CTRL+Qt::Key_K); d->removeItemAction->setEnabled(false); connect(d->removeItemAction, SIGNAL(triggered()), this, SLOT(slotRemoveItem())); actionCollection()->addAction("lighttable_removeitem", d->removeItemAction); - d->clearListAction = new KAction(KIcon("list-remove"), i18n("Clear all items"), this); + d->clearListAction = new KAction(KIcon("list-remove"), i18n("Remove all items from LightTable"), this); d->clearListAction->setShortcut(Qt::CTRL+Qt::SHIFT+Qt::Key_K); d->clearListAction->setEnabled(false); connect(d->clearListAction, SIGNAL(triggered()), this, SLOT(slotClearItemsList())); @@ -936,7 +936,7 @@ void LightTableWindow::slotRemoveItem(const ImageInfo &info) { - if (!d->previewView->leftImageInfo().isNull()) +/* if (!d->previewView->leftImageInfo().isNull()) { if (d->previewView->leftImageInfo() == info) { @@ -956,6 +956,206 @@ d->barView->removeItem(info); d->barView->setSelected(d->barView->currentItem()); +*/ + + // When either the image from the left or right panel is removed, + // there are various situations to account for. + // To describe them, 4 images A B C D are used + // and the subscript _L and _ R mark the currently + // active item on the left and right panel + + bool leftPanelActive = false; + ImageInfo curr_linfo = d->previewView->leftImageInfo(); + ImageInfo curr_rinfo = d->previewView->rightImageInfo(); + ImageInfo new_linfo; + ImageInfo new_rinfo; + + Q_LLONG infoId = info.id(); + + // First determine the next images to the current left and right image: + ImageInfo next_linfo; + ImageInfo next_rinfo; + + if (!curr_linfo.isNull()) + { + LightTableBarItem *ltItem = d->barView->findItemByInfo(curr_linfo); + if (ltItem) + { + LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); + if (next) + { + next_linfo = next->info(); + } + } + } + + if (!curr_rinfo.isNull()) + { + LightTableBarItem *ltItem = d->barView->findItemByInfo(curr_rinfo); + if (ltItem) + { + LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); + if (next) + { + next_rinfo = next->info(); + } + } + } + + d->barView->removeItem(info); + + // Make sure that next_linfo and next_rinfo are still available: + if (!d->barView->findItemByInfo(next_linfo)) + { + next_linfo = ImageInfo(); + } + if (!d->barView->findItemByInfo(next_rinfo)) + { + next_rinfo = ImageInfo(); + } + + // removal of the left panel item? + if (!curr_linfo.isNull()) + { + if ( curr_linfo.id() == infoId ) + { + leftPanelActive = true; + // Delete the item A_L of the left panel: + // 1) A_L B_R C D -> B_L C_R D + // 2) A_L B C_R D -> B C_L D_R + // 3) A_L B C D_R -> B_R C D_L + // 4) A_L B_R -> A_L + // some more corner cases: + // 5) A B_L C_R D -> A C_L D_R + // 6) A B_L C_R -> A_R C_L + // 7) A_LR B C D -> B_L C_R D (does not yet work) + // I.e. in 3) we wrap around circularly. + + // When removing the left panel image, + // put the right panel image into the left panel. + // Check if this one is not the same (i.e. also removed). + if (!curr_rinfo.isNull()) + { + if (curr_rinfo.id() != infoId) + { + new_linfo = curr_rinfo; + // Set the right panel to the next image: + new_rinfo = next_rinfo; + } + } + } + } + + // removal of the right panel item? + if (!curr_rinfo.isNull()) + { + if (curr_rinfo.id() == infoId) + { + // Leave the left panel as the current one + new_linfo = curr_linfo; + // Set the right panel to the next image + new_rinfo = next_rinfo; + } + } + + // Now we deal with the corner cases, where no left or right item exists. + // If the right panel would be set, but not the left-one, then swap + if (new_linfo.isNull() && !new_rinfo.isNull()) + { + new_linfo = new_rinfo; + new_rinfo = ImageInfo(); + leftPanelActive = true; + } + + if (new_linfo.isNull()) + { + if (d->barView->countItems() > 0) + { + LightTableBarItem* first = dynamic_cast<LightTableBarItem*>(d->barView->firstItem()); + new_linfo = first->info(); + } + } + + // no right item defined? + if (new_rinfo.isNull()) + { + // If there are at least two items, we can find reasonable right image. + if (d->barView->countItems() > 1) + { + // See if there is an item next to the left one: + LightTableBarItem *ltItem = d->barView->findItemByInfo(new_linfo); + LightTableBarItem* next = dynamic_cast<LightTableBarItem*>(ltItem->next()); + if (next) + { + new_rinfo = next->info(); + } + else + { + // If there is no item to the right of new_linfo + // then we can choose the first item for new_rinfo + // (as we made sure that there are at least two items) + LightTableBarItem* first = dynamic_cast<LightTableBarItem*>(d->barView->firstItem()); + new_rinfo = first->info(); + } + } + } + + // Check if left and right are set to the same + if (!new_linfo.isNull() && !new_rinfo.isNull()) + { + if (new_linfo.id() == new_rinfo.id()) + { + // Only keep the left one + new_rinfo = ImageInfo(); + } + } + + // If the right panel would be set, but not the left-one, then swap + // (note that this has to be done here again!) + if (new_linfo.isNull() && !new_rinfo.isNull()) + { + new_linfo = new_rinfo; + new_rinfo = ImageInfo(); + leftPanelActive = true; + } + + // set the image for the left panel + if (!new_linfo.isNull()) + { + d->barView->setOnLeftPanel(new_linfo); + slotSetItemOnLeftPanel(new_linfo); + + // make this the selected item if the left was active before + if ( leftPanelActive) + { + LightTableBarItem *ltItem = d->barView->findItemByInfo(new_linfo); + d->barView->setSelectedItem(ltItem); + } + } + else + { + d->previewView->setLeftImageInfo(); + d->leftSidebar->slotNoCurrentItem(); + } + + // set the image for the right panel + if (!new_rinfo.isNull()) + { + d->barView->setOnRightPanel(new_rinfo); + slotSetItemOnRightPanel(new_rinfo); + // make this the selected item if the left was active before + if (!leftPanelActive) + { + LightTableBarItem *ltItem = d->barView->findItemByInfo(new_rinfo); + d->barView->setSelectedItem(ltItem); + } + } + else + { + d->previewView->setRightImageInfo(); + d->rightSidebar->slotNoCurrentItem(); + } + refreshStatusBar(); }