CodeWidget : Dispose of completion menu on activatedSignal

[murraystevenson]

This fixes the "Ctrl+Return doesn't always commit my changes when editing set expressions in the Render Pass Editor" issue reported recently. If the completion menu was visible while Ctrl+Return was pressed, changes would not be committed due to the loss of focus workaround in CodeWidget._emitEditingFinished(). Or in the case of a widget in a PlugPopup, the changes would not be committed and the PlugPopup would dismiss...

This was particularly apparent in scenes containing overlapping set names such as "LGT" and "LGT:ALL", and entering the shorter "LGT" set into a SetExpressionPlugValueWidget in a PlugPopup. Typing "LGT" would leave the completion menu up for "LGT:ALL", and then pressing Ctrl+Return would dismiss the PlugPopup without committing the change while often leaving the completion menu still visible afterwards.

[johnhaddon]

Thanks Murray - excellent catch!

Feels like this could all be a bit cleaner if I'd put the completer into MultiLineTextWidget instead of CodeWidget, but given where we are this seems like the most minimal fix available. The only weakness would seem to be if someone used activatedSignal().connectFront() to prevent our __activated method from being called, but I think that's probably unlikely enough for now?

Looks like we just need the obligatory Changes.md rebase before merging.

[murraystevenson]

Feels like this could all be a bit cleaner if I'd put the completer into MultiLineTextWidget instead of CodeWidget.

I think we've now said that sentence enough times to make it an inevitability...

The only weakness would seem to be if someone used activatedSignal().connectFront()... but I think that's probably unlikely enough for now?

Yeah, that does seem fairly unlikely, though we've tempted the fates by bringing it up...

The other thought I had while investigating this was whether it would also be better for PlugPopup to dismiss based on editingFinished rather than activated? I'd expect that would make the failure case less destructive but still annoying as the presence of a completion menu would instead block the popup from closing?

[johnhaddon]

whether it would also be better for PlugPopup to dismiss based on editingFinished rather than activated?

editingFinished is a much more vague signal, so i don't think it's suitable for dismissal. it still gets emitted for the loss of focus you get when popping up a (non-completer) menu for instance. or for an accidental loss of focus.

[murraystevenson]

editingFinished is a much more vague signal, so i don't think it's suitable for dismissal. it still gets emitted for the loss of focus you get when popping up a (non-completer) menu for instance. or for an accidental loss of focus.

Ah, very good point! Scratch that one then...