Bug 257933

Created attachment 53745 [details]
screenshot: changed zoom level with identical slider #2

Created attachment 53746 [details]
screenshot: changed zoom level with identical slider #1

Created attachment 53747 [details]
screenshot: changed zoom level with identical slider #3

Expected behaviour, as seen before previewing image.

Vivek,

This entry still valid with 2.x serie ?

Gilles Caulier

It's still somewhat present in 3.x.

If you set the zoom slider to n in Thumbnail [zoom_slider_bug_01.png], then switch to Preview Image and back to Thumbnail, the zoom slider may show values of n+2 [zoom_slider_bug_02.png].

This happens because when switching back to Thumbnail, the slider gets reset with value of size of thumbnails, which will return size+2 if thumbnails have a highlight border [`ThumbnailLoadThread::ThumbnailLoadThreadPriv::pixmapSizeForThumbnailSize(...)`].

Note that this bug not only increases zoom slider's value to +2, but also resizes thumbnails to +2.

The following patch fixes that.
It checks is thumbnail has a border, and if it does, it returns sizeWithBorders-2, which then used in calling `DigikamApp::slotThumbSizeChanged(int size)`.

Created attachment 79631 [details]
zoom_slider_bug_01.png

Created attachment 79632 [details]
zoom_slider_bug_02.png

Created attachment 79636 [details]
patch_for_bug_257933.diff

My suggestions would be: name the reverse of "thumbnailPixmapSize()" "thumbnailImageSize" or rename both to a pair of words which better transport the meaning.
Why not patch "ThumbnailSize ImageCategorizedView::thumbnailSize() const" to return the actual size without border? It is ImageCategorizedView::setThumbnailSize(const ThumbnailSize& s) which adds the 2 pixels, so the same class should subtract them.

(In reply to comment #9)
> My suggestions would be: name the reverse of "thumbnailPixmapSize()"
> "thumbnailImageSize" or rename both to a pair of words which better
> transport the meaning.

I agree that names are confusing. What would you say about renaming `ThumbnailLoadThread::thumbnailPixmapSize` to `ThumbnailLoadThread::thumbnailToPixmapSize` and `ThumbnailLoadThread::pixmapThumbnailSize` to `ThumbnailLoadThread::pixmapToThumbnailSize`, because it's basically what they are doing, converting pixmap size into thumbnail size and back.

> Why not patch "ThumbnailSize ImageCategorizedView::thumbnailSize() const" to
> return the actual size without border? It is
> ImageCategorizedView::setThumbnailSize(const ThumbnailSize& s) which adds
> the 2 pixels, so the same class should subtract them.

Yes, patching `ImageCategorizedView::thumbnailSize` looks like a better solution, because logically it should return the value which was initially set, not the value+2. But still, we can't just return size-2 from it, -2 should be done only if thumbnail has highlight boarders.

Created attachment 79680 [details]
patch_for_bug_257933_v2.diff

Here is the new patch. Please disregard patch_for_bug_257933.diff.
I didn't rename methods because we still haven't decided on names.

I agree with your renaming suggestion, it's symmetric and speaking after all.
Patch is welcome, or commit if you've got KDE commit rights.

Created attachment 79720 [details]
patch_for_bug_257933_v3.diff

The same as previous + renamed methods.
(I don't have KDE commit rights.)

Marcel,

What's new about this entry ? Patch is acceptable for you ?

Nurupo,

Please update your patch against current digiKam code from git/master. It cannot be applied :

[gilles@localhost core]$ patch -p1 < patch_for_bug_257933_v3.diff 
patching file digikam/items/imagecategorizedview.cpp
Hunk #1 succeeded at 431 (offset -1 lines).
Hunk #2 succeeded at 446 (offset -1 lines).
patching file digikam/items/imagethumbnaildelegate.cpp
patching file libs/threadimageio/thumbnailloadthread.cpp
Hunk #1 succeeded at 277 (offset 3 lines).
Hunk #2 FAILED at 289.
1 out of 2 hunks FAILED -- saving rejects to file libs/threadimageio/thumbnailloadthread.cpp.rej
patching file libs/threadimageio/thumbnailloadthread.h

Gilles Caulier

Yes it's fine see comment 12 ;-)

Nurupo,

Accordingly with comment #15 from Marcel, i waiting your patch updated to include it with 4.0.0 release

Thanks in advance

Gilles Caulier

Surprised the patch wasn't applied 6 month ago. It will take me some time to update it.

Nurupo,

I always waiting your patch updated... 

Thanks in advance

Gilles Caulier

Created attachment 85403 [details]
remerged patch patch_for_bug_257933_v4.diff

remerged patch just in case nobody else does it

I tried to understand and reproduce the original problem here without to apply the patch, but i cannot using 4.0.0-beta3...

Switching between Icon-View and Preview mode restore properly zoom settings for these modes.

Can you explain better how to reproduce the dysfunction please ?

Gilles Caulier

Git commit bbcee345a825861e7a7e8d2e4a6cacdea92ce77c by Gilles Caulier.
Committed on 04/03/2014 at 21:08.
Pushed by cgilles into branch 'master'.

Apply patch #85403 from nurupo which fix bug increasing zoom slider/thumbs size value to +2 when user swith between preview and icon-view mode.
FIXED-IN: 4.0.0

M  +2    -2    digikam/items/imagecategorizedview.cpp
M  +1    -1    digikam/items/imagethumbnaildelegate.cpp
M  +7    -2    libs/threadimageio/thumbnailloadthread.cpp
M  +7    -2    libs/threadimageio/thumbnailloadthread.h

http://commits.kde.org/digikam/bbcee345a825861e7a7e8d2e4a6cacdea92ce77c