Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/#review53374 --- This review has been submitted with commit 6df2b486496ed500f5fbc02c4f3a58a0eb4f4b3f by David Gil to branch master. - Commit Hook On March 14, 2014, 8:46 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 14, 2014, 8:46 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/ --- (Updated March 18, 2014, 10:25 p.m.) Status -- This change has been marked as submitted. 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/#review53189 --- Ship it! Ship It! - Kevin Ottens On March 14, 2014, 8:46 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 14, 2014, 8:46 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/#review52928 --- src/kcompletionbox.h https://git.reviewboard.kde.org/r/116747/#comment37251 That two 'd's too many. - David Faure On March 13, 2014, 10:13 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 13, 2014, 10:13 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
On March 14, 2014, 9:18 a.m., David Faure wrote: src/kcompletionbox.h, line 229 https://git.reviewboard.kde.org/r/116747/diff/4/?file=253685#file253685line229 That two 'd's too many. Aaargh!! I added those two d's for refactoring and then I forgot to change it (actually I forgot two times, because one of the reviewers already told me)... The next time I will use something that doesn't compile. I'm sorry, I program at night and lately I'm usually very tired (I just had a baby three months ago). - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/#review52928 --- On March 13, 2014, 10:13 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 13, 2014, 10:13 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/ --- (Updated March 14, 2014, 8:46 p.m.) Review request for KDE Frameworks. Changes --- Hopefully everything is fixed now. 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 (updated) - 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/#review52953 --- Ship it! Ship It! - Alex Merry On March 14, 2014, 8:46 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 14, 2014, 8:46 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/#review52881 --- src/kcompletionbox.cpp https://git.reviewboard.kde.org/r/116747/#comment37230 Is this comment definitely no longer relevant? It looks like it refers to the sendPostedEvents() call that's still there. - Alex Merry On March 12, 2014, 10:57 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 12, 2014, 10:57 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/ --- (Updated March 13, 2014, 10:13 p.m.) Review request for KDE Frameworks. Changes --- Restore and rephrase comment in kcompletionbox.cpp, following suggestion of Alex Merry. 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 (updated) - 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
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. Alex Merry inlined deprecated methods in https://git.reviewboard.kde.org/r/116012 , so I thought that it was the way to go... 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... :/ 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?¿?:-/ - David --- 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/#review52740 --- Looks sensible to me, but I'll let Aleix reply. src/kcompletionbox.h https://git.reviewboard.kde.org/r/116747/#comment37196 If you change the parameter name, you have to change the docs to match src/kcompletionbox.h https://git.reviewboard.kde.org/r/116747/#comment37195 KCOMPLETION_NO_DEPRECATED - Alex Merry 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
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... Alex Merry wrote: 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 :-) Well, I wouldn't bother about runtime penalty given that it's deprecated and we shouldn't be using it anyway. Also we can't make assumptions on how things are going to be optimized. But it's ok, I don't think it's worth discussing further, I doubt this is going to be a problem in the future. 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?¿?:-/ Alex Merry wrote: 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. Well, having const methods as slots doesn't make sense indeed, if it's not for exposing on the meta object system, that's why I said you could do a fast lxr. I just did, didn't find anything relevant. - Aleix --- 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
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... Alex Merry wrote: 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 :-) Aleix Pol Gonzalez wrote: Well, I wouldn't bother about runtime penalty given that it's deprecated and we shouldn't be using it anyway. Also we can't make assumptions on how things are going to be optimized. But it's ok, I don't think it's worth discussing further, I doubt this is going to be a problem in the future. I mean the runtime penalty for things that *don't* use it. If it's header-only, it doesn't go in the library, so there is no load-time symbol-lookup penalty, and no code-size penalty. Admittedly, both of those are probably negligible... - 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
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... Alex Merry wrote: 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 :-) Aleix Pol Gonzalez wrote: Well, I wouldn't bother about runtime penalty given that it's deprecated and we shouldn't be using it anyway. Also we can't make assumptions on how things are going to be optimized. But it's ok, I don't think it's worth discussing further, I doubt this is going to be a problem in the future. Alex Merry wrote: I mean the runtime penalty for things that *don't* use it. If it's header-only, it doesn't go in the library, so there is no load-time symbol-lookup penalty, and no code-size penalty. Admittedly, both of those are probably negligible... Thank you both of you for the useful hints. With this little discussion I have learned some criteria to decide what to do now and in the future. 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?¿?:-/ Alex Merry wrote: 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. Aleix Pol Gonzalez wrote: Well, having const methods as slots doesn't make sense indeed, if it's not for exposing on the meta object system, that's why I said you could do a fast lxr. I just did, didn't find anything relevant. Thank you for looking it up :-) - David --- 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/ --- (Updated March 12, 2014, 10:54 p.m.) Review request for KDE Frameworks. Changes --- Address Alex and Aleix suggestions. 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 (updated) - 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/ --- (Updated March 12, 2014, 10:57 p.m.) Review request for KDE Frameworks. Changes --- Fix non-commited change. 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 (updated) - 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 116747: Clean up KCompletionBox
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116747/#review52703 --- src/kcompletionbox.h https://git.reviewboard.kde.org/r/116747/#comment37176 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. Have you looked through the uses of the un-slotted methods? (lxr.kde.org). Maybe there's a reason for that... :/ - Aleix Pol Gonzalez 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
