fix incorrect emission of signals by kLineEdit
ClosedPublic

Authored by dweatherill on Jan 11 2018, 1:59 AM.

Details

Reviewers
dhaumann
cullmann
dfaure
Group Reviewers
Frameworks
Summary

BUG: 388798
BUG: 373004

The unit test of kLineEdit was previously checking incorrectly that textEdited _HAD_ been emitted when the text is changed with setText(). This is undesired behaviour.
This patch fixes the unit test, and changes the problematic behaviour by removing the extra emission of textEdited in the private signal _k_textChanged.
In addition this is the underlying cause of a bug in kdevelop (and probably several others).

Diff Detail

Repository
R284 KCompletion
Lint
Lint Skipped
Unit
Unit Tests Skipped
dweatherill created this revision.Jan 11 2018, 1:59 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 11 2018, 1:59 AM
dweatherill requested review of this revision.Jan 11 2018, 1:59 AM
anthonyfieroni added inline comments.
src/klineedit.cpp
63

You mean this is emitted by Qt and this is duplicate one or so?

dfaure added a subscriber: dfaure.Jan 11 2018, 8:44 AM
dfaure added inline comments.
src/klineedit.cpp
184

Why did you rename the slot? I don't understand "textEmitted", and the signal is of course still called textChanged. Isn't the slot renaming just noise?

mwolff added a subscriber: mwolff.Jan 11 2018, 8:59 AM
mwolff added inline comments.
autotests/klineedit_unittest.cpp
87

typo: if the text box is already clear*ed*

src/klineedit.cpp
63

I agree with dfaure, isn't this line here the only "real" change? The extended unit test is good too of course.

dweatherill added inline comments.Jan 11 2018, 1:21 PM
autotests/klineedit_unittest.cpp
87

ok, it's unclear, but not a typo.
According to QLineEdit documentation, clear() should only emit textChanged when there is actually text to clear.

So the word "cleared" kind of implies "only after we've called clear()" I guess I should change it to

//if text box is already empty, calling clear() shouldn't emit
src/klineedit.cpp
63

@anthonyfieroni no, the textEdited signal should not be emitted when textChanged happens... at least according to the documentation, textEdited should only happen when the user changes the text. In this case, we are directly emitting textEdited exactly when the user has not changed the text.

But yes, it is the only real change.

184

you are absolutely right! My mistake, I intended to rename it to textEdited, as this is what the signal we actually want to emit from QLineEdit is, and the name textEmitted is spurious.

I do think it should be renamed however, because the entire bug is that it should not be emitting textChanged in this case...

dweatherill added inline comments.Jan 11 2018, 1:25 PM
src/klineedit.cpp
184

actually, reading more carefully, I agree it should not be renamed, just the spurious emission of textEdited should go

dfaure added inline comments.Jan 11 2018, 1:37 PM
autotests/klineedit_unittest.cpp
87

Agreed, that's clearer (haha).

src/klineedit.cpp
184

yes, it's connected to textChanged, so it SHOULD be called _k_textChanged, otherwise it gets really confusing.

Please submit an updated patch so we can reason about it.

Is this "spurious emission" there since 5.0? I'm worried about breaking apps due to the behaviour change... (not saying that's a reason to reject this, just needs additional thinking/research about the possible breakage)

this updated diff removes the rename, just has fixed and extended unit tests and the one line fix for not emitting the textChanged signal spuriously.

from the git history I believe this issue has been in place since before 5.0.

Agree the breakage is worrying, but at the moment the existence of this bug is breaking stuff I think. At the very least, if the behaviour can't be changed, the documentation should be updated to be consistent.

Sorry to be a pain, but can you also update the description, which still talks about textEmitted()?

(FYI using arc diff to upload the patch would also provide context, but OK, I can open the file locally to find out about the context for those unittest changes....)

dweatherill edited the summary of this revision. (Show Details)Jan 11 2018, 3:38 PM

Sorry to be a pain, but can you also update the description, which still talks about textEmitted()?

(FYI using arc diff to upload the patch would also provide context, but OK, I can open the file locally to find out about the context for those unittest changes....)

no problem, I have updated the description.
I will install arc for any future work.

dfaure accepted this revision.Jan 12 2018, 9:09 AM

Thanks for the fix. Looking at the reasoning in 737a983feb the added emit is definitely wrong, as the unittest proves. Please push.

autotests/klineedit_unittest.cpp
71

LOL the comment above says "setText emits textChanged and userTextChanged, but not textEdited", and the code didn't match that. Someone quickly adapted the tests after someone else added the emit, none of them really thought about this, sigh.

This revision is now accepted and ready to land.Jan 12 2018, 9:09 AM

as I don't have a developer account, I will need someone else to push it please :-)

If this actually fixes any of the bugs you mentioned, please edit the Summary section to include "BUG: <bug number>" on its own line for each bug that this fixes. Those are special keywords that will cause the Bugzilla tickets to get closed when this lands.

If there's any bug that's relevant but not directly fixed by this, you can do "CCBUG: <bug number>", which will make a comment in the Bugzilla ticket, but not close it.

dweatherill edited the summary of this revision. (Show Details)Jan 12 2018, 8:31 PM

I have updated the description to tag relevant bugs

I don't have commit access but please let me know if anything else is needed before it can be merged.

(for submission)
Name: Dan Weatherill
email: dan.weatherill@cantab.net

dfaure closed this revision.Jan 19 2018, 8:50 AM

Pushed in https://commits.kde.org/kcompletion/31f226116f99bcf40ddc67fe6328594d5fb46222, thanks for the contribution! Keep 'em coming ;-)