Bug 145309 - make "show this image" load all images in current view
Summary: make "show this image" load all images in current view
Status: RESOLVED FIXED
Alias: None
Product: kphotoalbum
Classification: Applications
Component: Viewer (show other bugs)
Version: unspecified
Platform: Gentoo Packages Linux
: NOR wishlist
Target Milestone: ---
Assignee: KPhotoAlbum Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-11 12:40 UTC by Jan Kundrát
Modified: 2007-06-16 14:07 UTC (History)
0 users

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


Attachments
allow-viewing-all-items-without-ctrl-a.patch (2.83 KB, patch)
2007-05-11 12:40 UTC, Jan Kundrát
Details
allow-viewing-all-items-without-ctrl-a.patch (2.25 KB, patch)
2007-05-12 20:20 UTC, Jan Kundrát
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Kundrát 2007-05-11 12:40:13 UTC
Version:           svn (using KDE KDE 3.5.5)
Installed from:    Gentoo Packages
Compiler:          gcc version 3.4.6 (Gentoo Hardened 3.4.6-r2, ssp-3.4.6-1.0, pie-8.7.10) 
OS:                Linux

When you doubleclick on a thumbnail, the Viewer opens, but you can't move to other images in your former view. The attached patch makes it possible.
Comment 1 Jan Kundrát 2007-05-11 12:40:54 UTC
Created attachment 20538 [details]
allow-viewing-all-items-without-ctrl-a.patch
Comment 2 Baptiste MATHUS 2007-05-12 18:51:30 UTC
Perfect addon! For a long time, I've thought it was annoying but I ended up getting used to it and almost forgot.

I suppose you made only the double-click provide the new behaviour on purpose. I'm wondering if we couldn't just decide that if there's exactly one photo selected always allow to do what your patch does.

In fact, for the example use case: sometimes I select a photo, then press escape and only use the keyboard arrows to move the selection and hit enter (so only one photo is currently selected).

I can't any reason why when just one photo is to be displayed, then the behaviour your patch allow would be activated. I guess we should ask the list (though this post should be sent to the list, thanks to Shawn, if I'm not wrong).
Comment 3 Jan Kundrát 2007-05-12 20:20:09 UTC
Created attachment 20554 [details]
allow-viewing-all-items-without-ctrl-a.patch

New patch that detects if current selection contains only one file and make all
images form current context available. Could be quite slow if current context
has plenty of images.
Comment 4 Jan Kundrát 2007-05-19 00:51:52 UTC
SVN commit 666153 by jkt:

BUG: 145309

Make "show this image" load all images in current view, thus making pressing Ctrl+A optional.

Thanks to Shawn Willden for nice comments and Baptiste Mathus for review.


 M  +18 -0     MainWindow/Window.cpp  
 M  +9 -0      Viewer/ViewerWidget.cpp  
 M  +1 -0      Viewer/ViewerWidget.h  


--- trunk/extragear/graphics/kphotoalbum/MainWindow/Window.cpp #666152:666153
@@ -458,10 +458,25 @@
 
 void MainWindow::Window::launchViewer( QStringList files, bool reuse, bool slideShow, bool random )
 {
+    int seek = -1;
     if ( files.count() == 0 ) {
         QMessageBox::warning( MainWindow::Window::theMainWindow(), i18n("No Images or Videos to Display"),
                               i18n("None of the selected images or videos were available on disk.") );
         return;
+    } else if ( files.count() == 1 ) {
+        // we fake it so it appears the user has selected all images
+        // and magically scrolls to the originally selected one
+        const QString fileName = ((const QStringList&)files).first();
+        files = DB::ImageDB::instance()->currentScope(  true );
+        int index = 0;
+
+        for( QStringList::const_iterator it = files.constBegin(); it != files.constEnd(); ++it, ++index ) {
+            if ( *it == fileName ) {
+                seek = index;
+                break;
+            }
+        }
+
     }
 
     if (random)
@@ -478,6 +493,9 @@
 
     viewer->show( slideShow );
     viewer->load( files );
+    if (seek != -1) {
+        viewer->seekTo( seek );
+    }
     viewer->raise();
 }
 
--- trunk/extragear/graphics/kphotoalbum/Viewer/ViewerWidget.cpp #666152:666153
@@ -443,6 +443,15 @@
     e->accept();
 }
 
