0

I have a standard dialog resource which has some radio controls on it.

At the moment it is all done the normal way so the first radio is mapped to a int variable.

DDX_Radio(pDX, IDC_RADIO_DISPLAY_EVERYONE, m_iDisplayMode);
DDX_Radio(pDX, IDC_RADIO_SELECT_EVERYONE, m_iSelectMode);

Here is the thing ... I have these associated enumerations:

enum class DisplayMode { Everyone = 0, Brother, Sister };
enum class SelectMode { Everyone = 0, Elders, MinisterialServants, Appointed, Custom, None };

Therefore, whenever I need to do some comparisons of the mapped variable I have to do it like this:

Example 1:

m_iDisplayMode = to_underlying(DisplayMode::Everyone);
m_iSelectMode = to_underlying(SelectMode::None);

Example 2:

if (m_iDisplayMode == to_underlying(DisplayMode::Everyone))
    bInclude = true;
else if (m_iDisplayMode == to_underlying(DisplayMode::Brother) && mapPublisher.second.eGender == Gender::Male)
    bInclude = true;
else if (m_iDisplayMode == to_underlying(DisplayMode::Sister) && mapPublisher.second.eGender == Gender::Female)
    bInclude = true;

The to_underlying function is a helper function that has been suggested previously to me here on SO and has been invaluable:

template <typename E>
constexpr auto to_underlying(E e) noexcept
{
    return static_cast<std::underlying_type_t<E>>(e);
}

What I want to know is whether it is possible to map those radio controls directly to the DisplayMode or SelectMode objects? So instead of mapping to 1 etc. it maps to DisplayMode::Everyone etc. This would simplfy the code in this context and avoid the need for all the to_underlying calls.


This is the MFC source for DDX_Radio:

void AFXAPI DDX_Radio(CDataExchange* pDX, int nIDC, int& value)
// must be first in a group of auto radio buttons
{
    pDX->PrepareCtrl(nIDC);
    HWND hWndCtrl;
    pDX->m_pDlgWnd->GetDlgItem(nIDC, &hWndCtrl);

    ASSERT(::GetWindowLong(hWndCtrl, GWL_STYLE) & WS_GROUP);
    ASSERT(::SendMessage(hWndCtrl, WM_GETDLGCODE, 0, 0L) & DLGC_RADIOBUTTON);

    if (pDX->m_bSaveAndValidate)
        value = -1;     // value if none found

    // walk all children in group
    int iButton = 0;
    do
    {
        if (::SendMessage(hWndCtrl, WM_GETDLGCODE, 0, 0L) & DLGC_RADIOBUTTON)
        {
            // control in group is a radio button
            if (pDX->m_bSaveAndValidate)
            {
                if (::SendMessage(hWndCtrl, BM_GETCHECK, 0, 0L) != 0)
                {
                    ASSERT(value == -1);    // only set once
                    value = iButton;
                }
            }
            else
            {
                // select button
                ::SendMessage(hWndCtrl, BM_SETCHECK, (iButton == value), 0L);
            }
            iButton++;
        }
        else
        {
            TRACE(traceAppMsg, 0, "Warning: skipping non-radio button in group.\n");
        }
        hWndCtrl = ::GetWindow(hWndCtrl, GW_HWNDNEXT);

    } while (hWndCtrl != NULL &&
        !(GetWindowLong(hWndCtrl, GWL_STYLE) & WS_GROUP));
}

I am trying to use the code in the answer but get this error:

enter image description here

