<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/126685/">https://git.reviewboard.kde.org/r/126685/</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 11th, 2016, 11:37 a.m. UTC, <b>Aleix Pol Gonzalez</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;">Wouldn't it be ok just implementing it on a per-view basis using ctrl+mouse wheel?</p></pre>
</blockquote>
<p>On January 11th, 2016, 11:44 a.m. UTC, <b>Kevin Funk</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;">Note: We have two submissions for fixing this bug.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There's also: https://phabricator.kde.org/D774 (with a much less intrusive solution, which I like better)</p></pre>
</blockquote>
<p>On January 11th, 2016, 5:52 p.m. UTC, <b>Igor Kushnir</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 added zoom-in and zoom-out buttons because of Kevin's suggestion: https://bugs.kde.org/show_bug.cgi?id=285162#c11
These buttons won't be really useful for everyone's workflow since the mouse wheel is enough. They could improve discover-ability of the feature though.
If the zoom feature would have been implemented on per-view basis, I'm not sure how zoom factor could be synchronized between different views. If it won't be synchronized at all, then this zoom factor should also be stored in KConfig for each view separately. But how to determine a different KConfig entry name in each view? Pass a string as a parameter instead of the DocumentationZoom object? Even then, having to adjust all views' zoom factors separately would be inconvenient for the user. Another solution is a static/global zoomFactor variable. I implemented something similar here: https://bugsfiles.kde.org/attachment.cgi?id=85775 (this is a patch attached to the issue). But I don't really like globals, and I didn't see much of those in the kdevplatform code.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unfortunately, I can't access https://phabricator.kde.org/D774 to evaluate the alternative implementation, because this particular page seems to be protected and I don't have an account there.</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;">I think that the alternative implementation is good too.
With respect to behavior, the biggest differences are:
1) the extra tool button (+1 functionality, but less space available);
2) horizontal scroll is not handled (which is interpreted in Qt Assistant and firefox in the way I implemented it).
3) larger/smaller wheel scroll steps are not handled. I couldn't see these different steps during debugging, but Qt documentation suggests that some devices support more fine-grained scrolling.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There are indeed less changes and they are less intrusive in Sergey's solution. But on the other hand, my implementation fits better in the existing architecture (zoom feature works similarly to the existing search feature), does not employ a qobject_cast trick and can support documentation widgets that don't use StandardDocumentationView.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not proficient in a proper KConfig use, so I guess my implementation is worse in this regard.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We could either merge these 2 solutions by choosing the best aspects of each (as defined by reviewers). Or maybe Sergey would want to enhance the implementation in a way suggested by Kevin in his review (I don't really have much time at the moment to re-implement StandardDocumentationView).</p></pre>
<br />
<p>- Igor</p>
<br />
<p>On January 9th, 2016, 6:56 p.m. UTC, Igor Kushnir 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 KDevelop.</div>
<div>By Igor Kushnir.</div>
<p style="color: grey;"><i>Updated Jan. 9, 2016, 6:56 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;">Add zoom support for documentation view</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;">Built, installed, run tests.</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>documentation/CMakeLists.txt <span style="color: grey">(ff57e258ab5c62ce737c4013005247d585e87b61)</span></li>
<li>documentation/documentationview.h <span style="color: grey">(5f7f6d8bb100ca3f2804cb716a6a4fe98c8ad05e)</span></li>
<li>documentation/documentationview.cpp <span style="color: grey">(9d184a071a49f25ce28f877ffb16c3d2c0b9f1f5)</span></li>
<li>documentation/documentationzoom.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>documentation/documentationzoom.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>documentation/standarddocumentationview.h <span style="color: grey">(ba454427165a0df8372fa6d51ebc714423845107)</span></li>
<li>documentation/standarddocumentationview.cpp <span style="color: grey">(a9185fd592771f9ead7d4216e08f8c93abab601a)</span></li>
<li>interfaces/idocumentation.h <span style="color: grey">(7673e5fe58ab736d864328b0c8c6b087b197867b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126685/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>