KateViewInternal: Remove unneeded functions
ClosedPublic

Authored by loh.tar on Nov 27 2018, 4:54 PM.

Details

Summary

...move code to KTextEditor::ViewPrivate

Diff Detail

Repository
R39 KTextEditor
Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Nov 27 2018, 4:54 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptNov 27 2018, 4:54 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Nov 27 2018, 4:54 PM

Simplifications in that way are ok in my eye, given the extra level of functions have no gain. (not even encapsulation)

loh.tar updated this revision to Diff 46411.Nov 28 2018, 3:04 PM

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.

cullmann requested changes to this revision.Nov 29 2018, 7:20 AM

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.

This revision now requires changes to proceed.Nov 29 2018, 7:20 AM
loh.tar updated this revision to Diff 46473.Nov 29 2018, 2:37 PM
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
  • 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
cullmann accepted this revision.Nov 29 2018, 2:55 PM

I am happy with this ;=)
Thanks for adding documentation to the header, such stuff is very welcome!

This revision is now accepted and ready to land.Nov 29 2018, 2:55 PM
This revision was automatically updated to reflect the committed changes.