Andrew Truckle
  • 17,769
  • 16
  • 66
  • 164
  • 1
    I'm pretty sure that [Stan, sorry, I mean Loretta](https://youtu.be/jlo7YZW8vPA) isn't going to like this code. Anyway... the simplest solution is probably to not store the selection in a member variable at all. Instead, implement a function that queries the UI state, and translates the selection into a scoped enum. It's probably also possible to implement a custom mapping between enum and UI state inside the `DoDataExchange` method, or some other data exchange machinery. – IInspectable Oct 12 '21 at 18:49
  • @IInspectable If I query the UI state then how would you know which radio is selected? DoDateExchange sounds interesting. One alternative is that I add my own enum variable (not mapped) and in all the clicked handlers update that enum manually. – Andrew Truckle Oct 12 '21 at 18:59
  • 2
    The framework provides [standard DDX routines](https://learn.microsoft.com/en-us/cpp/mfc/reference/standard-dialog-data-exchange-routines) that perform a two-way translation between data and the UI. You can implement your own, as hinted to [here](https://learn.microsoft.com/en-us/cpp/mfc/reference/cdataexchange-class). You can look into the provided implementation to see how to write a custom DDX routine. It probably boils down to lots of casts to perform the conversions, but it's confined to a single function. – IInspectable Oct 12 '21 at 19:36
  • @IInspectable This is also interesting: https://www.codeproject.com/articles/14510/dialog-data-exchange-in-mfc – Andrew Truckle Oct 12 '21 at 19:56
  • Realize by making it an enum class, if your UI needs to change, i.e. the radio button order needs to change (that technically should have "nothing to do with" the enum class order), you need to "map" it anyway? That's one of the drawbacks of tightly coupling UI to enum values (unless they CAN change, and their values are NEVER USED outside the dialog, but if not, you may run into this issue regardless) – franji1 Oct 12 '21 at 20:11
  • @franji1 In this case they won't change. I am personally thinking the easiest is what I said, to simply add to each radio click handler: `m_eDisplayMode = DisplayMode::Xxx`. That in a may is mapping via each click handler. – Andrew Truckle Oct 12 '21 at 20:14

1 Answers1

2

MFC supports mapping between data (class members) and UI state. The standard mechanism is called Dialog Data Exchange (DDX) which the code in the question is using already (DDX_Radio). The data exchange is two-way, triggered by a call to UpdateData, where an argument of TRUE translates the UI state into values, and FALSE reads the associated values and adjusts the UI appropriately.

There are a number of standard dialog data exchange routines provided by MFC already, but clients can provide their own in case none of them fit the immediate use case. The question falls into this category, and conveniently provides the implementation of DDX_Radio to serve as a starting point.

The implementation looks a fair bit intimidating, though things start to make sense once the code has been augmented with a few comments here and there:

CustomDDX.h:

template<typename E>
void AFXAPI DDX_RadioEnum(CDataExchange* pDX, int nIDC, E& value)
{
    // (1) Prepare the control for data exchange
    pDX->PrepareCtrl(nIDC);
    HWND hWndCtrl;
    pDX->m_pDlgWnd->GetDlgItem(nIDC, &hWndCtrl);

    // (2) Make sure this routine is associated with the first
    // radio button in a radio button group
    ASSERT(::GetWindowLong(hWndCtrl, GWL_STYLE) & WS_GROUP);
    // And verify, that it is indeed a radio button
    ASSERT(::SendMessage(hWndCtrl, WM_GETDLGCODE, 0, 0L) & DLGC_RADIOBUTTON);

    // (3) Iterate over all radio buttons in this group
    using value_t = std::underlying_type_t<E>;
    value_t rdbtn_index {};
    do {
        if (::SendMessage(hWndCtrl, WM_GETDLGCODE, 0, 0L) & DLGC_RADIOBUTTON) {
            // (4) Control is a radio button
            if (pDX->m_bSaveAndValidate) {
                // (5) Transfer data from UI to class member
                if (::SendMessage(hWndCtrl, BM_GETCHECK, 0, 0L) != 0) {
                    value = static_cast<E>(rdbtn_index);
                }
            } else {
                // (6) Transfer data from class member to UI
                ::SendMessage(hWndCtrl, BM_SETCHECK,
                              (static_cast<E>(rdbtn_index) == value), 0L);
            }
            ++rdbtn_index;
        } else {
            // (7) Not a radio button -> Issue warning
            TRACE(traceAppMsg, 0,
                  "Warning: skipping non-radio button in group.\n");
        }
        // (8) Move to next control in tab order
        hWndCtrl = ::GetWindow(hWndCtrl, GW_HWNDNEXT);

    }
    // (9) Until there are no more, or we moved to the next group
    while (hWndCtrl != NULL && !(GetWindowLong(hWndCtrl, GWL_STYLE) & WS_GROUP));
}

This declares a function template that can be instantiated for arbitrary scoped enum types, and implements the logic to translate between UI state and enum value. The integral underlying value of the enum serves as the zero-based index into the radio button group selection.

The implementation needs a bit of explanation, though. The following list provides a bit more information regarding the numbered // (n) code comments:

  1. This initializes internal state used by the framework. The precise details aren't very important, as long as the correct function is called. There are 3 implementations, one for OLE controls, one for edit controls, and one for every thing else. We're in the "everything else" category.
  2. Perform sanity checks. This verifies that the control identified by nIDC is the first control in a radio button group (WS_GROUP), and that it is indeed a radio button control. This helps weed out bugs early when running a debug build.
  3. Initialize the radio button index counter (rdbtn_index), and start iterating over radio buttons.
  4. Make sure the control we're operating on in this iteration is a radio button control (if not, see 7.).
  5. When translating UI state back to member variables, verify whether the current control is checked, and store its index in the group as a scoped enum value.
  6. Otherwise (i.e. when translating data to UI state) set the check mark if the numeric value of the enum matches the control index, and uncheck it otherwise. The latter is not strictly required when using BS_AUTORADIOBUTTON controls, but it's not harmful either.
  7. If we encounter a control that isn't a radio button control, issue a warning. Closely watch the debug output for this message; it designates a bug in the dialog template. Make sure to set the WS_GROUP style on the first control following this radio button group (in tab order).
  8. Move on to the next control in tab order.
  9. Terminate the loop if either there is no trailing control, or the control starts a new group, designated by the WS_GROUP style.

That's a fair bit to digest. Luckily, use of this function template is far less cumbersome. For purposes of illustration, let's use the following scoped enums:

enum class Season {
    Spring,
    Summer,
    Fall,
    Winter
};

enum class Color {
    Red,
    Green,
    Blue
};

and add the following class members to the dialog class:

private:
    Season season_ {};
    Color color_ { Color::Green };

All that's left is setting up the DDX associations, i.e.:

void CRadioEnumDlg::DoDataExchange(CDataExchange* pDX) {
    CDialogEx::DoDataExchange(pDX);
    DDX_RadioEnum(pDX, IDC_RADIO_SPRING, season_);
    DDX_RadioEnum(pDX, IDC_RADIO_RED, color_);
}

(with CRadioEnumDlg deriving from CDialogEx). All the template machinery is neatly hidden, with the template type argument getting inferred from the final argument.

For completeness, here is the dialog template used:

IDD_RADIOENUM_DIALOG DIALOGEX 0, 0, 178, 107
STYLE DS_SETFONT | DS_FIXEDSYS | WS_POPUP | WS_VISIBLE | WS_CAPTION | WS_SYSMENU | WS_THICKFRAME
EXSTYLE WS_EX_APPWINDOW
FONT 8, "MS Shell Dlg", 0, 0, 0x1
BEGIN
    DEFPUSHBUTTON   "OK",IDOK,59,86,50,14
    PUSHBUTTON      "Cancel",IDCANCEL,121,86,50,14
    CONTROL         "Spring",IDC_RADIO_SPRING,"Button",BS_AUTORADIOBUTTON | WS_GROUP | WS_TABSTOP,7,7,39,10
    CONTROL         "Summer",IDC_RADIO_SUMMER,"Button",BS_AUTORADIOBUTTON,7,20,39,10
    CONTROL         "Fall",IDC_RADIO_FALL,"Button",BS_AUTORADIOBUTTON,7,33,39,10
    CONTROL         "Winter",IDC_RADIO_WINTER,"Button",BS_AUTORADIOBUTTON,7,46,39,10
    CONTROL         "Red",IDC_RADIO_RED,"Button",BS_AUTORADIOBUTTON | WS_GROUP | WS_TABSTOP,54,7,39,10
    CONTROL         "Green",IDC_RADIO_GREEN,"Button",BS_AUTORADIOBUTTON,54,20,39,10
    CONTROL         "Blue",IDC_RADIO_BLUE,"Button",BS_AUTORADIOBUTTON,54,33,39,10
END

as well as its accompanying resource.h:

#define IDD_RADIOENUM_DIALOG            102
#define IDC_RADIO_SPRING                1000
#define IDC_RADIO_SUMMER                1001
#define IDC_RADIO_FALL                  1002
#define IDC_RADIO_WINTER                1003
#define IDC_RADIO_RED                   1004
#define IDC_RADIO_GREEN                 1005
#define IDC_RADIO_BLUE                  1006

Adjusting a default-generated MFC application (dialog-based) with the above produces the following result when launched:

RadioEnumDlg screencapture

That's sweet, actually. Note in particular that the second row of radio buttons has the second item checked, which matches the initial value set in the dialog class' implementation (Color color_ { Color::Green }).


So all's good then?

Well, yeah. I guess. Sort of, anyway. Let's talk about the things that aren't quite as cool, things to watch out for, and problems that simply don't have a solution.

The implementation provided above makes a number of assumptions, none of which can be verified at compile time, and only some of them can (and are) verified at run time:

  • Enum values need to be backed by integral values, starting at 0, and counting up without any gaps. To my knowledge, there's no way to enforce this today (C++20), and the most effective way to ensure this is a code comment.
  • The order of enum values must match the tab order of radio button controls. Again, this is nothing that can be enforced nor verified.
  • The control ID specified in the DDX_RadioEnum call must be the start of a radio button group. This is verified at run time (the first ASSERT).
  • The control ID specified in the DDX_RadioEnum call must identify a radio button control. Again, this is verified at run time (the second ASSERT).
  • The first control following the radio button group (in tab order) must have the WS_GROUP style set. This is verified at run time, in part. If the control following is not a radio button control, a warning is issued. If the control happens to be a radio button, then this is not something that can be verified.

Those assumptions certainly aren't impossible to match. The hard part is keeping those invariants valid over time. If you can, then this implementation is worth a try.

IInspectable
  • 46,945
  • 8
  • 85
  • 181
  • Thank you very much for your answer, code and explanations. I am going to give it a try early this evening. :) – Andrew Truckle Oct 13 '21 at 12:32
  • I seem to be missing something. I created a new header file and pasted your code. But I get an error on line `void AFXAPI DDX_RadioEnum(CDataExchange* pDX, int nIDC, E& value)`. It wants me to put a semicolon after `AFXAPI`. Have I missed a step? – Andrew Truckle Oct 13 '21 at 18:24
  • The error I am getting seems to be because we have `template`. – Andrew Truckle Oct 13 '21 at 19:17
  • This is odd. I can paste your code into the header of the dialog in question - no warning. – Andrew Truckle Oct 13 '21 at 19:32
  • I confirm that the whole thing does work well. Thanks. But as mentioned, it did not like it when I created a header file in my MFC project and put your code it in. It only likes it when I paste it into an existing dialog header file. It would be nice to resolve that. – Andrew Truckle Oct 13 '21 at 19:44
  • 1
    The error is caused because `AFXAPI` is unknown at the point of instantiation. It specifies the calling convention, and is defined in `afxver_.h`. I'm not sure which header should be included prior to the header that declares the class template, probably one of the catch-all headers, like `afx.h`. – IInspectable Oct 13 '21 at 19:51
  • I see. What I did notice is that if I do not "add" the header to the project is shows no warning. It is only when I "add" it to the project that this happens. – Andrew Truckle Oct 13 '21 at 19:52
  • Alright, `#include "stdafx.h"` solves it. – Andrew Truckle Oct 13 '21 at 19:53
  • Finally, is there any reason why we must set the next control in tab order after our radios to the group setting? Since the original radio ddx didn't need me to do that. I have done so just to be sure and it is Ok. But I may forget! – Andrew Truckle Oct 13 '21 at 19:55
  • 1
    `DDX_Radio` implements the same logic. If there is a control following the radio button group that doesn't have the `WS_GROUP` style and isn't a radio button control, you'll see the trace message (`"Warning: skipping non-radio button in group."`). That's just how radio button groups work: The first radio button needs the `WS_GROUP` style, and the group runs up to, but not including the next control with the `WS_GROUP` style. If you forget you still get the trace message as a reminder. – IInspectable Oct 13 '21 at 20:03
  • Don't you find it surprising that in Microsofts own code they don't actually test if `pDX` is `nullptr`? So it flags code analysis. Think I will insert a `if(pDX == nullptr) return;` as the first line. – Andrew Truckle Oct 24 '21 at 13:00
  • 1
    That code is old, likely older than 25 years. It was written at a time when static code analysis tools were mostly limited to C code, and came with a 5 figure price tag. It probably predates PREFast, or even the initial MISRA C draft. The world was different then, and the code is a time capsule from that time. No surprises here, really. If you want to harden your code (either by adding a null pointer check, or using an `_In_` annotation), that's fine. Just don't change MFC code. Instead, exclude it from code analysis. – IInspectable Oct 25 '21 at 09:01