<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/114846/">https://git.reviewboard.kde.org/r/114846/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 5th, 2014, 6:49 p.m. UTC, <b>Alexander Schuch</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;">I have not read the code - I just played this version.
I do not really understand how the space bar is supposed to work. When I am totally zoomed out, I press the space bar and it zooms in. If I press it again, nothing happens. If I zoom in or zoom out then, I can use the space bar to un-close-up. So am I supposed to always use space bar to toggle between two zoom states?</pre>
</blockquote>
<p>On January 5th, 2014, 10:06 p.m. UTC, <b>Ian Wadham</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;">Yes, it is best to always use the space bar. That should toggle between two zoom states, homing in on a spot the mouse is pointing at. I suspect the app had somehow lost keyboard focus before you pressed space bar to zoom out, then regained focus when you did your own zoom.
BTW, how are you doing zoom? With the mouse wheel or by using the zoom-widget in the status bar?</pre>
</blockquote>
<p>On January 12th, 2014, 6:41 p.m. UTC, <b>Alexander Schuch</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;">I try to give instructions on how to reproduce my problem.
1. Load a puzzle and totally zoom out. I use the mouse wheel. (Same when using the zoom buttons at the bottom right.)
2. I hit space and the view zooms.
3. Pressing space a second time does nothing - it does not zoom out again. Focusing/unfocusing the application does not change this. The application has the focus, as shortcuts like Alt+G to open the Game menu work.
While playing around with this zooming, I still feel it does not work right/intuitive.
I have a zoom level. I hit space and it zooms in. I hit space and it zooms out. I hit space again, and it zooms in again. I now use the mouse wheel to zoom in further (zoom level A). I hit space. It zooms out. I hit space, it zooms in again at the same zoom level I zoomed in to (zoom level A). I hit space again, it zooms out. Space again, zoom in again at zoom level A. I zoom out a little bit (zoom level B). I hit space. It zooms out. I hit space again, and get zoom level B. This is all great so far.
Now I hit space to zoom out. I now adjust this zoom by zooming in one level or zooming out one level. I hit space. But I am not at zoom level B but at a newly calculated zoom level.
So what do you think about this feature request: Two independent zoom levels are maintained and space simply toggled between them. This way the user can zoom in or out at one level without affecting the other.</pre>
</blockquote>
<p>On January 13th, 2014, 3:20 a.m. UTC, <b>Ian Wadham</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;">Re your steps 1, 2, 3, I can reproduce your problem now. If I totally zoom out to the limit Palapeli allows, using the wheel, and then go on scrolling, I find that the view momentarily zooms in and back out again. After that, the spacebar does not work. I think this is a bug in Palapeli and will look into it.
Thanks for playing around with the zooming. It's just the kind of feedback I am looking for, because I am "prototyping" design ideas rather than submitting finished code.
I am not happy with the idea of a "close-up" that toggles with an even closer view (cases of zoom A and B above). In particular, it looks ridiculous with an 8-piece puzzle I sometimes use for testing. The pieces actually get smaller when you zoom to "close-up".
The "close-up" is currently a calculated default. I was already thinking of having a Setting to adjust the close-up. It might be easier (and less programming) to adjust it using regular scrolling, as you suggest. Likewise, I think it is a good idea to have a set zoom-out level (a "long-shot" view), which can also have a calculated default (e.g. the one Palapeli has always used when it starts a puzzle), but can be adjusted to suit the user and the puzzle being solved.
My only concern is that, if you go to close-up level and then want to have an even closer look at one piece (Is it the piece of sky with a telephone wire across it?), that you might lose your usual close-up level and always be going to the "ultra-close-up" level. Do we need a "set it" function for the zoom levels provided by the spacebar action?
The main idea is for zooming in or out to be quick and predictable when doing a large puzzle. When you are zoomed to "close-up", it is handy to have a predictable viewport extent (in number of pieces seen) and to be able to step systematically through the puzzle table one viewport at a time (with a chosen close-up size) when searching for pieces.</pre>
</blockquote>
<p>On January 26th, 2014, 2:57 a.m. UTC, <b>Ian Wadham</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;">I have implemented your feature request, Alexander. There are now two saved zoom levels (close-up and distant) and spacebar toggles between them. Initially they are set for easy viewing of pieces and for overview of whole puzzle-table. If the number of pieces is small these levels may be identical (ie. if you can see all the pieces clearly on the whole puzzle table).
All previous Palapeli zoom methods work the same, whether or not you use the spacebar. If you adjust the zoom level manually and then use the spacebar to zoom in, your zoom level is saved as the new distant view, provided it is less than or equal to the close-up level. Conversely, if you use the spacebar to zoom out, any manual adjustment you have done is saved as the new close-up level, provided it is greater than or equal to the distant level. The invariant is that close-up level >= distant level always.
The code changes have been committed to master.</pre>
</blockquote>
<p>On January 26th, 2014, 4:36 p.m. UTC, <b>Albert Astals Cid</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;">"The code changes have been committed to master."
Is this the code of this review request or some other part?</pre>
</blockquote>
<p>On January 26th, 2014, 9:38 p.m. UTC, <b>Ian Wadham</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;">The original code of this review request is now more than 15 commits old (on origin master). For the code changes that implement Alexander's request, relevant to this review, see revision b0ffd3c7 of 26 Jan at https://projects.kde.org/projects/kde/kdegames/palapeli/repository.</pre>
</blockquote>
<p>On January 26th, 2014, 9:57 p.m. UTC, <b>Albert Astals Cid</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;">If this code is old and has been commited to master why is this still open?</pre>
</blockquote>
<p>On January 26th, 2014, 10:13 p.m. UTC, <b>Ian Wadham</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;">Huh?
Because I just committed a change yesterday ... in response to Alexander's comment and suggestion of 12 Jan. Isn't that what a review process is?
So if someone wants to try out the new feature of toggling between two zoom levels and offer a comment on it, please do.</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;">Ok, maybe i didn't read all the thread with enough attention :-) Sorry if i came out a bit rude. keep hacking and reviewing :-)</pre>
<br />
<p>- Albert</p>
<br />
<p>On January 4th, 2014, 11:14 a.m. UTC, Ian Wadham wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Games.</div>
<div>By Ian Wadham.</div>
<p style="color: grey;"><i>Updated Jan. 4, 2014, 11:14 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
palapeli
</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;">To add an instant close-up view and an alternative selection highlighting scheme.
The code has been committed to origin/master. The diff supplied here is for ease of reading only. It seems OK, but is not guaranteed to be authoritative.
The close-up view is operated by hovering the mouse pointer where you want to see a close-up and then toggling the space bar. The size of pieces, in pixels, is a fixed fraction (1/12th at present) of the number of pixels on the shorter side of the screen. Thus on an Apple MacBook 1440x900 screen, the close-up piece-size is 75 pixels. On a 1024x768 screen, it would be 64 pixels. In each case, the piece should be about the same apparent angular size. This should also work reasonably on 'phones, tablets and TV monitors, where the actual size of pixels (in mm) depends on the expected viewing distance.
The close-up action will eventually appear on the View menu and toolbar, for the sake of completeness and visibility, but is not expected to work as effectively as when you use the mouse and a shortcut key together.
The new highlighter is simply an elliptical pixmap containing a circular gradient in a bright color and it appears behind the selected jigsaw piece if the shadowing option is disabled, as if the piece is lit up from behind. The size of the highlight is (at present) about 1.5 times the size of the largest piece in the puzzle, at whatever zoom-level you are using. This gives good visibility of single-piece selections when there are 5,000 pieces in view, which shadow highlighting does not. The default color is bright green, which is good at low scale factors but a bit overpowering at large scale factors, especially when a large number of pieces becomes joined and highlighted. There is scope to choose better defaults, add settings and automatically "tune" settings to the view. This is just a prototype for people to look at.
Please feel free to comment, either on the UI aspects or the technicalities of the code. I am already finding it easier to handle large puzzles.</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;">Used the new close-up and highlight facilities in a 500-piece puzzle and a 5000-piece puzzle, including joining pieces in the 500-piece puzzle and checking the display of highlights on joined pieces - immediately after joining and also after saving and re-loading the puzzle. On the 5,000 piece puzzle, close-up worked well from a full view of all 5,000 pieces and highlights of selected pieces were easily visible on the 5,000 piece full view. Shadow-based highlights were too small to see at that scale.
Also did more detailed testing on an 8-piece puzzle, verifying that the highlights had the correct shape around various combinations of joined pieces.
Noted some desirable upgrades, as described in the TODO comments in the diff, such as use of Settings to control colors and sizes.</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>src/engine/mergegroup.cpp <span style="color: grey">(098744d)</span></li>
<li>src/engine/piece.h <span style="color: grey">(2c4d8fb)</span></li>
<li>src/engine/piece.cpp <span style="color: grey">(1fc2b94)</span></li>
<li>src/engine/scene.h <span style="color: grey">(2667404)</span></li>
<li>src/engine/scene.cpp <span style="color: grey">(cfa51d6)</span></li>
<li>src/engine/view.h <span style="color: grey">(70df58e)</span></li>
<li>src/engine/view.cpp <span style="color: grey">(d93ff3d)</span></li>
<li>src/window/mainwindow.cpp <span style="color: grey">(7e6cb95)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/114846/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>