<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/123427/">https://git.reviewboard.kde.org/r/123427/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 10th, 2015, 10:59 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/123427/diff/2/?file=390795#file390795line1402" style="color: black; font-weight: bold; text-decoration: underline;">ui/pageview.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void PageView::notifyCurrentPageChanged( int previous, int current )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1402</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> int mode = 0;</pre></td>
</tr>
</tbody>
</table>
<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;">You should use a string here and not an int, otherwise if in the future we add, move or remove items in this menu the numbers are going to be a pain to store/restore</p></pre>
</blockquote>
<p>On August 12th, 2015, 8:46 a.m. UTC, <b>Felix Mauch</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 don't like hard-coded strings very much, as you'll have to change all those later on, as well. However, I agree, that my simple solution is even worse than that. What about using the data field of the action? The viewMode enum-Type from Okular::Settings is stored in there. So when new modes are added there or the menu entries are reordered, no further changes should be necessary. However, I'd still have to use an int internally, as I can't save the enum type in the QVariant. Yet, I could to a casting the</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #B00040">int</span> mode;
<span style="color: #008000; font-weight: bold">for</span> (<span style="color: #B00040">int</span> i<span style="color: #666666">=0</span>; i <span style="color: #666666"><</span> d<span style="color: #666666">-></span>aViewMode<span style="color: #666666">-></span>menu()<span style="color: #666666">-></span>actions().size(); <span style="color: #666666">++</span>i) {
<span style="color: #008000; font-weight: bold">const</span> QAction<span style="color: #666666">*</span> action <span style="color: #666666">=</span> d<span style="color: #666666">-></span>aViewMode<span style="color: #666666">-></span>menu()<span style="color: #666666">-></span>actions().at(i);
<span style="color: #008000; font-weight: bold">if</span>(action<span style="color: #666666">-></span>isChecked() ) {
QVariant mode_id <span style="color: #666666">=</span> action<span style="color: #666666">-></span>data();
mode <span style="color: #666666">=</span> mode_id.toInt();
<span style="color: #008000; font-weight: bold">break</span>;
}
}
<span style="color: #008000; font-weight: bold">return</span> mode;
</pre></div>
</p></pre>
</blockquote>
<p>On August 25th, 2015, 10:58 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes using something like the viewMode that doesn't tie physical order of the actions to the saved setting is much better.</p></pre>
</blockquote>
<p>On September 14th, 2015, 10:56 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Did you find the time to develop something like what was suggested above?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">Hej Albert,</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">yes, I did something, however work came in between. I've got some more time now and this issue has high priority on my agenda. Hopefully I'll get it finished this week.</p></pre>
<br />
<p>- Felix</p>
<br />
<p>On September 15th, 2015, 6:35 p.m. UTC, Felix Mauch 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 Okular.</div>
<div>By Felix Mauch.</div>
<p style="color: grey;"><i>Updated Sept. 15, 2015, 6:35 p.m.</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=341318">341318</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
okular
</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;">Adds the functiionality to save the view mode (single page, facing...) and continuous scrolling to the document information as it is already done with the zoom information.</p></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;">I use a version with this patch applied since a couple of days. The intended functionality seems to work reliably.
Maybe it would be nice to implement a "standard view" selection in the options dialog as it's done for the zoom mode.</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>core/document.cpp <span style="color: grey">(cd9ea6d)</span></li>
<li>core/view.h <span style="color: grey">(a369d24)</span></li>
<li>ui/pageview.h <span style="color: grey">(376bbdc)</span></li>
<li>ui/pageview.cpp <span style="color: grey">(dc9eab8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123427/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>