Opened 12 years ago

Closed 12 years ago

#4132 closed patch (fixed)

In some menu's ESCAPE action is not handled appropriately

Reported by: danielk Owned by: danielk
Priority: minor Milestone: 0.21
Component: mythtv Version: head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

In a number of MythPopupBox::Show...() dialogs pressing escape will act like pressing the first button rather than being processed as a Rejected action.

This is due to a previously applied changeset [14478], which was supposed to fix this type of problem for other MythPopupBox? based dialogs. It changed the reject action that MythDialog? inherited dialogs used from -1 to 0, to match the reject action of QDialog. Before this change sometimes the QDialog reject of 0 was returned, and sometimes the MythTV reject value of -1 was returned which caused this type of aliasing for other dialogs. Unfortunately I missed a whole set of dialogs using the defaultButtonPressedHandler() in that changeset.

Attachments (13)

4132-v1.patch (43.4 KB) - added by danielk 12 years ago.
Addresses show2ButtonPopup() and showButtonPopup() dialogs, I'm still looking at custom and other dialogs using addButton() with the default handler.
4132-v2.patch (78.8 KB) - added by danielk 12 years ago.
Addresses the custom popups using ExecPopup?() in addition to the popups addressed in the last patch
4132-dbg-v3.patch (147.2 KB) - added by danielk 12 years ago.
Work in progress patch... This temporarily changes the return code of the Dialog exec functions to a new type "DialogCode?" to make compiler help us find problem areas.
4132-dbg-v4.patch (142.1 KB) - added by danielk 12 years ago.
Updated patch
4132-dbg-v5.patch (144.2 KB) - added by danielk 12 years ago.
Updated patch
4132-dbg-v6.patch (146.6 KB) - added by danielk 12 years ago.
Updated patch, fixes a regression in versions 4 & 5 in patch where MythDialog::Accepted had the incorrect DialogCode? assigned. Also all "TODO checkme" sections of code have been addressed.
4132-dbg-v7.patch (216.2 KB) - added by danielk 12 years ago.
Updated patch, this fixes some problems with popping up the virtual keyboard for editing text fields and with improper QObject deletion causing segfaults
4132-dbg-v8.patch (168.6 KB) - added by danielk 12 years ago.
Updated patch, no fixes this just applies to the latest svn
4132-dbg-v9.patch (169.8 KB) - added by danielk 12 years ago.
Updated patch, fixes search dialog focus problem
4132-v10.patch (101.5 KB) - added by danielk 12 years ago.
Updated patch, fixes some virtualkeyboard problems.
4132-v11.patch (168.4 KB) - added by danielk 12 years ago.
Updated patch (needed after [14888])
4132-v12.patch (176.4 KB) - added by danielk 12 years ago.
Updated patch, addresses MythControls? issues
4132-v13.patch (164.0 KB) - added by danielk 12 years ago.
Updated patch, needed after [14895] & [14896].

Download all attachments as: .zip

Change History (31)

Changed 12 years ago by danielk

Attachment: 4132-v1.patch added

Addresses show2ButtonPopup() and showButtonPopup() dialogs, I'm still looking at custom and other dialogs using addButton() with the default handler.

Changed 12 years ago by danielk

Attachment: 4132-v2.patch added

Addresses the custom popups using ExecPopup?() in addition to the popups addressed in the last patch

comment:1 Changed 12 years ago by paulh

I've just given the v2 patch a quick test.

The two test cases I knew about are still broken with this patch all be it in slightly different ways :-).

One is the search dialog in MythMusic ticket #4124. When you press escape you are still presented with the 'Update Playlist Options' dialog when you shouldn't. See PlaybackBoxMusic::showSearchDialog()

Also in the MythPopupBox::showGetTextPopup() dialog escape now works as expected but the Cancel button doesn't that's because there is no SLOT called cancel() I guess that should be reject(). I noticed this in MythGallery when using the rename option.

Doesn't changing the return values from MythPopupBox? have a lot of potential to break things? How confident are you that you have found all places where it could break things? Also what about all the unofficial plugins this may break?

Changed 12 years ago by danielk

Attachment: 4132-dbg-v3.patch added

Work in progress patch... This temporarily changes the return code of the Dialog exec functions to a new type "DialogCode?" to make compiler help us find problem areas.

comment:2 Changed 12 years ago by danielk

paulh, the problem is that even prior to [14478] about 2/3 of the places the dialogs are used we expect a 0 on reject. This is the Qt reject value, and depending on how you create the dialog you get different reject values. A lot of places are coded for the wrong return value (-1 or 0) on the escape key. Some places just segfault if you hit escape to get rid of the menu, because they don't check the range of the return and use it as an index into a list or an array.

In the last patch I used the C++ typing system to find all uses of the exec() return value. It helped me find several segfault cases and found the broken examples (except for the cancel/reject SLOT problem). In the next patch I'll remove the "DialogCode?" class and remove the unnecessary changes it forced. Then I'll test every different type of dialog and all test all the "TODO checkme" points I marked in the code this time around.

comment:3 Changed 12 years ago by paulh

Ah clever that should at least find everywhere it's used :-)

I don't have time to test this right now but if you want me to test the final patch before you apply it I'll see what I can do.

comment:4 Changed 12 years ago by danielk

Thanks. I've been doing testing in my spare time this week, there are a lot of dialogs in MythTV! By the end of the weekend I should be confident enough to have a final patch.

comment:5 Changed 12 years ago by danielk

(In [14831]) Refs #4132. Fixes two bugs in the playbackbox recording group password dialog handling.

In noticed this when testing dialogs for #4132. First initial focus was being set, this adds a little code that puts the focus on the old password entry if a password has been set, and on the new password entry otherwise. Second while setRecGroupPassword() did set the password in the db it did not update the recGroupPassword variable so if you pulled up the dialog again in the same session the new password you had just entered would not work. Both these problems are fixed in this changeset.

Changed 12 years ago by danielk

Attachment: 4132-dbg-v4.patch added

Updated patch

Changed 12 years ago by danielk

Attachment: 4132-dbg-v5.patch added

Updated patch

Changed 12 years ago by danielk

Attachment: 4132-dbg-v6.patch added

Updated patch, fixes a regression in versions 4 & 5 in patch where MythDialog::Accepted had the incorrect DialogCode? assigned. Also all "TODO checkme" sections of code have been addressed.

Changed 12 years ago by danielk

Attachment: 4132-dbg-v7.patch added

Updated patch, this fixes some problems with popping up the virtual keyboard for editing text fields and with improper QObject deletion causing segfaults

comment:6 Changed 12 years ago by danielk

Paul, can you give the latest patch a good workout?

The only regression I'm experiencing is not being able to exit the Video Manager portion of the MythVideo? plugin. I'll be looking at this tomorrow.

comment:7 Changed 12 years ago by danielk

(In [14874]) Refs #4115. Refs #4132. Changes explicit deletes of three dialogs to deleteLater() calls.

Like [14862] which addressed this problem for MythDialog?, this addresses the delete problem for three children of MythDialog?, MythProgressDialog?, MythBusyDialog?, and DialogBox?. As explained in the comments on [14858] and [14862] deleting these dialogs explicitly can allow signals to arrive for the dialog after it has been deleted causing segfaults. This is hard to do when using a keyboard and mouse to provide input, but much easier to trigger when using network control to send key commands to the UI.

This only fixes 3 dialogs and the change appears much larger than it in fact is. I'm not doing all the dialogs at once so that I can roll back a just a few at a time if there are any problems. If you have an outside plugin that fails to compile after this changeset, you probably just need to change some 'delete dialogname;' expressions to 'dialogname->deleteLater();'.

This does change the myth binary revision so plugins will have to be recompiled.

Changed 12 years ago by danielk

Attachment: 4132-dbg-v8.patch added

Updated patch, no fixes this just applies to the latest svn

comment:8 in reply to:  6 Changed 12 years ago by paulh

Replying to danielk:

Paul, can you give the latest patch a good workout?

The only regression I'm experiencing is not being able to exit the Video Manager portion of the MythVideo? plugin. I'll be looking at this tomorrow.

Sure may take a while there's a _lot_ of dialogs!

It looks like the plugins at least are going to need a make clean/distclean after this patch. I tried to save a little time and tried to just recompile but a few plugins, MythZoneMinder? and MythNews? I think, failed to initialize. A make clean fixed it.

So far I've gone though these plugins:

MythZoneMonder? - no problems found.

MythNews? - no problems found.

MythArchive? - There's a reproduceable hang in the Import Video Files screens involving the SearchDialog?. It may be easier for you to reproduce it in MythMusic the same thing happens there as well.

  1. Got to the music playback screen.
  2. Press 'I' to edit the tracks metadata.
  3. Press one of the small buttons at the end of the edits to show the search dialog.
  4. Press Escape to exit the dialog.

Results in a complete hang for me. Looks like the editor screen never gets the focus back.

That's enough for one day I'll try some more tomorrow :-)

Changed 12 years ago by danielk

Attachment: 4132-dbg-v9.patch added

Updated patch, fixes search dialog focus problem

comment:9 Changed 12 years ago by danielk

I've attached a patch that fixes the bug, at least in MythMusic.

MythSearchDialog::deleteLater(void) wasn't calling MythPopupBox::deleteLater(). So the dialog wasn't getting deleted, it was just hidden from view.

BTW Exiting from VideoManager? is working for me now...

comment:10 Changed 12 years ago by paulh

MythArchive? is now working correctly with the v9 patch.

No problems with MythBrowser?. I did notice a possible unsafe delete of the VirtualKeyboard? in tabview.cpp line 613? Not sure it's ever likely to be a problem because TTBOMK MythBrowser? cannot be used with the network interface anyway.

MythWeather? is currently unusable for me due to the bug described in comment 6 in #3921 so was unable to test. I use my own custom version of MythWeather? anyway :-)

MythMovies? - no problems found.

MythMusic - no problems found so far still a few things to test though.

comment:11 Changed 12 years ago by danielk

I didn't try to fix all the improper deletes of QObject derived classes on purpose, but I'll make a patch which fixes virtualkeyboard use. I was concentrating only on the ones that were causing segfaults when I was testing. There are over 90 classes derived from either MythDialog? or MythThemedDialog?, and making sure all of them behaved properly wrt to delete would have greatly expanded the size of the changeset; which is already on the big side.

MythWeather? is also doesn't work for me, but I did fix a segfault which I encountered trying to setup the screens. I'll see if fixing #3921 allows me to set it up so it works and we can test the dialogs...

Changed 12 years ago by danielk

Attachment: 4132-v10.patch added

Updated patch, fixes some virtualkeyboard problems.

comment:12 Changed 12 years ago by danielk

(In [14888]) Refs #4132. Fixes some miscellaneous problems with MythWeather?.

There were three problems which made it impossible to test MythWeather? dialogs.

  • When sources are not defined (see #3921) the text of the error dialog is not visible.
  • doListSelect() used an unchecked return value from showButtonPopup as an index into an array.
  • connectScreen()/disconnectScreen() segfault when sources are not properly configured.

This fixes these problems so we can test the dialogs.

Changed 12 years ago by danielk

Attachment: 4132-v11.patch added

Updated patch (needed after [14888])

comment:13 Changed 12 years ago by paulh

There a small problem in MythControls? when you try to bind a key to an action you get this in the log:-

QObject::connect: No such slot KeyGrabPopupBox::Cancel() QObject::connect: (sender name: 'keygrabber') QObject::connect: (receiver name: 'keygrabber')

comment:14 Changed 12 years ago by paulh

Not sure if this is a new bug caused by this patch or not.

In MythControls? if you try to bind a key to an action and the key is already being used in another context you get the 'Conflicting Binding' dialog with no buttons on it. If you then press Escape you get the segfault.

Changed 12 years ago by danielk

Attachment: 4132-v12.patch added

Updated patch, addresses MythControls? issues

comment:15 Changed 12 years ago by danielk

Paul, I've attached a new patch that fixes the MythControls? issues...

comment:16 Changed 12 years ago by danielk

(In [14895]) Refs #4132. Changes to standard dialogs for MythControls? menus.

There were some usage problems noticed with the dialogs when testing #4132. This just switches those dialogs to using the standard dialogs where appropriate to avoid any strange user interaction problems. KeyGrabPopupBox? is still a custom dialog since it needs to be able to capture any key, including escape.

comment:17 Changed 12 years ago by danielk

(In [14896]) Refs #4132. Fixes copy-n-paste error in [14895].

Changed 12 years ago by danielk

Attachment: 4132-v13.patch added

Updated patch, needed after [14895] & [14896].

comment:18 Changed 12 years ago by danielk

Resolution: fixed
Status: newclosed

(In [14908]) Fixes #4132. Escape action was not handled correctly in a number of dialogs after [14478].

This creates a DialogCode? enum and uses it for the return value of the MythTV dialogs in lib/libmyth. This allows us to fix an inconstency in reject() handling that exists due to Qt using a reject value of 0 while we have used both -1 and 0 in the past.

While -1 is more like how C handles error returns from functions the inconsitency lead to many errors the Qt object were inherited publicly so child dialogs would sometimes use those exec() functions and slots. From looking at the code of many people in MythTV this has lead to many ways to solve the problem; many with signals and slots that should not be needed. All of the solutions use one of three methods to check returns: hard coded ints, QDialog::Accepted/QDialog::Rejected, or after [14478] MythDialog::Accepted/MythDialog::Rejected. In [14478], I tried to keep the affected code to a minimum to avoid needing to test dialogs unrelated to the ones I was fixing, but when I tried to fix the escape problem using this policy it just didn't work. In this change I've instead changed all hard coded ints to check against the enumerated DialogCodes? and those dialog codes are consistent with the existing QDialog return code enum and the MythDialog? return code enum introduced in [14478]; during testing I temporarily changed DialogCode? to a class so as to catch all the implicit casts of this value to int.

Obviously, I've done a lot of testing in the last two weeks of the mythfrontend, mythtv-setup and plugins; and Paul has also tested most of the plugins. We needed to fix some bugs along the way as referenced in the ticket in order to test the dialogs in some of the plugins that were broken. But there are a large number of dialogs in MythTV and they have not all been tested, if you wind any strange behaviour please note it on this ticket or create a new ticket and I'll attend to the problem.

This change requires mythtv and its plugins to be recompiled.

Note: See TracTickets for help on using tickets.