Bug 229795

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. ***