<table><tr><td style="">loh.tar updated this revision to Diff 46411.<br />loh.tar 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/D17193">View Revision</a></tr></table><br /><div><div><p>I think that's all. <br />
Well, it looks less beneficial as previous anticipated by me, but I think it's still the right direction.</p>

<p>Just a view questions regarding the mix in the KTextEditor::ViewPrivate code how to access some data:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">doc() vs m_doc: doc() is inline</li>
<li class="remarkup-list-item">cursorPosition() vs m_viewInternal->m_cursor: cursorPosition() is NOT inline</li>
</ul>

<p>I may have used in my patch also a mix due to the variety of existing code. Let me know which one to use.<br />
Furthermore I offer a S&R to unify the existing code, as an own DIFF/ticket or if you like in this one.</p>

<p>Normally I would prefer the function calls, but because the highliting looks so nice :-) and cursorPosition() is not inline I'm undecided.<br />
Would it be make sense to change them to inline?</p>

<p>Some more things I noticed and offer to change.</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">KateViewInternal::updateView (slot) calls KateViewInternal::doUpdateView (nowhere else called). Any benefit from that construct? Think its only a leftover from the ancient. (would make sense to include in this patch)</li>
</ul>

<ul class="remarkup-list">
<li class="remarkup-list-item">KateViewInternal::renderer() (often used) just returns KTextEditor::ViewPrivate::m_renderer but is NOT inline. When removed, could the call "renderer()" replaced by "m_view->m_renderer" or "m_view->renderer()" (the latter is also NOT inline)</li>
</ul>

<p>KateViewInternal contains some additional classes: CalculatingCursor, BoundedCursor, WrappingCursor, together ~275 lines<br />
ZoomEventFilter ~53 lines</p>

<p>At least the cursor stuff looks worth to move into an own file.</p>

<p>Some out-commented FIXME KF5 stuff (4 years old) encapsulated in #ifndef QT_NO_ACCESSIBILITY, lines ~1921 and ~747.<br />
May removed in above S&R offer when commit is named "Cleanup" or similar.</p></div></div><br /><div><strong>CHANGES SINCE LAST UPDATE</strong><div><a href="https://phabricator.kde.org/D17193?vs=46327&id=46411">https://phabricator.kde.org/D17193?vs=46327&id=46411</a></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D17193">https://phabricator.kde.org/D17193</a></div></div><br /><div><strong>AFFECTED FILES</strong><div><div>src/view/kateview.cpp<br />
src/view/kateviewinternal.cpp<br />
src/view/kateviewinternal.h</div></div></div><br /><div><strong>To: </strong>loh.tar, KTextEditor<br /><strong>Cc: </strong>cullmann, kwrite-devel, kde-frameworks-devel, KTextEditor, hase, michaelh, ngraham, bruns, demsking, sars, dhaumann<br /></div>