Review Request 116747: Clean up KCompletionBox

[Alex Merry]

> On March 11, 2014, 11:17 p.m., Aleix Pol Gonzalez wrote:
> > src/kcompletionbox.h, line 228
> > <https://git.reviewboard.kde.org/r/116747/diff/1/?file=253404#file253404line228>
> >
> >     I wouldn't leave the implementation here. Move it to the .cpp file, this way it can be changed in the future, if it's required for some reason.
> >     
> >     Also there's a typo in the method name.
> 
> David Gil Oliva wrote:
>     Alex Merry inlined deprecated methods in https://git.reviewboard.kde.org/r/116012 , so I thought that it was the way to go...

Well, there's a balance to be struck: putting them in the header ensures there is no runtime cost to programs that don't use the deprecated methods, as the code should be optimised away, that the library is always binary-compatible even if you compile it with deprecated code disabled (*_NO_DEPRECATED) and makes the header code document how to replace existing calls.  The downsides are an inability to fix the code later and an inability to access members of a private d-pointer class.  Neither of those are an issue here, as we're just renaming the method.

tl;dr: I disagree with Aleix, and think it should stay in the header.

Oh, and Aleix: could you please select the whole method when you're doing a comment like this, rather than just the first line? Otherwise it's a pain to see what you're referring to.  Thanks :-)


On March 11, 2014, 11:17 p.m., David Gil Oliva wrote:
> > Have you looked through the uses of the "un-slotted" methods? (lxr.kde.org). Maybe there's a reason for that... :/
> 
> David Gil Oliva wrote:
>     Maybe I'm totally wrong, but I can't imagine any way that a getter can be useful as a slot. 
>     
>     connect (widget, SLOT(valueChanged(), completionBox, SIGNAL(isTabHandling());
>     
>     Does it make sense?    ¿?:-/

Non-void slots are only useful if they work as a slot (ie: have some sort of side-effect) and might be called directly or with QMetaObject::invokeMethod().  If the method is const (like a getter), there's no point having it a slot at all; if you want to be able to use it with invokeMethod, you can just make it Q_INVOKABLE.


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116747/#review52703
-----------------------------------------------------------


On March 11, 2014, 10:32 p.m., David Gil Oliva wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116747/
> -----------------------------------------------------------
> 
> (Updated March 11, 2014, 10:32 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kcompletion
> 
> 
> Description
> -------
> 
> Clean up KCompletionBox
> 
> -canceled() -> cancelled (private method)
> -Deprecate sizeAndPosition() --> resizeAndReposition()
> -Remove old comments and commented-out code
> -Move some slots to be normal methods, since they don't seem to be able to
> work as slots.
> 
> 
> Diffs
> -----
> 
>   src/kcompletionbox.h 09b7527 
>   src/kcompletionbox.cpp 92e87b3 
> 
> Diff: https://git.reviewboard.kde.org/r/116747/diff/
> 
> 
> Testing
> -------
> 
> It builds and tests pass.
> 
> 
> Thanks,
> 
> David Gil Oliva
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140312/d814e20c/attachment.html>