<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119549/">https://git.reviewboard.kde.org/r/119549/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On août 4th, 2014, 10:44 matin CEST, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hi, looks great, thanks for the patch!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I only have minor requests for the configuration dialog:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- I suggest renaming "Shared zoom and position" to "Keep zoom and position" (and rename the enum value to Keep) and "Individual (per image) zoom and position" to "Per image zoom and position".<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- Move the "Zoom mode" block above the "Enlarge smaller images" checkbox to keep all zooming options together</p></pre>
</blockquote>
<p>On août 5th, 2014, 2:20 matin CEST, <b>John Zaitseff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Moving the zoom mode block is probably a good idea. Did you want me to redo the patch, or will you? I can post the "git request-pull" here if you like.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Renaming "Individual (per image) zoom and position" is also a good idea.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not so sure about renaming "Shared zoom and position": the point is that the zoom and position are <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">shared</em> between different images. I do think it can be worded better, however, so I'm open to suggestions. What about "Keep same zoom and position" (with enum renamed to KeepSame)?</p>
</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I can also provide a patch for the documentation (handbook, etc.) to explain all this to the user; I just didn't want to do that before this patch was accepted :-)</p></pre>
</blockquote>
<p>On août 5th, 2014, 9:33 matin CEST, <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Keep same zoom and position" and a KeepSame enum sounds good to me. A documentation update sounds good as well :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Would be great if you could update the merge request with these changes as it makes it easy for me to merge them in without having to use git author command line options to attribute the changes to you.</p></pre>
</blockquote>
<p>On août 6th, 2014, 1:06 matin CEST, <b>John Zaitseff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've done all the above, except for the documentation update: that I'll do as a separate patch, as I may well modify more than just my little section. I also plan to add tooltips, etc, to the dialog boxes, as part of the documentation patch.</p></pre>
</blockquote>
<p>On août 6th, 2014, 1:20 matin CEST, <b>John Zaitseff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The following changes since commit fde8adf02b8c7ce6b5f2975af3a628daabbe567d:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">SVN_SILENT made messages (.desktop file) (2014-07-06 04:29:41 +0000)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">are available in the git repository at:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">git://git.zap.org.au/data/git/gwenview zoom-modes</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">for you to fetch changes up to a32c9c127eebe2e90c4af6a9c84a662321d1f80e:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Move the "Zoom mode" block above "Enlarge smaller images" checkbox (2014-08-06 08:56:18 +1000)</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">John Zaitseff (13):<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Add radio buttons to set the default zoom mode. Also renumber all controls on the page to be systematic.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Rename Single zoom mode to Shared zoom mode<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Add the ZoomMode config file option<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Remove the superfluous Lock Zoom button in the zoom widget<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Completely remove traces of ZoomLocked<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Use saved zoom and position only for individual zoom mode<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Fix issue where "0% zoom" is shown (bug #335675)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Add missing ">" to e-mail address<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Remove trailing whitespace<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Replace tabs with spaces in source code<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Rename label "Individual (per-image)" to "Per image zoom and position"<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Rename Shared zoom mode to KeepSame<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Move the "Zoom mode" block above "Enlarge smaller images" checkbox</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">app/configdialog.cpp | 7 ++<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
app/imageviewconfigpage.ui | 226 ++++++++++++++++++++++++++++++--------<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
app/kipiinterface.h | 1 -<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
app/viewmainpage.cpp | 19 ++--<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
cmake/FindLCMS2.cmake | 3 +-<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
devdoc/CONTRIBUTING.md | 1 -<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
doc/index.docbook | 38 +++----<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
lib/archiveutils.h | 1 -<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
lib/cms/iccjpeg.c | 2 +-<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
lib/documentview/documentview.cpp | 6 +-<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
lib/gwenviewconfig.kcfg | 28 +++--<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
lib/zoommode.h | 49 +++++++++<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
lib/zoomwidget.cpp | 29 -----<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
lib/zoomwidget.h | 6 -<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
part/gvpart.rc | 1 -<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
15 files changed, 289 insertions(+), 128 deletions(-)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
create mode 100644 lib/zoommode.h</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks a lot! I just merged it in master (it's too late for 4.14 unfortunately).</p></pre>
<br />
<p>- Aurélien</p>
<br />
<p>On août 7th, 2014, 6:27 après-midi CEST, John Zaitseff wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Gwenview.</div>
<div>By John Zaitseff.</div>
<p style="color: grey;"><i>Updated août 7, 2014, 6:27 après-midi</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=337262">337262</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
gwenview
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(Taken from bug #337262 )</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Over the years, there has been much discussion about whether the zoom and position should be kept the same between images (see, for example, bug 293103, which I submitted, or bugs 291759, 294915, 321122, 327889, 331412, 334530, 337037 --- there may be more!). I have even submitted a patch (with bug 293103) which was applied in modified form---thanks! However, it has been subsequently broken...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have come to realise that there are THREE main ways people like to have the zoom and position settings applied to successive images:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Each image should be opened in Zoom to Fit mode, even if the previous image was zoomed in or out and was panned to a different position. This is Aurélien Gâteau's preferred mode of operation: call it Autofit zoom mode.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The zoom and position settings should be shared across all images. New images should be opened with the previous image's settings. If you go back to a previous image (from image B to image A, previously opened), image A's settings should be updated to be those of image B. This allows you to set zoom and position on an image, then go back and forward to the previous and next image while retaining the settings. This is MY preferred mode of operation: call it Shared mode.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Each image should remember its own zoom and position settings. New images should be opened with the previous image's position and zoom, but if you then change image B's zoom/position, this is NOT passed back to image A, if you go back to that image. This is Abhinav Gangwar's preferred mode of operation, and is what LockZoom implements in Gwenview: call it Individual mode.</p>
</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think people don't mind Autofit being the default, as long as it can be changed. At the moment, doing so is very non-obvious, and the Shared mode of operation simply does not exist (as of 12th February 2014).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am submitting a patch that does the following:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Create a config file option "ZoomMode" with ZoomMode::Autofit, ZoomMode::Shared and ZoomMode::Individual being the choices (Autofit is the default, per Aurélien's preferences).</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Remove the now-obsolete ShowLockZoomButton and LockZoom config options.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Remove the Lock Zoom button: it is not visible in Autofit mode anyway, and when a user wants the other modes, they want it on ALL the time, not just some of the time! Besides, it clutters up the interface :-)</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Set the zoom and position settings for each image depending on the ZoomMode selected.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Add a group of three Zoom Mode radio buttons to the Image Settings configuration dialog page: "Autofit each image", "Shared zoom and position" and "Individual (per-image) zoom and position". I think adding these radio buttons is the most elegant way for users to change this setting, given how non-obvious the current method is (as can be seen by the continual stream of bug reports!).</p>
</li>
</ol></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Have compiled Gwenview GIT head with this patch on my system; tested successfully with all variants.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>app/configdialog.cpp <span style="color: grey">(a582695)</span></li>
<li>app/imageviewconfigpage.ui <span style="color: grey">(f5e1e5a)</span></li>
<li>app/kipiinterface.h <span style="color: grey">(45fb03b)</span></li>
<li>app/viewmainpage.cpp <span style="color: grey">(7caf099)</span></li>
<li>cmake/FindLCMS2.cmake <span style="color: grey">(cc17e9f)</span></li>
<li>devdoc/CONTRIBUTING.md <span style="color: grey">(ffdb7f7)</span></li>
<li>doc/index.docbook <span style="color: grey">(d3266b8)</span></li>
<li>lib/archiveutils.h <span style="color: grey">(db79771)</span></li>
<li>lib/cms/iccjpeg.c <span style="color: grey">(1f6c4b1)</span></li>
<li>lib/documentview/documentview.cpp <span style="color: grey">(1ab8bbb)</span></li>
<li>lib/gwenviewconfig.kcfg <span style="color: grey">(92d39f7)</span></li>
<li>lib/zoommode.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>lib/zoomwidget.h <span style="color: grey">(b181f5a)</span></li>
<li>lib/zoomwidget.cpp <span style="color: grey">(da66a56)</span></li>
<li>part/gvpart.rc <span style="color: grey">(35703a7)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119549/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>