<table><tr><td style="">davidhurka 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/D21266">View Revision</a></tr></table><br /><div><div><p>This is part of my goal to understand how TextEntity reordering works. There will probably be more patches like this soon.</p>
<p>I included some examples about how/where these classes are used. But why are there both RegularAreaRect (with its base classes) and ObjectRect?</p>
<p>What is a special purpose of ObjectRect? As it is used now, it’s indeed rectangular and RegularAreaRect could do most of it. ObjectRect derived objects do store their type, but are not stored mixed. An annotation or a source reference could simply link to an own RegularAreaRect or NormalizedPoint.</p>
<p>There are some more things I couldn't figure out. See my inline comments.</p>
<p>How can I add inline comments to unmodified files...?</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21266#inline-119300">View Inline</a><span style="color: #4b4d51; font-weight: bold;">area.h:108</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">static</span> <span style="color: #aa4000">double</span> <span style="color: #004012">distanceSqr</span><span class="p">(</span> <span style="color: #aa4000">double</span> <span class="n">x</span><span class="p">,</span> <span style="color: #aa4000">double</span> <span class="n">y</span><span class="p">,</span> <span style="color: #aa4000">double</span> <span class="n">xScale</span><span class="p">,</span> <span style="color: #aa4000">double</span> <span class="n">yScale</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span class="n">NormalizedPoint</span><span style="color: #aa2211">&</span> <span class="n">start</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span class="n">NormalizedPoint</span><span style="color: #aa2211">&</span> <span class="n">end</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Is there a reason to make this one function <tt style="background: #ebebeb; font-size: 13px;">static</tt>?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21266#inline-119301">View Inline</a><span style="color: #4b4d51; font-weight: bold;">area.h:192</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">bool</span> <span style="color: #004012">isNull</span><span class="p">()</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is implemented with double == 0. Won’t this always return true?</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">bool NormalizedRect::isNull() const
{
return left == 0 && top== 0 && right == 0 && bottom == 0;
}</pre></div></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21266#inline-119302">View Inline</a><span style="color: #4b4d51; font-weight: bold;">area.h:229</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">QRect</span> <span style="color: #004012">roundedGeometry</span><span class="p">(</span> <span style="color: #aa4000">int</span> <span class="n">xScale</span><span class="p">,</span> <span style="color: #aa4000">int</span> <span class="n">yScale</span> <span class="p">)</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What’s the purpose of rounding? The application code looks like it doesn’t matter.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21266#inline-119303">View Inline</a><span style="color: #4b4d51; font-weight: bold;">area.h:308</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">bool</span> <span style="color: #004012">isLeft</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">NormalizedPoint</span><span style="color: #aa2211">&</span> <span class="n">pt</span><span class="p">)</span> <span style="color: #aa4000">const</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Adjectives like ‘left’ or ‘top’ aren’t very precise. Is the rectangle left of the point or is the point left of the rectangle?</p>
<p style="padding: 0; margin: 8px;">Additionally, this is inconsistent to ‘isTop’ <-> ‘isTopOrLevel’. I suggest to rename the functions to names like:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">isAboveOrLevel</li>
<li class="remarkup-list-item">isLeftOrAmong</li>
<li class="remarkup-list-item">isLeftOfOrAlong</li>
</ul></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21266#inline-119305">View Inline</a><span style="color: #4b4d51; font-weight: bold;">area.h:379</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d">/**</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> * @short NormalizedRect that contains a reference to an object.</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> *</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">“A reference to an ‘object’” is very broad.</p>
<p style="padding: 0; margin: 8px;">This is an area on a page (like RegularAreaRect), and links to an Annotation or SourceReference, or something “not owned”.</p>
<p style="padding: 0; margin: 8px;">Some bad alternative ideas to ‘ObjectRect’: ;)</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">InterestingArea</li>
<li class="remarkup-list-item">AreaToClick</li>
<li class="remarkup-list-item">DetailArea</li>
<li class="remarkup-list-item">ActiveArea</li>
<li class="remarkup-list-item">IntelligentArea</li>
<li class="remarkup-list-item">SpeciaArea</li>
<li class="remarkup-list-item">ReferencingArea</li>
</ul></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21266#inline-119306">View Inline</a><span style="color: #4b4d51; font-weight: bold;">area.h:386</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> *</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> * Type / Class correspondence tab:</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> * - Action : class Action: description of an action</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What does this mean?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21266#inline-119304">View Inline</a><span style="color: #4b4d51; font-weight: bold;">area.h:412</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> * @param bottom The bottom coordinate of the rectangle.</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> * @param ellipse If true the rectangle describes an ellipse.</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #74777d"> * @param type The type of the storable object @see ObjectType.</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What’s the purpose of ‘ellipse’? Currently it’s not used.</p>
<p style="padding: 0; margin: 8px;">Are any of these parameters used at all?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21266#inline-119307">View Inline</a><span style="color: #4b4d51; font-weight: bold;">area.h:456</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">/**</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * Returns whether the object rectangle contains the point <span class="bright">@p x, @p y for the</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * <span class="bright">scaling factor</span> @p xScale <span class="bright">and</span> @p yScale.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * Returns whether the object rectangle contains the point <span class="bright">with absolute coordinates</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * <span class="bright">(@p x, @p y) at a page size of</span> @p xScale <span class="bright">x</span> @p yScale.</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The base class implementation just ignores the page size and takes the point as normalized.</p>
<p style="padding: 0; margin: 8px;">This method is not used anywhere. Maybe it should be fixed, ore be pure virtual?</p></div></div></div></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/D21266">https://phabricator.kde.org/D21266</a></div></div><br /><div><strong>To: </strong>davidhurka, Okular<br /><strong>Cc: </strong>okular-devel, joaonetto, tfella, ngraham, darcyshen, aacid<br /></div>