...move code to KTextEditor::ViewPrivate
Details
Details
- Reviewers
cullmann - Group Reviewers
KTextEditor - Commits
- R39:8b4cd5f37947: KateViewInternal: Remove unneeded functions
Diff Detail
Diff Detail
- Repository
- R39 KTextEditor
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Simplifications in that way are ok in my eye, given the extra level of functions have no gain. (not even encapsulation)
Comment Actions
I think that's all.
Well, it looks less beneficial as previous anticipated by me, but I think it's still the right direction.
Just a view questions regarding the mix in the KTextEditor::ViewPrivate code how to access some data:
- doc() vs m_doc: doc() is inline
- cursorPosition() vs m_viewInternal->m_cursor: cursorPosition() is NOT inline
I may have used in my patch also a mix due to the variety of existing code. Let me know which one to use.
Furthermore I offer a S&R to unify the existing code, as an own DIFF/ticket or if you like in this one.
Normally I would prefer the function calls, but because the highliting looks so nice :-) and cursorPosition() is not inline I'm undecided.
Would it be make sense to change them to inline?
Some more things I noticed and offer to change.
- 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)
- 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)
KateViewInternal contains some additional classes: CalculatingCursor, BoundedCursor, WrappingCursor, together ~275 lines
ZoomEventFilter ~53 lines
At least the cursor stuff looks worth to move into an own file.
Some out-commented FIXME KF5 stuff (4 years old) encapsulated in #ifndef QT_NO_ACCESSIBILITY, lines ~1921 and ~747.
May removed in above S&R offer when commit is named "Cleanup" or similar.
Comment Actions
I would prefer function calls to members, in the most cases, the performance difference is not measurable, even without inline.
For non-exported functions that are one liners, feel free to make them inline.
I have no issues with merging more stuff from internal to view, the distinction is ancient.
Perhaps we could do this in parts. Could you change here the use of members to functions?
Then we can apply this first.
Comment Actions
- Prefer function calls for member excess
- Merge KateViewInternal::updateView with KateViewInternal::doUpdateView
- Add documentation to related functions and some near by cosmetic. Well, "Better no docu than bad docu", so if that is to difficult to check now, I can remove it and ship as own diff
- Complete commit message
- Remove Testplan, I'm always surpsised to see that later in the commit message
Comment Actions
I am happy with this ;=)
Thanks for adding documentation to the header, such stuff is very welcome!