<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/103427/">http://git.reviewboard.kde.org/r/103427/</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 22nd, 2012, 7:12 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;">Hi, me again, sorry for the delay.
I find the current implementation a bit confusing user-wise since i can have the "Page number" toolbar shown and no "Page number" toolbar is really visible at all since the other page widget is visible.
For that i'm suggesting two solutions:
a) Do not force the limit of only one minibar available, so the user can have none, one or two page widgets visible
b) Uncheck the "Page number" toolbar action when the "old" page widget is visible, and then if somebody checks it, toggle it back
The option b) looks difficult-ish since you'd probably have to do quite a bit of xml-gui vodoo to make it work nicely, so i'm leaning towards a)
What do you think?</pre>
</blockquote>
<p>On January 22nd, 2012, 8:34 p.m., <b>Jonathan Marten</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 objection to doing (a), since that would avoid all of the user confusion with only one of the tool and page bar being able to be shown at any time. It will slightly complicate the MiniBar implementation, the intended observerId() will have to be specified to its constructor instead of being hardwired in minibar.h. With that being available, two MiniBar's can be created for the page bar and for the tool each with a unique ID. If this is acceptable in terms of code complexity and BC then I will work on the changes.
</pre>
</blockquote>
<p>On January 22nd, 2012, 8:53 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;">Why do you need two different observed id? After all even if it's two "different" widgets they are basically "mirrors" one of the other, i think it should be doable with just one observer id.</pre>
</blockquote>
<p>On January 22nd, 2012, 9:18 p.m., <b>Jonathan Marten</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;">Because of Document::addObserver(DocumentObserver *pObserver), which stores the pObserver in a QMap indexed by the observerId. So adding a second observer with the same observerId as one already existing will overwrite the first.
It could do an insertMulti() into the map, but then many more changes would be needed in Document to handle this.</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;">Or you could decouple the logic of the minibar from the widget itself, sharing the same logic for both of the widgets, that would make sense imho, but it's probably more work than you were expecting, what do you think? i'm not totally against adding a new observer id but i'd like to avoid it unless totally necessary</pre>
<br />
<p>- Albert</p>
<br />
<p>On December 28th, 2011, 8:21 p.m., Jonathan Marten 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 Jonathan Marten.</div>
<p style="color: grey;"><i>Updated Dec. 28, 2011, 8:21 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;">The bug (and duplicates) suggests that the page bar which normally appears at the bottom of the Okular window could be docked in the toolbar, in order to save vertical screen space which is especially useful on a wide screen. This patch implements that.
There can only be one MiniBar in existence, because of the fixed observer ID (which must be unique). So the page tool only appears in the toolbar if the page bar is hidden; if it is shown, then the page number and size label appears there as before. The MiniBar is reparented in Part::slotShowBottomBar() when the page bar is shown or hidden.
The page tool is placed in its own toolbar by default, so that it can be positioned or floated in accordance with the user's preference.
There are GUI changes and a new I18N string, so this change (if accepted) would be targeted at KDE SC 4.8.1 or later.
</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;">Built Okular from master with these changes. Checked operation in both embedded and standalone modes with page bar shown or not, with a variety of PDF files.</pre>
</td>
</tr>
</table>
<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=279128">279128</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>part.rc <span style="color: grey">(33d3829)</span></li>
<li>part.cpp <span style="color: grey">(b3b4234)</span></li>
<li>part-viewermode.rc <span style="color: grey">(dbd8e42)</span></li>
<li>part.h <span style="color: grey">(cae5af0)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103427/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/103427/s/372/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2011/12/16/okular-pagetool_1_400x100.png" style="border: 1px black solid;" alt="With page bar shown" /></a>
<a href="http://git.reviewboard.kde.org/r/103427/s/373/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2011/12/16/okular-pagetool_2_400x100.png" style="border: 1px black solid;" alt="With page bar hidden" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>