Potential Bug (Qt 5.15): QLineEdit emits "editingFinished()" signal twice when the text is changed and enter is pressed

oblivioncth

Ok I made a new project with the minimum requirements to recreate the issue and tested it on the pre-compiled release of Qt 5.15.0 MSVC2019 64-bit that you can get through the Qt website/maintenance tool and unfortunately the issue does still occur. Really curious as to what's up with this one since you weren't able to recreate it.

See the following quick example:
https://www.youtube.com/watch?v=hRgSA_0lm9k&feature=youtu.be

This happens regardless of the build configuration (i.e. same issue on Release config and using just "Run" instead of "Start debugging").

Also, since I've seen some older mentions of this issue happening when a QMessageBox is used in the corresponding slot, I also tried just putting dummy code (creating an int and then incrementing it once) and using the debugger to break within the slot and it is still fired twice in the 3rd case when no message box is used.

mrjj

Hi
Could you try my small test project
https://www.dropbox.com/s/o9p7hbhb0nr3054/TestLineEdit.zip?dl=0

Also if possible i want to try your small test project and see.

oblivioncth

@mrjj

Ok, I've figured out exactly what is happening due to your test project and should have thought to use qDebug earlier, as there happened to be a coincidence between my two testing methods that caused a false positive for "detecting" this would-be bug (this coincidence is why I thought the QMessageBox usage was not related when in fact it was).

You are correct, the editingFinsihed() signal does work "correctly", though my realization has led me to discover another quirk of the signal that I feel is still an oversight, albeit a lesser one.

Here is what was happening:

editingFinsihed() is supposed to trigger when the Return key is pressed, or when the QLineEdit loses focus, with the exception that it will not trigger upon only (key for later) focus loss if no changes were actually made to the QLineEdit's contents as I mentioned previously (undocumented in the Qt docs). Note that the key detail here is that pressing the Return key does trigger the signal, but does not cause the QLineEdit to drop focus as well.

With my example as shown in the video whenever I would press enter in the QLineEdit the editingFinsihed() signal would fire once, but then when the QMessageBox would pop and draw focus that would count as the QLineEdit losing focus and so editingFinsihed() would trigger a second time. The catch/coincidence is that when I then tested the project without the QMessageBox and simply had the slot do a dummy operation, I was testing for the number of times the signal was called by using a breakpoint on the dummy operation and checking how many times the breakpoint was hit. As in the first example editingFinsihed() would fire once when pressing enter; however, I was completely caught off guard by the fact that the debugger being brought into the foreground when the breakpoint is hit also counts as the QLineEdit losing focus and so in this case editingFinished() would also fire a second time (again, assuming changes were made to the contents).

Because in both cases the signal fires twice for different reasons I incorrectly assumed that there was a bug and that QMessageBox was not to blame when in fact it and my usage of the debugger were both secondary causes of the issue for separate reasons.

I say secondary because there is ultimately another quirk/oversight of this signal that if different would have prevented this entire series of events (and you may have been able to pickup on it even from just this explanation). Here is what I mean:

Officially, editingFinsihed() triggers as described here, which if that is exactly how it worked would be fine and the end of this issue; however, by including the undocumented feature I mentioned above, the Qt developers clearly were aiming to enhance this signals functionality by preventing redundant/undesirable triggers. This makes sense. If the user simply clicks into the QLineEdit and then clicks off of it without a single content changing keystroke occurring, there is no reason to fire this signal as the QLineEdit hasn't truly been edited at all.

This is where my argument for there being an oversight comes in. The above explanation makes the intentions of the Qt developers and this signal clear: Fire this signal when the QLineEdit has actually been edited and then loses focus, or the return key is pressed. There is no need to fire the signal when no state changes have occurred whatsoever. However, there are two sides to the coin of this ideology and while they covered one side with this undocumented QoL feature I've described, they missed the other one. If you click into the QLineEdit and actually edit it (i.e. add a character) and then press enter, editingFinsihed() triggers as expected, which means that it's current contents have now been handled by the programmer and this internal flag Qt uses to note that no changes to the widget have actually occurred should be reset to "true" until further changes are made, but it is not. If you then drop the QLineEdit's focus immediately after pressing the enter key the editingFinsihed() signal will fire again even though no edit's of any kind have occurred since the last time the signal was fired. To me it is clear that in this situation the signal should not fire, as otherwise there is a direct inconsistency with this undocumented feature.

If the idea is that losing QLineEdit's focus after no editing actually occurs since doesn't fire the signal then this should also remain true even if the last thing to "finish the edits" was the return key. Or in other words, if clicking into the widget, making a change and then clicking out fires the signal, but then clicking back in and out while making no changes doesn't fire the signal, then the signal should also not fire a second time if the user click's in, makes a change and presses enter, and then clicks out.

