[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-05 7:33:17
Message-ID: 20140805073317.20974.42078 () probe ! kde ! org
[Download RAW message or body]
--===============5468905819637087114==
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 :-)
"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.
- Aurélien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119549/#review63738
-----------------------------------------------------------
On juil. 31, 2014, 4:40 matin, John Zaitseff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119549/
> -----------------------------------------------------------
>
> (Updated juil. 31, 2014, 4:40 matin)
>
>
> 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/viewmainpage.cpp 7caf099
> lib/documentview/documentview.cpp 1ab8bbb
> lib/gwenviewconfig.kcfg 92d39f7
> lib/zoommode.h PRE-CREATION
> lib/zoomwidget.h b181f5a
> lib/zoomwidget.cpp da66a56
>
> 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
>
>
--===============5468905819637087114==
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>
</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;">"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> <br />
<p>- Aurélien</p>
<br />
<p>On juillet 31st, 2014, 4:40 matin 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 juil. 31, 2014, 4:40 matin</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/viewmainpage.cpp <span style="color: grey">(7caf099)</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>
</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>
--===============5468905819637087114==--
_______________________________________________
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