<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, 12:37 p.m. CET, <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, 12:44 p.m. CET, <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, 6:52 p.m. CET, <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>
<p>On January 11th, 2016, 7:26 p.m. CET, <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 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>
</blockquote>
<p>On January 23rd, 2016, 6:07 p.m. CET, <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 made changes on top of this review request in several branches.
In one branch, a KToolBar mActions is eliminated, simple QActions are added via DocumentationView::addAction; QComboBox mProviders and QLineEdit mIdentifiers are added in a QHBoxLayout <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">under</em> the back/forward/find/home/zoom icons.
In another branch, a QToolBar addressActions is added <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">above</em> mActions; mProviders and mIdentifiers are moved to addressActions.
See screenshots here: https://drive.google.com/uc?export=download&id=0B0F-aqLyFk_PaGVxWU54cWJaUTA
Let me know which implementation looks best, and I'll update this review request with the corresponding branch.</p></pre>
</blockquote>
<p>On January 23rd, 2016, 7:37 p.m. CET, <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;">Removed text from buttons on the documentation find widget. I don't like the idea to remove the case-insensitive functionality, so I haven't done this. The find widget patch is totally independent from this review request, so I will create a separate review request. This is a link to screenshots with zoom and search patches combined: https://drive.google.com/uc?export=download&id=0B0F-aqLyFk_PQU05b1ItbGo3aEU
Please choose the best-looking documentation zoom implementation, assuming that it is combined with the search patch.</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;">2 thoughts if I may chime in:
- the mouse wheel zoom is extremely annoying for those of us who use a trackpad with 2 finger scroll and inertia, because that is handled as scroll wheel events (= those keep coming in for the duration of the inertial scroll). Any support for scroll-wheel should be coupled with a shortcut to go back to the default zoom level (typically Ctrl-0) IMHO.
- other views could benefit greatly from a font size configure option, or better yet a font selection option (not really any more complicated to implement), like for instance a smaller sized "Narrow" font in certain list views that should take the full window width.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On January 9th, 2016, 7:56 p.m. CET, 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, 7: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>