Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash on closing iOS chat dialog #3413

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Fix crash on closing iOS chat dialog #3413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions src/chatdlg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() ) );
Copy link
Collaborator

@pljones pljones Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@pljones pljones Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

QAction* closeAction = pMenu->addAction ( tr ( "&Close" ) );
pljones marked this conversation as resolved.
Show resolved Hide resolved
#endif

#if defined( ANDROID ) || defined( Q_OS_ANDROID )
pEditMenu->addAction ( tr ( "&Close" ), this, SLOT ( close() ), QKeySequence ( Qt::CTRL + Qt::Key_W ) );
pEditMenu->addAction ( tr ( "&Close" ), this, SLOT ( OnCloseClicked() ), QKeySequence ( Qt::CTRL + Qt::Key_W ) );
#endif

// Now tell the layout about the menu
Expand All @@ -78,6 +77,10 @@ CChatDlg::CChatDlg ( QWidget* parent ) : CBaseDlg ( parent, Qt::Window ) // use
QObject::connect ( butSend, &QPushButton::clicked, this, &CChatDlg::OnSendText );

QObject::connect ( txvChatWindow, &QTextBrowser::anchorClicked, this, &CChatDlg::OnAnchorClicked );

#if defined( Q_OS_IOS )
QObject::connect ( closeAction, &QAction::triggered, this, &CChatDlg::OnCloseClicked );
#endif
}

void CChatDlg::OnLocalInputTextTextChanged ( const QString& strNewText )
Expand Down Expand Up @@ -149,3 +152,18 @@ void CChatDlg::OnAnchorClicked ( const QUrl& Url )
}
}
}

#if defined( Q_OS_IOS ) || defined( ANDROID ) || defined( Q_OS_ANDROID )
void CChatDlg::OnCloseClicked()
{
// on mobile add a close button or menu entry
# if defined( Q_OS_IOS )
// On Qt6, iOS crashes if we call close() due to unknown reasons, therefore we just hide() the dialog. A Qt bug is suspected.
// Checkout https://github.com/jamulussoftware/jamulus/pull/3413
hide();
# endif
# if defined( ANDROID ) || defined( Q_OS_ANDROID )
close();
# endif
}
#endif
3 changes: 3 additions & 0 deletions src/chatdlg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ public slots:
void OnLocalInputTextTextChanged ( const QString& strNewText );
void OnClearChatHistory();
void OnAnchorClicked ( const QUrl& Url );
#if defined( Q_OS_IOS ) || defined( ANDROID ) || defined( Q_OS_ANDROID )
void OnCloseClicked();
#endif

signals:
void NewLocalInputText ( QString strNewText );
Expand Down
Loading