[prev in list] [next in list] [prev in thread] [next in thread]
List: gwenview-devel
Subject: Re: Review Request 119549: Allow different ways to handle zoom and position between images
From: Aurélien Gâteau <agateau () kde ! org>
Date: 2014-08-07 16:28:30
Message-ID: 20140807162830.20974.80578 () probe ! kde ! org
[Download RAW message or body]
--===============5118399826738306668==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On août 4, 2014, 10:44 matin, Aurélien Gâteau wrote:
> > Hi, looks great, thanks for the patch!
> >
> > I only have minor requests for the configuration dialog:
> > - 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".
> > - Move the "Zoom mode" block above the "Enlarge smaller images" checkbox to keep \
> > all zooming options together
>
> John Zaitseff wrote:
> * 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.
> * Renaming "Individual (per image) zoom and position" is also a good idea.
>
> * I'm not so sure about renaming "Shared zoom and position": the point is that the \
> zoom and position are *shared* 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)?
> 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 :-)
> Aurélien Gâteau wrote:
> "Keep same zoom and position" and a KeepSame enum sounds good to me. A \
> documentation update sounds good as well :)
> 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.
> John Zaitseff wrote:
> 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.
> John Zaitseff wrote:
> The following changes since commit fde8adf02b8c7ce6b5f2975af3a628daabbe567d:
>
> SVN_SILENT made messages (.desktop file) (2014-07-06 04:29:41 +0000)
>
> are available in the git repository at:
>
> git://git.zap.org.au/data/git/gwenview zoom-modes
>
> for you to fetch changes up to a32c9c127eebe2e90c4af6a9c84a662321d1f80e:
>
> Move the "Zoom mode" block above "Enlarge smaller images" checkbox (2014-08-06 \
> 08:56:18 +1000)
> ----------------------------------------------------------------
> John Zaitseff (13):
> Add radio buttons to set the default zoom mode. Also renumber all controls on the \
> page to be systematic. Rename Single zoom mode to Shared zoom mode
> Add the ZoomMode config file option
> Remove the superfluous Lock Zoom button in the zoom widget
> Completely remove traces of ZoomLocked
> Use saved zoom and position only for individual zoom mode
> Fix issue where "0% zoom" is shown (bug #335675)
> Add missing ">" to e-mail address
> Remove trailing whitespace
> Replace tabs with spaces in source code
> Rename label "Individual (per-image)" to "Per image zoom and position"
> Rename Shared zoom mode to KeepSame
> Move the "Zoom mode" block above "Enlarge smaller images" checkbox
>
> app/configdialog.cpp | 7 ++
> app/imageviewconfigpage.ui | 226 ++++++++++++++++++++++++++++++--------
> app/kipiinterface.h | 1 -
> app/viewmainpage.cpp | 19 ++--
> cmake/FindLCMS2.cmake | 3 +-
> devdoc/CONTRIBUTING.md | 1 -
> doc/index.docbook | 38 +++----
> lib/archiveutils.h | 1 -
> lib/cms/iccjpeg.c | 2 +-
> lib/documentview/documentview.cpp | 6 +-
> lib/gwenviewconfig.kcfg | 28 +++--
> lib/zoommode.h | 49 +++++++++
> lib/zoomwidget.cpp | 29 -----
> lib/zoomwidget.h | 6 -
> part/gvpart.rc | 1 -
> 15 files changed, 289 insertions(+), 128 deletions(-)
> create mode 100644 lib/zoommode.h
Thanks a lot! I just merged it in master (it's too late for 4.14 unfortunately).
- Aurélien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119549/#review63738
-----------------------------------------------------------
On août 7, 2014, 6:27 après-midi, John Zaitseff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119549/
> -----------------------------------------------------------
>
> (Updated août 7, 2014, 6:27 après-midi)
>
>
> Review request for Gwenview.
>
>
> Bugs: 337262
> http://bugs.kde.org/show_bug.cgi?id=337262
>
>
> Repository: gwenview
>
>
> Description
> -------
>
> (Taken from bug #337262 )
>
> 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...
> I have come to realise that there are THREE main ways people like to have the zoom \
> and position settings applied to successive images:
> 1. 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.
> 2. 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.
> 3. 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.
> 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).
> I am submitting a patch that does the following:
>
> 1. 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).
> 2. Remove the now-obsolete ShowLockZoomButton and LockZoom config options.
>
> 3. 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 :-)
> 4. Set the zoom and position settings for each image depending on the ZoomMode \
> selected.
> 5. 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!).
>
> Diffs
> -----
>
> app/configdialog.cpp a582695
> app/imageviewconfigpage.ui f5e1e5a
> app/kipiinterface.h 45fb03b
> app/viewmainpage.cpp 7caf099
> cmake/FindLCMS2.cmake cc17e9f
> devdoc/CONTRIBUTING.md ffdb7f7
> doc/index.docbook d3266b8
> lib/archiveutils.h db79771
> lib/cms/iccjpeg.c 1f6c4b1
> lib/documentview/documentview.cpp 1ab8bbb
> lib/gwenviewconfig.kcfg 92d39f7
> lib/zoommode.h PRE-CREATION
> lib/zoomwidget.h b181f5a
> lib/zoomwidget.cpp da66a56
> part/gvpart.rc 35703a7
>
> Diff: https://git.reviewboard.kde.org/r/119549/diff/
>
>
> Testing
> -------
>
> Have compiled Gwenview GIT head with this patch on my system; tested successfully \
> with all variants.
>
> Thanks,
>
> John Zaitseff
>
>
--===============5118399826738306668==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<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>
--===============5118399826738306668==--
_______________________________________________
Gwenview-devel mailing list
Gwenview-devel@kde.org
https://mail.kde.org/mailman/listinfo/gwenview-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic