Created attachment 192098 [details] Screenshot showing the problem Patch [1] implemented the use of native Qt widgets for message dialogs. After this patch, if I run a macro with a MsgBox call, the dialog does not have the default "OK" button and it is not possible to close it. Steps to reproduce: 1) Open LO 24.8 using kf5 2) Run a macro that calls MsgBox 3) Execute the macro and notice that the MsgBox does not have the default "Ok" button and it cannot be closed If you set SAL_VCL_QT_NO_WELDED_WIDGETS=1 before running LO, MsgBox will work again. [1] https://gerrit.libreoffice.org/c/core/+/161073 System info: Version: 24.8.0.0.alpha0+ (X86_64) / LibreOffice Community Build ID: 7850a7deb59b890c73dfd52bd5aced6a538e6349 CPU threads: 16; OS: Linux 6.5; UI render: default; VCL: kf5 (cairo+xcb) Locale: pt-BR (pt_BR.UTF-8); UI: en-US Calc: CL threaded
Hi Michael, can you please take a look at this one?
Ran into this recently.
The problem is that the implementation of MsgBox in "SbRtl_MsgBox" (see methods.cxx) calls the method "add_button" that should exist in QtInstanceDialog.cxx, but it is still not implemented. Is there a plan to implement it? Code pointers: https://opengrok.libreoffice.org/xref/core/basic/source/runtime/methods.cxx?r=c4c017d1#4340 https://opengrok.libreoffice.org/xref/core/vcl/qt5/QtInstanceDialog.cxx?r=1ace8888#41
(In reply to Rafael Lima from comment #3) > The problem is that the implementation of MsgBox in "SbRtl_MsgBox" (see > methods.cxx) calls the method "add_button" that should exist in > QtInstanceDialog.cxx, but it is still not implemented. > > Is there a plan to implement it? I ran into a similar issue on Friday (dialog that shows when opening a file that's already open in another instance of LO doesn't have buttons) and have a local WIP branch that implements this, but needs a bit of cleanup. I hope I'll get to that sometime tomorrow.
(In reply to Michael Weghorn from comment #4) > I ran into a similar issue on Friday (dialog that shows when opening a file > that's already open in another instance of LO doesn't have buttons) and have > a local WIP branch that implements this, but needs a bit of cleanup. I hope > I'll get to that sometime tomorrow. Besides just showing a button that closes the dialog, that dialog has some additional functionality that needs to be implemented (For simplicity, I'll just add it here instead of creating a separate bug report.): * it has a custom title that's not set correctly * the different buttons result in different actions * it has an explicit default button set ("Open Read-Only") The suggested Gerrit change series up to https://gerrit.libreoffice.org/c/core/+/162441 implements what's needed to make that work for me.
(In reply to Michael Weghorn from comment #5) > The suggested Gerrit change series up to > https://gerrit.libreoffice.org/c/core/+/162441 implements what's needed to > make that work for me. Merged now, but I only noticed now that I had the wrong bug ID in the commit summary (tdf#154381), so the automatic commit comments wrongly ended up there... (In reply to Commit Notification from bug 154381 comment #11) > Michael Weghorn committed a patch related to this issue. > It has been pushed to "master": > > https://git.libreoffice.org/core/commit/ > d76d4df7ea1db0fdd27d350119ff7c0b2024b6e1 > > tdf#154381 qt weld: Implement QtInstanceWindow::{g,s}et_title > > It will be available in 24.8.0. > > The patch should be included in the daily builds available at > https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More > information about daily builds can be found at: > https://wiki.documentfoundation.org/Testing_Daily_Builds > > Affected users are encouraged to test the fix and report feedback. (In reply to Commit Notification from bug 154381 comment #12) > Michael Weghorn committed a patch related to this issue. > It has been pushed to "master": > > https://git.libreoffice.org/core/commit/ > bb0c0e422788b6c461387491c59911ad84c27b83 > > tdf#154381 qt weld: Add button handling for message box > > It will be available in 24.8.0. > > The patch should be included in the daily builds available at > https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More > information about daily builds can be found at: > https://wiki.documentfoundation.org/Testing_Daily_Builds > > Affected users are encouraged to test the fix and report feedback. (In reply to Commit Notification from bug 154381 comment #13) > Michael Weghorn committed a patch related to this issue. > It has been pushed to "master": > > https://git.libreoffice.org/core/commit/ > e44a322fd00e017ea6494d836f66e5ddf6cfa9d5 > > tdf#154381 qt weld: Allow setting message box default button > > It will be available in 24.8.0. > > The patch should be included in the daily builds available at > https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More > information about daily builds can be found at: > https://wiki.documentfoundation.org/Testing_Daily_Builds > > Affected users are encouraged to test the fix and report feedback.
Hi Michael, thank you for the fix! I did some testing and I got a few crashes and errors. Next are my test results: OK: Msgbox("Hello") OK: Msgbox("Hello", MB_OKCANCEL) OK: Msgbox("Hello", MB_ABORTRETRYIGNORE) CRASH: Msgbox("Hello", MB_YESNOCANCEL) CRASH: Msgbox("Hello", MB_YESNO) OK: Msgbox("Hello", MB_RETRYCANCEL) OK: Msgbox("Hello", MB_ICONSTOP) OK: Msgbox("Hello", MB_ICONQUESTION) OK: Msgbox("Hello", MB_ICONEXCLAMATION) OK: Msgbox("Hello", MB_ICONINFORMATION) FAIL: Msgbox("Hello", MB_OKCANCEL + MB_DEFBUTTON2) FAIL: Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON2) OK: Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON3) The 2 fails with MB_DEFBUTTON2 occurred because the expected button was not selected. Each case below should select a different button. Msgbox("Hello", MB_ABORTRETRYIGNORE) Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON2) Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON3)
Hi Rafael, thanks a lot for testing and the detailed description! the previous changes were assuming that return values would be from the VclResponseType enum, but it turns out that any arbitrary int can be used. (In reply to Rafael Lima from comment #7) > I did some testing and I got a few crashes and errors. Next are my test > results: > > OK: Msgbox("Hello") > OK: Msgbox("Hello", MB_OKCANCEL) > OK: Msgbox("Hello", MB_ABORTRETRYIGNORE) > CRASH: Msgbox("Hello", MB_YESNOCANCEL) > CRASH: Msgbox("Hello", MB_YESNO) Suggested changes to fix the crashes you describe: https://gerrit.libreoffice.org/c/core/+/162559 https://gerrit.libreoffice.org/c/core/+/162560 > FAIL: Msgbox("Hello", MB_OKCANCEL + MB_DEFBUTTON2) > FAIL: Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON2) > OK: Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON3) > > The 2 fails with MB_DEFBUTTON2 occurred because the expected button was not > selected. Each case below should select a different button. > > Msgbox("Hello", MB_ABORTRETRYIGNORE) > Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON2) > Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON3) I'm not sure about these, can you be more specific what button is selected and which one you would expect? In a quick test of mine, the selected button was the same with or without `SAL_VCL_QT_NO_WELDED_WIDGETS=1` set. However, the button order may be different (e.g. the "Ignore" button was selected in both cases, but it might have been the second or third button visually).
(In reply to Michael Weghorn from comment #8) > I'm not sure about these, can you be more specific what button is selected > and which one you would expect? It may be an issue that existed before your patch. Using kf5 in LO 7.6.4, I get these results: Msgbox("Hello", MB_ABORTRETRYIGNORE) ' Retry is selected Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON2) ' Retry is selected (should be Ignore) Msgbox("Hello", MB_ABORTRETRYIGNORE + MB_DEFBUTTON3) ' Ignore is selected (should be Cancel) The code above works fine in gtk3.
(In reply to Michael Weghorn from comment #8) > https://gerrit.libreoffice.org/c/core/+/162559 > https://gerrit.libreoffice.org/c/core/+/162560 With these patches I get the correct buttons, as in gtk3. And no more crashes. I also tested the return values in all cases and they work as well. Thanks again for the fixes.
BTW as for the button orders, they are different when using native Qt and the previous implementation. Previously we got: Retry, Ignore, Abort And with Qt we now get: Abort, Retry, Ignore (which is the same order as in gtk3; this seems better to me) The use of MB_DEFBUTTON2 and MB_DEFBUTTON3 now actually selects the second and third buttons. One last issue: I can only close the MsgBox using the X (close button) when the dialog has the OK button only. If you run the code below and try to close it with the X button, it won't close. Msgbox("Hello", MB_ABORTRETRYIGNORE)
(In reply to Rafael Lima from comment #11) > BTW as for the button orders, they are different when using native Qt and > the previous implementation. > > Previously we got: Retry, Ignore, Abort > > And with Qt we now get: Abort, Retry, Ignore (which is the same order as in > gtk3; this seems better to me) I didn't look into it further, but `SalInstanceDialog::run` has this which may be related: VclButtonBox* pActionArea = m_xDialog->get_action_area(); if (pActionArea) sort_native_button_order(*pActionArea); > The use of MB_DEFBUTTON2 and MB_DEFBUTTON3 now actually selects the second > and third buttons. Nice. > One last issue: > > I can only close the MsgBox using the X (close button) when the dialog has > the OK button only. > > If you run the code below and try to close it with the X button, it won't > close. > > Msgbox("Hello", MB_ABORTRETRYIGNORE) From a first look into the QMessageBox source code, this seems related to the concept of an "escape button": https://doc.qt.io/qt-6/qmessagebox.html#escapeButton If there's none (either set explicitly or determined implicitly, e.g. if there's only a single button or one with a suitable role), closing the dialog using the Esc key or the "X" in the window bar doesn't work. Explicitly setting one or using different roles for buttons might be a solution here. I'll think about it some more. Do you happen to know whether there are any message boxes where closing without hitting an actual button would actually be undesirable, e.g. to force the user to make an explicit choice?
(In reply to Michael Weghorn from comment #12) > Do you happen to know whether there are any message boxes where closing > without hitting an actual button would actually be undesirable, e.g. to > force the user to make an explicit choice? I can't think of any... also, in gtk3 message boxes don't even have window decorations (if you use them in Gnome), nor close and minimize buttons. So I guess we can keep it this way.
(In reply to Rafael Lima from comment #13) > I can't think of any... also, in gtk3 message boxes don't even have window > decorations (if you use them in Gnome), nor close and minimize buttons. > > So I guess we can keep it this way. Thanks for the input. OK, let's keep it as is for now then, we can still reconsider later. One approach then might be to set a button with return value VclResponseType::RET_CANCEL as "escape button" if it gets added in QtInstanceMessageDialog::add_button, s. diff below. This way, RET_CANCEL would be returned if the dialog gets cancelled that way, which seems to match what SalInstanceDialog does. However, the issue that the return codes are not clearly defined and the macro MsgDialog uses custom return codes would remain and there's no button with return value 0 (= RET_CANCEL) for your example, so that still wouldn't work for that particular case. Setting "any arbitrary" button as escape button is something I'd rather not do, since that might result in something unexpectedly being applied,... when at least I as a user would expect that closing the dialog this way wouldn't do that. If there are no further comments from you or Omkar, I'll merge the current version of the patches (PS 2, which is PS 1 + making clang plugins happy). diff --git a/vcl/qt5/QtInstanceMessageDialog.cxx b/vcl/qt5/QtInstanceMessageDialog.cxx index 7e505aff7cd0..9b6eb66a705b 100644 --- a/vcl/qt5/QtInstanceMessageDialog.cxx +++ b/vcl/qt5/QtInstanceMessageDialog.cxx @@ -53,6 +53,11 @@ void QtInstanceMessageDialog::add_button(const OUString& rText, int nResponse, c QPushButton* pButton = m_pMessageDialog->addButton(vclToQtStringWithAccelerator(rText), QMessageBox::ButtonRole::ActionRole); pButton->setProperty(PROPERTY_VCL_RESPONSE_CODE, QVariant::fromValue(nResponse)); + + // if an escape button is set, the message box can be closed via the Esc key or the + // "X" in the dialog's window bar and the escape button is considered as activated + if (nResponse == RET_CANCEL) + m_pMessageDialog->setEscapeButton(pButton); }
Michael Weghorn committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/c038b4fd162abb780ab0f9bcafb1421c5ec5786e tdf#159323 qt weld: Set standard buttons via weld API It will be available in 24.8.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Michael Weghorn committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/053b88c371461cda49e99ab2fc060eb1f1ded580 tdf#159323 qt weld: Remember VCL response code, don't try to map It will be available in 24.8.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
(In reply to Michael Weghorn from comment #14) > If there are no further comments from you or Omkar, I'll merge the current > version of the patches (PS 2, which is PS 1 + making clang plugins happy). Merged now.