Version: (using KDE 4.4.1) Installed from: Ubuntu Packages Problem description: If you are switching from one album forth and back again, the last position in the album is not preserved. This makes it very difficult to review a large number of pictures and compare them with a different album. After coming back you need to start searching your pictures from top down until found the previous picture. With more than 100 pictures this is a painful task. The same problem happens if you are starting a similarity search. When your search is completed you cannot just go back to the original picture. Here again you need to walk top down until found. Expected behavior: When I press the "Go Back" button I expect to jump back exactly to the location in my album where I started from. This should happen regardless if I where in a different album or in the search tab.
I confirm, and i agree with the wish Gilles Caulier
Please see bug 208506: How can these two be reconciled? We must decide for either behavior for the case that the currently selected item is also contained in the album switched to.
Well, why do you think that both requirements are contradictionary? In this request you are moving back. In the other your switching to a new view.
When image a and b are contained in two views, let's say they are in the same album and have a common tag. Select a in the album, switch to the tag. Select b in tag view. Switch back to the album. Shall a be selected (this wish), or b (208506)?
Add a user option?
Think a bit more about this I would also say these two reports are contradictory. This report is valid for switching from one album to the other of the same type, where as the other bug report is about switching between different album types.
Created attachment 42550 [details] patch for bug229795
This patch implements the conservation position (physical and tags) when switching between albums. In my opinion what has been written in the first record.
Marcel, Johaness, What do you think about this patch ? It's not too intrusive. Gilles Caulier
Do you think should be done as an option, which can be turned off in settings?
Konstantin, I vote for no option in setup. Sound like this behavior is natural for me. In fact, i waiting this feature for a long time. Note : There is another bugzilla entry, relevant of icon view, where users want to customize items order like they want. But this is another story, if you is interested... I think that database need to be adapted to store album filtering and ordering properties. https://bugs.kde.org/show_bug.cgi?id=91562 Gilles Caulier
What do you think about this patch? It stands as a change, improve, leave as is, or he does not need?
Just compiled and tested : - Scrolling position is perfectly saved and restored. - Selection is not restored. Only the first image of selection is restored, in case of more than one item is selected. Gilles Caulier
All right. I'll do whatever remained selection.
You can use DigikamView's signalImageSelected() to get information about all currently selected images. There is no good way currently to apply this information though - remember that images are loaded in chunks. For me it's fine as it is. Some minor remarks: Cleanup the map entry in deleteAlbum? In one or other occasion you can use const ImageInfo& to pass by value. Replace tabs by 4 spaces.
Created attachment 42661 [details] Save all the selected images Added to the patch: - Save all the selected images - Delete information about an album from the history map when deleting album - Just a couple of minor fixes
Tested. The patch sound working fine. Some remark about coding style : always use m_ prefix for class members (sometime you use it, sometime no). A lots of my time in digiKam project, i review code to polish implementation. The goal is to have same coding style everywhere to have an homogenous implementation. It's important. about pointer declaration, use "MyClass* ptr" is better than "MyClass *ptr" and align all of then, line by line. When it's possible, use d private class container to speed up compilation, and clean up include file in header implementation. Marcel, check out again before patch inclusion in svn Thanks Gilles
Comment on attachment 42661 [details] Save all the selected images diff -u ./dk.original/graphics/digikam/digikam/albumhistory.cpp ./dk.patch/graphics/digikam/digikam/albumhistory.cpp --- ./dk.original/graphics/digikam/digikam/albumhistory.cpp 2010-04-07 01:53:31.744391200 +0400 +++ ./dk.patch/graphics/digikam/digikam/albumhistory.cpp 2010-04-11 13:31:53.936729064 +0400 @@ -32,6 +32,8 @@ // Local includes #include "album.h" +#include "imageinfo.h" +#include "albummanager.h" namespace Digikam { @@ -65,11 +67,36 @@ QWidget* widget; }; +class HistoryPosition +{ +public: + + HistoryPosition() + { + + }; + + HistoryPosition(ImageInfo c, KUrl::List s) + { + current = c; + select = s; + }; + + bool operator==(const HistoryPosition& item) + { + return (current == item.current) && (select == item.select); + } + + ImageInfo current; + KUrl::List select; +}; + AlbumHistory::AlbumHistory() { m_backwardStack = new AlbumStack; m_forwardStack = new AlbumStack; m_moving = false; + m_blockSelection = false; } AlbumHistory::~AlbumHistory() @@ -344,4 +371,75 @@ return (m_backwardStack->count() <= 1) ? true : false; } +void AlbumHistory::slotAlbumSelected() +{ + Album* currentAlbum = AlbumManager::instance()->currentAlbum(); + if (m_historyPos.contains(currentAlbum)) + { + if (currentAlbum->type() == Album::PHYSICAL || currentAlbum->type() == Album::TAG) + { + m_blockSelection = true; + emit signalSetCurrent(m_historyPos[currentAlbum].current.id()); + } + } +} + +void AlbumHistory::slotAlbumCurrentChanged() +{ + Album* currentAlbum = AlbumManager::instance()->currentAlbum(); + if (m_historyPos.contains(currentAlbum)) + { + if (currentAlbum->type() == Album::PHYSICAL || currentAlbum->type() == Album::TAG) + { + if (m_historyPos[currentAlbum].select.size()) + { + emit signalSetSelectedUrls(m_historyPos[currentAlbum].select); + } + } + } + m_blockSelection = false; +} + +void AlbumHistory::slotCurrentChange(const ImageInfo& info) +{ + Album* currentAlbum = AlbumManager::instance()->currentAlbum(); + m_historyPos[currentAlbum].current = info; +} + +void AlbumHistory::slotImageSelected(const ImageInfoList& selectedImage) +{ + if (m_blockSelection) + return; + Album *currentAlbum = AlbumManager::instance()->currentAlbum(); + if (m_historyPos.contains(currentAlbum)) + m_historyPos[currentAlbum].select.clear(); + for (int i = 0; i < selectedImage.count(); i++) + { + if (!m_historyPos[currentAlbum].select.contains(selectedImage[i].fileUrl())) + { + m_historyPos[currentAlbum].select.insert(0, selectedImage[i].fileUrl()); + } + } +} + +void AlbumHistory::slotClearSelectPAlbum(const ImageInfo& imageInfo) +{ + Album* album = dynamic_cast<Album*>(AlbumManager::instance()->findPAlbum(imageInfo.albumId())); + if (m_historyPos.contains(album)) + m_historyPos[album].select.clear(); +} + +void AlbumHistory::slotClearSelectTAlbum(int id) +{ + Album* album = dynamic_cast<Album*>(AlbumManager::instance()->findTAlbum(id)); + if (m_historyPos.contains(album)) + m_historyPos[album].select.clear(); +} + +void AlbumHistory::slotAlbumDeleted(Album* album) +{ + if (m_historyPos.contains(album)) + m_historyPos.remove(album); +} + } // namespace Digikam diff -u ./dk.original/graphics/digikam/digikam/albumhistory.h ./dk.patch/graphics/digikam/digikam/albumhistory.h --- ./dk.original/graphics/digikam/digikam/albumhistory.h 2010-04-07 01:53:31.796392789 +0400 +++ ./dk.patch/graphics/digikam/digikam/albumhistory.h 2010-04-11 13:25:49.872729576 +0400 @@ -30,14 +30,21 @@ // Qt includes #include <QList> +#include <QMap> #include <QObject> #include <QStringList> +//KDE includes +#include <KUrl> + namespace Digikam { class Album; class HistoryItem; +class HistoryPosition; +class ImageInfo; +class ImageInfoList; /** * Manages the history of the last visited albums. @@ -67,6 +74,21 @@ bool isForwardEmpty() const; bool isBackwardEmpty() const; +Q_SIGNALS: + + void signalSetCurrent(qlonglong imageId); + void signalSetSelectedUrls(const KUrl::List&); + +public Q_SLOTS: + + void slotAlbumCurrentChanged(); + void slotAlbumDeleted(Album* album); + void slotAlbumSelected(); + void slotClearSelectPAlbum(const ImageInfo& imageInfo); + void slotClearSelectTAlbum(int id); + void slotCurrentChange(const ImageInfo& info); + void slotImageSelected(const ImageInfoList& selectedImage); + private: HistoryItem* getCurrentAlbum() const; @@ -76,10 +98,12 @@ typedef QList<HistoryItem*> AlbumStack; - bool m_moving; + bool m_moving; + bool m_blockSelection; AlbumStack *m_backwardStack; AlbumStack *m_forwardStack; + QMap<Album*, HistoryPosition> m_historyPos; }; } // namespace Digikam diff -u ./dk.original/graphics/digikam/digikam/digikamview.cpp ./dk.patch/graphics/digikam/digikam/digikamview.cpp --- ./dk.original/graphics/digikam/digikam/digikamview.cpp 2010-04-07 01:53:31.580397973 +0400 +++ ./dk.patch/graphics/digikam/digikam/digikamview.cpp 2010-04-11 00:26:00.100730406 +0400 @@ -495,6 +495,34 @@ connect(AlbumSettings::instance(), SIGNAL(setupChanged()), this, SLOT(slotSidebarTabTitleStyleChanged())); + + // -- Album History ----------------- + connect(this, SIGNAL(signalAlbumSelected(bool)), + d->albumHistory, SLOT(slotAlbumSelected())); + + connect(this, SIGNAL(signalImageSelected(const ImageInfoList&, bool, bool, const ImageInfoList&)), + d->albumHistory, SLOT(slotImageSelected(const ImageInfoList&))); + + connect(d->iconView, SIGNAL(currentChanged(const ImageInfo&)), + d->albumHistory, SLOT(slotCurrentChange(const ImageInfo&))); + + connect(d->iconView, SIGNAL(gotoAlbumAndImageRequested(const ImageInfo&)), + d->albumHistory, SLOT(slotClearSelectPAlbum(const ImageInfo&))); + + connect(d->iconView, SIGNAL(gotoTagAndImageRequested(int)), + d->albumHistory, SLOT(slotClearSelectTAlbum(int))); + + connect(d->iconView->imageModel(), SIGNAL(imageInfosAdded(QList<ImageInfo>)), + d->albumHistory, SLOT(slotAlbumCurrentChanged())); + + connect(d->albumHistory, SIGNAL(signalSetCurrent(qlonglong)), + d->iconView, SLOT(setCurrentWhenAvailable(qlonglong))); + + connect(d->albumHistory, SIGNAL(signalSetSelectedUrls(KUrl::List)), + d->iconView, SLOT(setSelectedUrls(KUrl::List))); + + connect(d->albumManager, SIGNAL(signalAlbumDeleted(Album*)), + d->albumHistory, SLOT(slotAlbumDeleted(Album*))); } void DigikamView::connectIconViewFilter(AlbumIconViewFilter *filter) Общие подкаталоги: ./dk.original/graphics/digikam/digikam/.svn и ./dk.patch/graphics/digikam/digikam/.svn
Created attachment 42680 [details] corrected Gilles Caulier, I corrected.
What do you think about this?
Are there any reasons to not include this patch? I tried this one and selection and current image is restored well on album switching (normals albums and tag albums). Lovely feature konstantin, thanks! I also looked at the code and do not see any faults so far. Only one sugestion: It would be nice to have the same on timeline albums. Regards, Jens
Jens, well, i thinking this patch is already in svn... sorry... I forget it. Please feel free to apply it to trunk. I'm right with you too. virtual album as timeline must be supported too. Gilles Caulier
SVN commit 1119195 by jmueller: Apply patch from konstantin krivakin. Thanks for contrubution! Timeline albums needed to be supported in History, too. Konstantin, could you extend your patch this way? CCBUGS: 229795 Regards, Jens M +98 -0 albumhistory.cpp M +24 -0 albumhistory.h M +28 -0 digikamview.cpp WebSVN link: http://websvn.kde.org/?view=rev&revision=1119195
I will try to find time to implement it.
konstantin, Do you take a look about timeline support in album history ? We would close this file for next 1.4.0 release... Gilles Caulier
Git commit e476dd1f8f6306b97b3480659325af6f11df79a4 by Marcel Wiesweg. Committed on 01/07/2011 at 17:57. Pushed by mwiesweg into branch 'master'. Remove IMO unnecessary restriction to tags and physical albums for storing scroll position. Works nicely with searches. BUG: 229795 M +4 -10 digikam/album/albumhistory.cpp http://commits.kde.org/digikam/e476dd1f8f6306b97b3480659325af6f11df79a4
*** Bug 208506 has been marked as a duplicate of this bug. ***