OSXMessageBox bug (in async mode)

MacOSX and iOS
1

Hi Juce team!

Something just came up as a result of your “modal” changes in develop.

class OSXMessageBox  : private AsyncUpdater
...
    const char* button1;
    const char* button2;
    const char* button3;

That is very likely to misbehave in the async case (in fact, I’ve just seen it fail while testing!).

You need to change those 3 values to (say) juce::String to capture the arguments which are passed-in, which might well not be const char*, but actually computed (temporary) values (e.g. from juce::String contents).

The fix becomes:

class OSXMessageBox  : private AsyncUpdater
{
public:
    OSXMessageBox (AlertWindow::AlertIconType type, const String& t, const String& m,
                   const String& b1, const String& b2, const String& b3,
                   ModalComponentManager::Callback* c, const bool runAsync)
        : iconType (type), title (t), message (m), callback (c),
          button1 (b1), button2 (b2), button3 (b3)
    {
        if (runAsync)
            triggerAsyncUpdate();
    }

    int getResult() const
    {
        switch (getRawResult())
        {
            case NSAlertFirstButtonReturn:  return 1;
            case NSAlertThirdButtonReturn:  return 2;
            default:                        return 0;
        }
    }

    static int show (AlertWindow::AlertIconType iconType, const String& title, const String& message,
                     ModalComponentManager::Callback* callback, const String& b1, const String& b2, const String& b3,
                     bool runAsync)
    {
        std::unique_ptr<OSXMessageBox> mb (new OSXMessageBox (iconType, title, message, b1, b2, b3,
                                                              callback, runAsync));
        if (! runAsync)
            return mb->getResult();

        mb.release();
        return 0;
    }

private:
    AlertWindow::AlertIconType iconType;
    String title, message;
    std::unique_ptr<ModalComponentManager::Callback> callback;

  // MPC fix - begin - was const char*
    String button1;
    String button2;
    String button3;
  // MPC fix - end - was const char*

    void handleAsyncUpdate() override
    {
        auto result = getResult();

        if (callback != nullptr)
            callback->modalStateFinished (result);

        delete this;
    }

    NSInteger getRawResult() const
    {
        NSAlert* alert = [[[NSAlert alloc] init] autorelease];

        [alert setMessageText:     juceStringToNS (title)];
        [alert setInformativeText: juceStringToNS (message)];

        [alert setAlertStyle: iconType == AlertWindow::WarningIcon ? NSAlertStyleCritical
                                                                   : NSAlertStyleInformational];
      
        addButton (alert, button1);
        addButton (alert, button2);
        addButton (alert, button3);

        return [alert runModal];
    }

    static void addButton (NSAlert* alert, const String& button)
    {
        if (button != "")
            [alert addButtonWithTitle: juceStringToNS (TRANS (button))];
    }
}

You also need to change these:

    OSXMessageBox::show (iconType, title, message, callback, "OK", nullptr, nullptr, true);
...
   OSXMessageBox::show (iconType, title, message, callback,
                                "OK", "Cancel", nullptr, callback != nullptr) == 1;
...
   OSXMessageBox::show (iconType, title, message, callback,
                                "Yes", "No", nullptr, callback != nullptr);

to these:

    OSXMessageBox::show (iconType, title, message, callback, "OK", "", "", true);
...
  OSXMessageBox::show (iconType, title, message, callback,
                                "OK", "Cancel", "", callback != nullptr) == 1;
...
    OSXMessageBox::show (iconType, title, message, callback,
                                "Yes", "No", "", callback != nullptr);

Hoping that helps,

Pete

2

Are you using OSXMessageBox in your own code? It’s an internal class used only by the NativeMessageBox on macOS and in all of the methods we’re using string literals which have static storage duration so won’t ever dangle.

3

Hi @ed95,

Aha, thanks - of course. That explains it.

To explain - I’ve long ago changed the interface to NativeMessageBox::showYesNoCancelBox and OSXMessageBox, to work properly (using the supplied strings…) on macOS.

I did this so long ago, I forgot - and now see of course I should replace my use of const char* (ahem) with juce::String references.

int JUCE_CALLTYPE NativeMessageBox::showYesNoCancelBox (AlertWindow::AlertIconType iconType,
                                                        const String& title, const String& message,
                                                        Component* /*associatedComponent*/,
                                                        // MARK: MPC - begin
                                                        ModalComponentManager::Callback* callback,
                                                        const String& buttonYesText,
                                                        const String& buttonNoText ,
                                                        const String& cancelText
                                                        )
                                                        // MARK: MPC - end       

{
    // MARK: MPC - begin
    return OSXMessageBox::show (iconType, title, message, callback,
                                buttonYesText.toUTF8().getAddress(), cancelText.toUTF8().getAddress(), buttonNoText.toUTF8().getAddress(), callback != nullptr);
    // MARK: MPC - end
}

etc.

I’ve also changed for example on Windows:

int JUCE_CALLTYPE NativeMessageBox::showYesNoCancelBox (AlertWindow::AlertIconType iconType,
                                                        const String& title, const String& message,
                                                        Component* associatedComponent,
                                                        // MARK: MPC - begin
                                                        ModalComponentManager::Callback* callback,
                                                        const String&, //  buttonYesText,
                                                        const String&, //  buttonNoText ,
                                                        const String&  // cancelText
                                                        )
                                                        // MARK: MPC - end       

{
    std::unique_ptr<WindowsMessageBox> mb (new WindowsMessageBox (iconType, title, message, associatedComponent,
                                                                  MB_YESNOCANCEL, callback, callback != nullptr));
    if (callback == nullptr)
        return mb->getResult();

    mb.release();
    return 0;
}

… except the text values aren’t used in the Windows versions yet in my version along these lines, something I keep meaning to achieve using this style of replacement for WindowsMessageBox and MessageBox

CTaskDialog taskDialog(L"", L"Blah", L"Blah Blah", 0, 0);
taskDialog.AddCommandControl(100, L"Something");
taskDialog.AddCommandControl(102, L"Else"); 
taskDialog.SetDefaultCommandControl(100);
INT_PTR x = taskDialog.DoModal();

So, sorry about the confusion. But this reflects ultimately that the native versions of NativeMessageBox::showYesNoCancelBox aren’t passed the expected strings from AlertWindow::showYesNoCancelBox

int AlertWindow::showYesNoCancelBox (AlertIconType iconType,
                                     const String& title,
                                     const String& message,
                                     const String& button1Text,
                                     const String& button2Text,
                                     const String& button3Text,
                                     Component* associatedComponent,
                                     ModalComponentManager::Callback* callback)
{
    if (LookAndFeel::getDefaultLookAndFeel().isUsingNativeAlertWindows())
        // MARK: MPC - begin
        return NativeMessageBox::showYesNoCancelBox (iconType, title, message, associatedComponent, callback, button1Text, button2Text, button3Text);
        // MARK: MPC - end

    AlertWindowInfo info (title, message, associatedComponent, iconType, 3, callback, callback == nullptr);
    info.button1 = button1Text.isEmpty() ? TRANS("Yes")     : button1Text;
    info.button2 = button2Text.isEmpty() ? TRANS("No")      : button2Text;
    info.button3 = button3Text.isEmpty() ? TRANS("Cancel")  : button3Text;

    return info.invoke();
}

Best wishes,

Pete

4

Yes that might be a nice feature, it’d require a fair bit of modification to get all of the other native message boxes in line though.

5

This commit adds support for passing the button strings to the native message boxes and also adds support for the newer Task Dialog on Windows (on Vista and above where available).

3 Likes
6

Good to see this fixed!
Apparently we’ve had it fixed a while ago on our branch but it appears that we forgot to report it (just discovered this now when handling the merge conflicts)

7

Great work - thanks, @ed95 !

Best wishes, Pete

8

Is it just me or the implementation of AlertWindow::showNativeDialogBox() has been wiped from juce_AlertWindow.cpp without providing a replacement?

It looks like an oversight because in the same commit the declaration in the .h is marked as deprecated (but not removed altogether), and in fact building with the latest develop I’m getting a linker error (I’m on Windows)

9

Thanks for reporting. That was an unintentional oversight and I’ve added the implementation back in here:

2 Likes
10

Hi @ed95,

I might be going bonkers, but when I call this on macOS:

AlertWindow::showYesNoCancelBox

in sync mode … the result codes are no longer as documented!

                 - 0 if the third button was pressed (normally used for 'cancel')
                 - 1 if the first button was pressed (normally used for 'yes')
                 - 2 if the middle button was pressed (normally used for 'no')

i.e. the values 0/1/2 are returned, but they don’t correspond to the documented buttons.

Just doing some more digging…

Pete

11

Hi @ed95

Here is my suggested fix:

    int getResult() const
    {
        switch (getRawResult())
        {
            // return 0 if the third button was pressed (normally used for 'cancel')
            // return 1 if the first button was pressed (normally used for 'yes')
            // return 2 if the middle button was pressed (normally used for 'no')

            case NSAlertFirstButtonReturn:   return 1;
            case NSAlertSecondButtonReturn:  return 2;
            case NSAlertThirdButtonReturn:   return 0;
          
            default:                         break;
        }

        jassertfalse;
        return 0;
    }

Hoping this helps!

Pete

12

@ed95

Similar fix required on Windows!

    int getResult() override
    {
        TASKDIALOGCONFIG config = { 0 };

        config.cbSize         = sizeof (config);
        config.pszWindowTitle = title.toWideCharPointer();
        config.pszContent     = message.toWideCharPointer();
        config.hInstance      = (HINSTANCE) Process::getCurrentModuleInstanceHandle();

        if (iconType == MessageBoxIconType::QuestionIcon)
        {
            if (auto* questionIcon = LoadIcon (nullptr, IDI_QUESTION))
            {
                config.hMainIcon = questionIcon;
                config.dwFlags |= TDF_USE_HICON_MAIN;
            }
        }
        else
        {
            auto icon = [this]() -> LPWSTR
            {
                switch (iconType)
                {
                    case MessageBoxIconType::WarningIcon:   return TD_WARNING_ICON;
                    case MessageBoxIconType::InfoIcon:      return TD_INFORMATION_ICON;

                    case MessageBoxIconType::QuestionIcon:  JUCE_FALLTHROUGH
                    case MessageBoxIconType::NoIcon:
                        break;
                }

                return nullptr;
            }();

            if (icon != nullptr)
                config.pszMainIcon = icon;
        }

        std::vector<TASKDIALOG_BUTTON> buttons;

        for (const auto* buttonText : { &button1, &button2, &button3 })
            if (buttonText->isNotEmpty())
                buttons.push_back ({ (int) buttons.size(), buttonText->toWideCharPointer() });

        config.pButtons = buttons.data();
        config.cButtons = (UINT) buttons.size();

        int buttonIndex = 0;
        taskDialogIndirect (&config, &buttonIndex, nullptr, nullptr);

        // MARK: MPC (begin)
        if (buttons.size() >= 3) {
          // return 0 if the third button was pressed (normally used for 'cancel')
          // return 1 if the first button was pressed (normally used for 'yes')
          // return 2 if the middle button was pressed (normally used for 'no')

          switch (buttonIndex) {
            case 0:
              return 1;
            case 1:
              return 2;
            case 2:
              return 0;
            default:
              jassertfalse;
              break;
          }

          return 0;
        }

        return buttonIndex;
        // MARK: MPC (
    }

HTH, Pete

13

Thanks for reporting. There’s a fix on the develop branch now:

1 Like