<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 nachm. 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;">How does this code play with the other code that remembers this stuff globally? Should that code be removed?</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;">yes, revision 2 reverts those changes, so revision 1 and 2 give the complete change. Is this the wrong way to post it? I'm sorry, I'm working with the reviewboard for the first time. Should I post complete diffs from the master instead so the different revisions will be independent?</p></pre>
<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 nachm. 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>
</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;">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>
<br />
<p>- Felix</p>
<br />
<p>On August 6th, 2015, 3:22 nachm. 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 Aug. 6, 2015, 3:22 nachm.</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">(9d12488)</span></li>
<li>core/view.h <span style="color: grey">(a369d24)</span></li>
<li>ui/pageview.h <span style="color: grey">(e65b575)</span></li>
<li>ui/pageview.cpp <span style="color: grey">(b57e6ae)</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>