<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="http://git.reviewboard.kde.org/r/103424/">http://git.reviewboard.kde.org/r/103424/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 19th, 2011, 7:12 p.m., <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="http://git.reviewboard.kde.org/r/103424/diff/3/?file=43585#file43585line3646" style="color: black; font-weight: bold; text-decoration: underline;">ui/pageview.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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::slotRelayoutPages()</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">3646</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="kt">bool</span> <span class="n">facingPages</span> <span class="o">=</span> <span class="p">(</span><span class="n">facing</span> <span class="o">||</span> <span class="n">facingCentered</span><span class="p">)</span> <span class="o">&&</span> <span class="o">!</span><span class="n">overrideCentering</span><span class="p">;</span></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;">Wouldn't this just be
const bool facingPages = facing || centerFirstPage;
?</pre>
</blockquote>
<p>On December 19th, 2011, 10:14 p.m., <b>Stephen Anthony</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;">No, because if overrideCentering is enabled, then it doesn't matter whether facing mode is specified or not; it is disabled. Your fix includes the possibility that overrideCentering may be true (which forces centerFirstPage to be false), yet facing will still be true (because of the ||). But this should never happen, since overrideCentering *always* overrides any facing mode.</pre>
</blockquote>
<p>On December 19th, 2011, 10:31 p.m., <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;">Hmmm, can you check my logic and tell me what's wrong?
const bool facingPages = (facing || facingCentered) && !overrideCentering;
?
const bool facingPages = ((facing && !overrideCentering) || (facingCentered && !overrideCentering))
? because const bool centerFirstPage = facingCentered && !overrideCentering;
const bool facingPages = ((facing && !overrideCentering) || (centerFirstPage))
? because const bool overrideCentering = facingCentered && pageCount < 3;
const bool facingPages = ((facing && !(facingCentered && pageCount < 3)) || (centerFirstPage))
?
const bool facingPages = ((facing && (!facingCentered || pageCount >= 3)) || (centerFirstPage))
? because const bool facing = Okular::Settings::viewMode() == Okular::Settings::EnumViewMode::Facing; and const bool facingCentered = Okular::Settings::viewMode() == Okular::Settings::EnumViewMode::FacingFirstCentered; thus if facing is true facingCentered will be fase confirming the first part of the || so we can just collapse it
const bool facingPages = ((facing) || (centerFirstPage))</pre>
</blockquote>
<p>On December 19th, 2011, 10:32 p.m., <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;">Gah, the ? should be down facing arrows...</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;">You're right, there's nothing wrong with your logic. I refer to the original description, which basically states to override 'facing (with center)' only. This means if facing mode is used, we don't care about the override. So facingPages is indeed "facing || centerFirstPage". I'll attach a new patch in a moment.</pre>
<br />
<p>- Stephen</p>
<br />
<p>On December 17th, 2011, 11:16 p.m., Stephen Anthony wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Okular.</div>
<div>By Stephen Anthony.</div>
<p style="color: grey;"><i>Updated Dec. 17, 2011, 11:16 p.m.</i></p>
<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;">This patch improves 'facing pages (center first page)' mode in the case where a document consists of only 1 or 2 pages. Currently, this mode always assumes two columns of output (which makes sense for 'facing pages'). However, in the case of 1 or 2 page documents, the pages are sized according to two columns, when only one column will ever be used (because of the number of pages present), resulting in pages that don't fill the page in 'Fit to Page' view mode.
Basically, this patch overrides facing pages in such a case and uses one virtual column, allowing the page to take all available space.</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;">Testing was done on 1, 2, 3, many page documents in all modes and views. This patch addresses the specific case where 'facing pages (center first page)' is enabled AND the pagecount is 2 or less.</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>ui/pageview.cpp <span style="color: grey">(78a007a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103424/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>