+void Viewer::ViewerWidget::seekTo(int position)
+{
+    if ( ( position < (int) _list.count() ) && ( position >= 0 ) ) {
+        save();
+	_current = position;
+	load();
+    }
+}
+
 void Viewer::ViewerWidget::showNextN(int n)
 {
     save();
--- trunk/extragear/graphics/kphotoalbum/Viewer/ViewerWidget.h #666152:666153
@@ -53,6 +53,7 @@
     bool showingFullScreen() const;
     void setShowFullScreen( bool on );
     void show( bool slideShow );
+    void seekTo( int position );
     KActionCollection* actions();
 
 public slots:
Comment 5 Risto H. Kurppa 2007-06-05 11:48:17 UTC
In a view with 500 images can take up to 3-4 seconds to show the first image - too slow.. (core2duo 2.4GHz, 2GB)
Comment 6 Baptiste MATHUS 2007-06-05 15:15:56 UTC
Are you sure you don't have a problem accessing your photos?

I remember I tested this feature with all my db (6500+ images/videos) and it was taking about one second or two maximum... (Pentium M 1.7Ghz, 1GB ram)...

I'll try and re-test this evening.
Comment 7 Jan Kundrát 2007-06-05 20:28:11 UTC
Could you please provide "tracefile" generated by this:

strace -e trace=file -r -o tracefile ./kphotoalbum
Comment 8 Jan Kundrát 2007-06-05 21:04:43 UTC
Got requested data, but I can't see any delay in it.
Comment 9 Risto H. Kurppa 2007-06-06 13:34:15 UTC
Noticed a bug with this:

Search for example all images w. token K (or I'd suppose any category will do the same). Then remove the searched token from an image (so you have all images with a token K and that one where you just removed the token K). 

Now if you try to open the viewer for the image without K, it'll show the first image of the set, not the selected image.

And after opening the browser, it will not show the item anymore even it's visible there in the set.


So I suggest that either the thumbnail view should be automatically refreshed to hide the images that don't any more match the original query or image viewer should be smarter and include all images that are shown in the thumbnail view, not only those matching the previous query.
Comment 10 Jan Kundrát 2007-06-07 04:08:49 UTC
I'll send a patch to the list shortly with fix for this.
Comment 11 Jan Kundrát 2007-06-16 14:07:26 UTC
SVN commit 676248 by jkt:

- changed MainWindow::Window::selectedOnDisk() to return list of images
  available in current view if selection is empty
- reworked Viewer not to check for file availability before it's really needed

BUG: 145309


 M  +4 -0      ChangeLog  
 M  +11 -16    MainWindow/Window.cpp  


--- branches/extragear/kde3/graphics/kphotoalbum/ChangeLog #676247:676248
@@ -5,6 +5,10 @@
 
 	* Stop video playback before seeking to another image
 
+	* Changed MainWindow::Window::selectedOnDisk() to return list of
+	images available in current view if selection is empty and reworked
+	Viewer not to check for file availability before it's really needed
+
 2007-06-11  Tuomas Suutari  <thsuut@utu.fi>
 
 	* Do not show -1 x -1 image sizes at all. And as suggested by
--- branches/extragear/kde3/graphics/kphotoalbum/MainWindow/Window.cpp #676247:676248
@@ -436,14 +436,18 @@
     slotView( false, false );
 }
 
+/*
+ * Returns a list of files that are both selected and on disk. If there are no
+ * selected files, returns all files form current context that are on disk.
+ * */
 QStringList MainWindow::Window::selectedOnDisk()
 {
-    QStringList listOnDisk;
     QStringList list = selected();
     if ( list.count() == 0 )
-        list = DB::ImageDB::instance()->currentScope(  true );
+        return DB::ImageDB::instance()->currentScope( true );
 
-    for( QStringList::Iterator it = list.begin(); it != list.end(); ++it ) {
+    QStringList listOnDisk;
+    for( QStringList::const_iterator it = list.constBegin(); it != list.constEnd(); ++it ) {
         if ( DB::ImageInfo::imageOnDisk( *it ) )
             listOnDisk.append( *it );
     }
@@ -453,7 +457,7 @@
 
 void MainWindow::Window::slotView( bool reuse, bool slideShow, bool random )
 {
-    launchViewer( selectedOnDisk(), reuse, slideShow, random );
+    launchViewer( selected(), reuse, slideShow, random );
 }
 
 void MainWindow::Window::launchViewer( QStringList files, bool reuse, bool slideShow, bool random )
@@ -467,7 +471,7 @@
         // we fake it so it appears the user has selected all images
         // and magically scrolls to the originally selected one
         const QString fileName = ((const QStringList&)files).first();
-        files = DB::ImageDB::instance()->currentScope(  true );
+        files = _thumbnailView->imageList( ThumbnailView::ThumbnailWidget::ViewOrder );
         seek = files.findIndex(fileName);
     }
 
@@ -672,13 +676,9 @@
 
 void MainWindow::Window::slotExportToHTML()
 {
-    QStringList list = selectedOnDisk();
-    if ( list.count() == 0 )
-        list = DB::ImageDB::instance()->currentScope( true );
-
     if ( ! _htmlDialog )
         _htmlDialog = new HTMLGenerator::HTMLDialog( this, "htmlExportDialog" );
-    _htmlDialog->exec( list );
+    _htmlDialog->exec( selectedOnDisk() );
 }
 
 void MainWindow::Window::startAutoSaveTimer()
@@ -1123,12 +1123,7 @@
 
 void MainWindow::Window::slotExport()
 {
-    QStringList list = selectedOnDisk();
-    if ( list.count() == 0 ) {
-        KMessageBox::sorry( this, i18n("Nothing to export.") );
-    }
-    else
-        ImportExport::Export::imageExport( list );
+    ImportExport::Export::imageExport( selectedOnDisk() );
 }
 
 void MainWindow::Window::slotReenableMessages()