KTextEditor::Message: Review documentation
ClosedPublic

Authored by loh.tar on Dec 16 2018, 1:41 PM.

Details

Summary
  • Reduce redundant text
  • Change some argument names in the hope it's an improvement
  • Update a code example to fit new style
  • Try to improve some wording
  • Add some @see, @p and such, but probably not all will be needed due to "auto reference"
  • Fix some outdated info

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Dec 16 2018, 1:41 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptDec 16 2018, 1:41 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Dec 16 2018, 1:41 PM
dhaumann accepted this revision.Dec 16 2018, 1:51 PM
dhaumann added a subscriber: dhaumann.

Looks almost good, except for "optional" and a missing "be". Could you update again?

src/include/ktexteditor/message.h
337–342

Add an optional @p icon ... which will be shown....

This revision is now accepted and ready to land.Dec 16 2018, 1:51 PM
loh.tar updated this revision to Diff 47673.Dec 16 2018, 2:50 PM
  • Fix optionally/optional + be

I like to suggest that some more eyes take a look at that :-)

loh.tar updated this revision to Diff 47678.Dec 16 2018, 4:14 PM

-Rename member&parameter autoHide->delay to fit accepted change in header file

just an offer

Now you are mixing documentation changes with code changes. I would have preferred to have different review requests, especially since the documentation part was already reviewed. Further, I would prefer autoHideDelay instead of just delay. If we use "delay" only, then the question immediately arises "the delay of what"? With this in mind, the naming of "autoHide" as better, since at least the context was clear. I will take care of the documentation now, but will not take care of the renaming for the reason given.

Sorry for the hassle, will revert the update

Further, I would prefer autoHideDelay instead of just delay.

Are you refering here to the header or the code part?
The old name in the header was "autoHideTimeR" which is pretty wrong. So have I to change this in the header/docu to "autoHideDelay"? Or perhaps "autoHideTime"? Then is the change minimal.

dhaumann added a comment.EditedDec 16 2018, 6:46 PM

I integrated this now, see efa92627494726d5863dd01ea6571031c1178cd3 - the change also includes the rename of autoHide to autoHideDelay. The function names cannot be changed of course, since it's public API.

dhaumann closed this revision.Dec 16 2018, 6:53 PM