Fix crash on closing iOS chat dialog #3413
Conversation
| @@ -62,7 +62,7 @@ CChatDlg::CChatDlg ( QWidget* parent ) : CBaseDlg ( parent, Qt::Window ) // use | |||
| pMenu->addMenu ( pEditMenu ); | |||
| #if defined( Q_OS_IOS ) | |||
| QAction* action = pMenu->addAction ( tr ( "&Close" ) ); | |||
| connect ( action, SIGNAL ( triggered() ), this, SLOT ( close() ) ); | |||
There was a problem hiding this comment.
Whilst we're doing this, I would like the two lines changed to match the addActions before and after that both include the action handler.
I'll also ask why this should be hide() for iOS and close() for Android -- I'm going to hazard a guess it should be the same request to Qt on both platforms and that Qt should do the right thing.
I'd even go as far as to suggest that having &Close on the menu bar in the app, regardless of platform, wouldn't necessarily be a bad thing.
There was a problem hiding this comment.
As far as I understand hide() is close() minus deleting the dialog.
Somehow on iOS under Qt6 the dialog already gets deleted and close() may attempt to do it twice.
Potentially it's the same on android with Qt6 - but we don't know for sure yet.
Yes, we could add the close menu entry on all OS - on desktop it seems redundant however.
There was a problem hiding this comment.
I'd argue that UI consistency across platforms isn't "redundant" but makes for a more consistent user experience -- with the benefit of reducing code complexity and maintenance overhead.
There was a problem hiding this comment.
The close entry on desktop OS is redundant since there's a close button in the window manager.
It does not make sense to hide the close button in a menu on mobile OS (at least iOS, where there's no back button).
ann0see
force-pushed
the
iOS/FixCrashChat
branch
from
dd375d9
to
0a5e617
Compare
|
@softins I've pushed a new build with this + the icon PR on TestFlight. |
|
Thanks for the updated Testflight. I've tried it out, and can confirm it doesn't crash, and the icon looks fine. But I have found some oddities regarding the Chat window, which while fairly minor, would be worth investigating. When opening the Chat window, it does not cover the main menu bar, but instead adds its own menu bar below it:
Closing and re-opening it makes no difference. This is in contrast to the Settings window, which replaces the main menu bar with its own, and the Connect window, which doesn't have a menu bar at all (it is exited with either Cancel or Connect). Having connected to a server with a welcome message, the Chat window automatically displayed as usual, but still with the main menu bar showing:
I closed it, disconnected and connected again to check, and it was the same. I then tapped in the text box to type a message, and the on-screen keyboard appeared, correctly moving the window upwards. After typing the message and pressing Send, the keyboard disappeared again as normal, but now the Chat window was correctly showing without the main menu bar:
The very first time I tried this Testflight, and also when I was trying the previous one, the Chat window showed with the message entry box and Send button half off the bottom of the screen. But I have failed to reproduce this behaviour since, so maybe it was a hangover from the older version? |
|
Also, at one point after this, I tapped on Connect in the main window, and the Connect dialog showed at the wrong size:
Tipping the iPad to rotate to portrait mode and back to landscape mode reset the dialog size to full screen correctly. I'm not sure what I did to provoke that behaviour. |
|
I did notice problems like this on the main mixer board too. All can be fixed by rotating the device. I believe it's due to an overflow introduced by some widget (see the UI issues mentioned in #3406) |
|
Maybe #3397 |
Fair enough. As this PR fixes the crash it's supposed to, I'm happy to approve and leave the UI for separate investigation. |
Using close() would crash due to a null pointer dereference
ann0see
force-pushed
the
iOS/FixCrashChat
branch
from
0a5e617
to
9cf708c
Compare
|
I've no way to test it. |
| @@ -61,12 +61,11 @@ CChatDlg::CChatDlg ( QWidget* parent ) : CBaseDlg ( parent, Qt::Window ) // use | |||
|
|
|||
| pMenu->addMenu ( pEditMenu ); | |||
| #if defined( Q_OS_IOS ) | |||
| QAction* action = pMenu->addAction ( tr ( "&Close" ) ); | |||
| connect ( action, SIGNAL ( triggered() ), this, SLOT ( close() ) ); | |||
| QAction* closeAction = pMenu->addAction ( tr ( "&Close" ) ); | |||
There was a problem hiding this comment.
Why is this added to the QMenuBar rather than the edit menu, like for Android? Have we checked if that's the source of the problem?
QMenuBar* pMenu = new QMenuBar ( this );
|
I'm unable to test on iOS either. |
Short description of changes
Using close() instead of hide() would crash the app as soon as one taps at the screen after closing the chat dialog. Due to a null pointer dereference (?).
I do not understand why the app crashes. Probably it's a qt bug.
https://qtcentre.org/threads/5572-QCoreApplication-postEvent-Unexpected-null-receiver (?)
CHANGELOG: iOS: Fix crash on Qt6 after closing the chat window.
Context: Fixes an issue?
Related to: #3406
Does this change need documentation? What needs to be documented and how?
Status of this Pull Request
Ready for review.
What is missing until this pull request can be merged?
Discussion. This is a hot fix...
Checklist