I could probably explain this better in a short video verbally and with an on-screen example, but I think it should be clear what I'm getting at. If not let me know. If so do you agree with my reasoning?

Do you happen to know the preferred channel to make a request for this behavior to be changed? I know Qt is opensource but with how huge it is a pull request and change from some random person isn't likely to be accepted/merged I'd imagine.

oblivioncth

If this step-by-step that covers all cases for your test program makes it more clear:

Click into the line edit and type "a" into it, then click the "Take Focus" box -> editingFinished() fires as expected
Click back into the line edit, then press enter -> editingFinished() fires as expected
Click out of the line edit -> editingFinished() doesn't fire due to the undocumented QoL feature
Click into the line edit, and then back into the "Take Focus" box -> editingFinished() doesn't fire due to the undocumented QoL feature
Click into the line edit, type "b" into it and the press enter -> editingFinished() fires as expected
Click out of the line edit -> based on the QoL feature paradigm clearly established at this point we would expect editingFinished() not to fire here since the last edits were already accounted for by the previous return key press, but it fires again anyway

mrjj

Hi
Good, write up. It's 100% clear to me.
I agree that it's not optimal handled as it would also happen in a very common use case where enter will move to the next input line and in that case also fire 2 editingFinished as
far as i can see from your test case. User type text and press enter, then next edit is entered and old loose focus.

Do you happen to know the preferred channel to make a request for this behavior to be changed? I know Qt is opensource but with how huge it is a pull request and change from some random person isn't likely to be accepted/merged I'd imagine.

They do accept incoming patches but they care a lot of breaking code/ABI so
might not agree.

However, I would ask on the mailing list where the devs hang out.
https://lists.qt-project.org/

SGaist

Hi,

To add to @mrjj the size of the patch does not matter. If really too big for some reason, you will be asked to split it.

If breaking some ABI, now is the good time since you will have to first provide it for Qt 6 anyway.

Since we are in a period of Holidays, you might have to wait a bit for the reviews but in any case, do not hesitate to provide a patch. It will be welcome.

oblivioncth

@mrjj said in Potential Bug (Qt 5.15): QLineEdit emits "editingFinished()" signal twice when the text is changed and enter is pressed:

Hi
Good, write up. It's 100% clear to me.
I agree that it's not optimal handled as it would also happen in a very common use case where enter will move to the next input line and in that case also fire 2 editingFinished as
far as i can see from your test case. User type text and press enter, then next edit is entered and old loose focus.

Do you happen to know the preferred channel to make a request for this behavior to be changed? I know Qt is opensource but with how huge it is a pull request and change from some random person isn't likely to be accepted/merged I'd imagine.

They do accept incoming patches but they care a lot of breaking code/ABI so
might not agree.

However, I would ask on the mailing list where the devs hang out.
https://lists.qt-project.org/

@SGaist said in Potential Bug (Qt 5.15): QLineEdit emits "editingFinished()" signal twice when the text is changed and enter is pressed:

Hi,

To add to @mrjj the size of the patch does not matter. If really too big for some reason, you will be asked to split it.

If breaking some ABI, now is the good time since you will have to first provide it for Qt 6 anyway.

Since we are in a period of Holidays, you might have to wait a bit for the reviews but in any case, do not hesitate to provide a patch. It will be welcome.

Thanks for the information.

I looked into it a bit and did see there was a wiki page describing the official procedure for contributing for the project and while I've never used Gerrit before or staged a commit for something other than much smaller projects, there seeems to be enough guidelines that it makes the process somewhat clear.

The changes required should be quite minimal, so I'll re-download the sources and look at qlinewidget.cpp to see where the signal is emitted and what would need to be done. If I get stuck trying to submit a patch I'm sure someone could give me a nudge through the IRC or email mentioned on the page that @mrjj mentioned.

SGaist

While the documentation might look a bit daunting, it's just pretty thorough so you know what you are doing. The procedure is not as hard as it looks like.

You'll also find people here :-)

oblivioncth

@SGaist

Sorry got busy for a while (mainly working on the project I'm making use of QLineEdit in, lol).

Really appreciate the support as it ensures I'll be able to get this implemented one way or another without having to be too anxious over the process; though, its initial intimidation is certainly just a side effect of its reasonable formality given the size of Qt and how many people can be impacted by changes to it.

I will start looking this over in the next few days.

oblivioncth

My behavior correction patch for this one has also gone through to be available in 6.0/the current dev build

https://codereview.qt-project.org/c/qt/qtbase/+/312083

Cheers.