Bug 229795 - Position in album lost when switching album forth and back [patch]
Summary: Position in album lost when switching album forth and back [patch]
Status: RESOLVED FIXED
Alias: None
Product: digikam
Classification: Applications
Component: Albums-MainView (show other bugs)
Version: 1.3.0
Platform: Ubuntu Unspecified
: NOR normal
Target Milestone: ---
Assignee: Digikam Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-07 11:59 UTC by Jörn Schimmelpfeng
Modified: 2012-06-27 10:29 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.0.0
Sentry Crash Report:


Attachments
patch for bug229795 (4.48 KB, patch)
2010-04-07 00:14 UTC, konstantin
Details
Save all the selected images (6.99 KB, patch)
2010-04-10 22:44 UTC, konstantin
Details
corrected (6.97 KB, patch)
2010-04-11 11:40 UTC, konstantin
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jörn Schimmelpfeng 2010-03-07 11:59:27 UTC
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.
Comment 1 caulier.gilles 2010-03-07 18:21:02 UTC
I confirm, and i agree with the wish

Gilles Caulier
Comment 2 Marcel Wiesweg 2010-03-17 19:08:37 UTC
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.
Comment 3 Jörn Schimmelpfeng 2010-03-17 21:25:51 UTC
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.
Comment 4 Marcel Wiesweg 2010-03-17 21:38:35 UTC
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)?
Comment 5 Johannes Wienke 2010-03-17 21:41:01 UTC
Add a user option?
Comment 6 Johannes Wienke 2010-03-17 21:45:06 UTC
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.
Comment 7 konstantin 2010-04-07 00:14:09 UTC
Created attachment 42550 [details]
patch for bug229795
Comment 8 konstantin 2010-04-07 00:14:49 UTC
This patch implements the conservation position (physical and tags) when switching between albums. In my opinion what has been written in the first record.
Comment 9 caulier.gilles 2010-04-07 08:38:50 UTC
Marcel, Johaness,

What do you think about this patch ? It's not too intrusive.

Gilles Caulier
Comment 10 konstantin 2010-04-07 16:40:53 UTC
Do you think should be done as an option, which can be turned off in settings?
Comment 11 caulier.gilles 2010-04-07 17:00:47 UTC
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
Comment 12 konstantin 2010-04-07 17:45:30 UTC
What do you think about this patch? It stands as a change, improve, leave as is, or he does not need?
Comment 13 caulier.gilles 2010-04-07 18:28:58 UTC
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
Comment 14 konstantin 2010-04-07 18:43:30 UTC
All right. I'll do whatever remained selection.
Comment 15 Marcel Wiesweg 2010-04-07 19:37:10 UTC
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.
Comment 16 konstantin 2010-04-10 22:44:08 UTC
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
Comment 17 caulier.gilles 2010-04-11 11:09:44 UTC
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 18 konstantin 2010-04-11 11:35:52 UTC
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
Comment 19 konstantin 2010-04-11 11:40:54 UTC
Created attachment 42680 [details]
corrected

Gilles Caulier, I corrected.
Comment 20 konstantin 2010-04-12 17:55:32 UTC
What do you think about this?
Comment 21 Jens Mueller 2010-04-26 20:28:42 UTC
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
Comment 22 caulier.gilles 2010-04-26 20:50:13 UTC
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
Comment 23 Jens Mueller 2010-04-26 21:18:04 UTC
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
Comment 24 konstantin 2010-04-27 10:13:59 UTC
I will try to find time to implement it.
Comment 25 caulier.gilles 2010-07-29 10:02:23 UTC
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
Comment 26 Marcel Wiesweg 2011-07-11 17:03:02 UTC
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
Comment 27 Marcel Wiesweg 2011-07-11 17:24:58 UTC
*** Bug 208506 has been marked as a duplicate of this bug. ***