<table><tr><td style="">simgunz added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D15580">View Revision</a></tr></table><br /><div><div><p>There is a TODO list of the working and planned feature on task <a href="https://phabricator.kde.org/T8076" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">T8076</a>. Some of your suggestions are already there and I added the missing one. I have updated the description of this review to point to that TODO for better clarity.</p>
<blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D15580#473460" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D15580#473460</a>, <a href="https://phabricator.kde.org/p/ngraham/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@ngraham</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>getting there!</p>
<p>UI review of the latest version:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">Having the annotation tools on the main toolbar makes it overflow for all but the largest window sizes. How about putting them in a secondary toolbar below the main one that's hidden until the user shows it by clicking on a "Show annotation tools" item on the main toolbar and/or menubar?</li>
</ul></div>
</blockquote>
<p>Already in TODO</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">The <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Line width</span></span></span> dropdown menu button should make the whole button clickable to show the drop-down rather than only the space under the arrow on the right</li>
</ul></blockquote>
<p>Added to TODO</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">The entries in the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Line width</span></span></span> dropdown should visually reflect the width of the line. Could these graphics be generated programmatically rather than using icons?</li>
</ul></blockquote>
<p>Added to TODO</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item"><span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Squiggle</span></span></span> and <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Arrow</span></span></span> need new icons; please file a bug on Breeze | Icons and request them</li>
</ul></blockquote>
<p>Already in TODO. (I was waiting before requesting icons to see if I managed to get something working :-) )</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">The <tt style="background: #ebebeb; font-size: 13px;">draw-polygon</tt> icon might make it seem like that tool can only draw pentagons, consider using <tt style="background: #ebebeb; font-size: 13px;">draw_polyline</tt> or <tt style="background: #ebebeb; font-size: 13px;">draw-polygon-star</tt> instead</li>
</ul></blockquote>
<p>I'll set it to draw_polyline</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">The Color button should actually show the current color rather than a generic icon</li>
</ul></blockquote>
<p>This should already work.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">The Inner color button needs something to show that it exists; right now it just looks like whitespace in the toolbar. It should show its color like the other button, and for "no color", maybe an empty transparent square?</li>
</ul></blockquote>
<p>Also the icons of color and inner color need to be done from scratch. I was thinking at a circle fillable with the current color and a border to show it exists.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item"><span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Straight Line</span></span></span> is mis-named, it's for drawing anything <em>but</em> a straight line! :) Should be something like "Freehand line"</li>
</ul></blockquote>
<p>Have you removed .config/okularpartrc ? Otherwise the toolbar picks up the annotation in you custom orders and the buttons are mismatched.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">I can't figure out what <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Pin Annotation</span></span></span> actually does</li>
</ul></blockquote>
<p>If checked the current annotation tool is kept selected after use (as double-click does in the current Okular). Needs a better name/tooltip. Added to TODO.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><ul class="remarkup-list">
<li class="remarkup-list-item">It's not clear to me how to select existing annotations once an annotation tool has been activated; consider maybe adding a "select annotations" tool or mode under the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Selection</span></span></span> dropdown menu.</li>
</ul></blockquote>
<p>Currently you need to click Esc to deselect the annotation, then you can select the annotations (standard Browse mode). If instead 'pin annotation' is unchecked, the annotation is deselected automatically and you can select annotation. Beside the fact that clicking on an annotation does not select it (added to TODO) and that selecting and annotation does not switch to Browse mode (added to TODO), it works as the current version of Okular. I think we do not need a dedicated Selection tool.</p>
<blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D15580#473460" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D15580#473460</a>, <a href="https://phabricator.kde.org/p/ngraham/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@ngraham</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>getting there!</p>
<p>UI review of the latest version:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">Having the annotation tools on the main toolbar makes it overflow for all but the largest window sizes. How about putting them in a secondary toolbar below the main one that's hidden until the user shows it by clicking on a "Show annotation tools" item on the main toolbar and/or menubar?</li>
<li class="remarkup-list-item">The <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Line width</span></span></span> dropdown menu button should make the whole button clickable to show the drop-down rather than only the space under the arrow on the right</li>
<li class="remarkup-list-item">The entries in the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Line width</span></span></span> dropdown should visually reflect the width of the line. Could these graphics be generated programmatically rather than using icons?</li>
<li class="remarkup-list-item"><span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Squiggle</span></span></span> and <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Arrow</span></span></span> need new icons; please file a bug on Breeze | Icons and request them</li>
<li class="remarkup-list-item">The <tt style="background: #ebebeb; font-size: 13px;">draw-polygon</tt> icon might make it seem like that tool can only draw pentagons, consider using <tt style="background: #ebebeb; font-size: 13px;">draw_polyline</tt> or <tt style="background: #ebebeb; font-size: 13px;">draw-polygon-star</tt> instead</li>
<li class="remarkup-list-item">The Color button should actually show the current color rather than a generic icon</li>
<li class="remarkup-list-item">The Inner color button needs something to show that it exists; right now it just looks like whitespace in the toolbar. It should show its color like the other button, and for "no color", maybe an empty transparent square?</li>
<li class="remarkup-list-item"><span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Straight Line</span></span></span> is mis-named, it's for drawing anything <em>but</em> a straight line! :) Should be something like "Freehand line"</li>
<li class="remarkup-list-item">I can't figure out what <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Pin Annotation</span></span></span> actually does</li>
<li class="remarkup-list-item">It's not clear to me how to select existing annotations once an annotation tool has been activated; consider maybe adding a "select annotations" tool or mode under the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">Selection</span></span></span> dropdown menu.</li>
</ul></div>
</blockquote>
</div></div><br /><div><strong>REPOSITORY</strong><div><div>R223 Okular</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D15580">https://phabricator.kde.org/D15580</a></div></div><br /><div><strong>To: </strong>simgunz, Okular<br /><strong>Cc: </strong>knambiar, ngraham, tobiasdeiminger, okular-devel, joaonetto, tfella, darcyshen, aacid<br /></div>