0

My code in itself is working OK and I am wondering if there is a simpler way to determine if at least one of my menu items are checked.

Context

The menu is being used by a CMFCMenuButton and looks like this:

Menu

Behaviour

The menu is essentially a set of checked menu items which the user can toggle as needed. The application then performs some actions based on the state of these menu items. Basic stuff.

At the moment I have this code snippet (working fine):

MSAToolsLibrary::IPublisherPtr pPublisher = nullptr;

if (theApp.MSAToolsInterface().GetPublisher(aryStrNames[i], pPublisher))
{
    bool bAdd = false;

    MSAToolsLibrary::Serving eServing;
    MSAToolsLibrary::Appointed eAppointed;

    pPublisher->get_ServingAs(&eServing);
    pPublisher->get_AppointedAs(&eAppointed);

    bool bNoExtraPublisherFilter = true;
    if (m_menuExtraPublisherFilter.GetMenuState(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_UNBAPTISED_PUBLISHER, MF_BYCOMMAND) == MF_CHECKED)
    {
        bNoExtraPublisherFilter = false;
        if (eServing == MSAToolsLibrary::Serving_UnbaptisedPublisher)
            bAdd = true;
    }
    if (m_menuExtraPublisherFilter.GetMenuState(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_PUBLISHER, MF_BYCOMMAND) == MF_CHECKED)
    {
        bNoExtraPublisherFilter = false;
        if (eServing == MSAToolsLibrary::Serving_Publisher)
            bAdd = true;
    }
    if (m_menuExtraPublisherFilter.GetMenuState(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_REGULAR_PIONEER, MF_BYCOMMAND) == MF_CHECKED)
    {
        bNoExtraPublisherFilter = false;
        if (eServing == MSAToolsLibrary::Serving_RegularPioneer)
            bAdd = true;
    }
    if (m_menuExtraPublisherFilter.GetMenuState(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_OTHER, MF_BYCOMMAND) == MF_CHECKED)
    {
        bNoExtraPublisherFilter = false;
        if (eServing == MSAToolsLibrary::Serving_Other)
            bAdd = true;
    }
    if (m_menuExtraPublisherFilter.GetMenuState(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_NOT_APPOINTED, MF_BYCOMMAND) == MF_CHECKED)
    {
        bNoExtraPublisherFilter = false;
        if (eAppointed == MSAToolsLibrary::Appointed_NotAppointed)
            bAdd = true;
    }
    if (m_menuExtraPublisherFilter.GetMenuState(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_MINISTERIAL_SERVANT, MF_BYCOMMAND) == MF_CHECKED)
    {
        bNoExtraPublisherFilter = false;
        if (eAppointed == MSAToolsLibrary::Appointed_MinisterialServant)
            bAdd = true;
    }
    if (m_menuExtraPublisherFilter.GetMenuState(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_ELDER, MF_BYCOMMAND) == MF_CHECKED)
    {
        bNoExtraPublisherFilter = false;
        if (eAppointed == MSAToolsLibrary::Appointed_Elder)
            bAdd = true;
    }

    if(bNoExtraPublisherFilter)
    {
        // None of the menu items were checked
        bAdd = true;
    }

    if(bAdd)
        m_lbPublishers.AddString(aryStrNames[i]);
}

I just wondered if there is a quicker way to see if none of the menu items have been set to MF_CHECKED? Because if that condition can be detected I could bypass the GetPublisher code all together and just add them to the list.

I realise I can repeat my existing if statements in a dedicated function that returns bool and do it that way but is there an easier way?

Update

The resource ID values for the menu items are sequential, as follows:

#define ID_PUBLISHERS_DATABASE_EXTRA_FILTER_PUBLISHER            35389
#define ID_PUBLISHERS_DATABASE_EXTRA_FILTER_REGULAR_PIONEER      35390
#define ID_PUBLISHERS_DATABASE_EXTRA_FILTER_NOT_APPOINTED        35391
#define ID_PUBLISHERS_DATABASE_EXTRA_FILTER_MINISTERIAL_SERVANT  35392
#define ID_PUBLISHERS_DATABASE_EXTRA_FILTER_ELDER                35393
#define ID_PUBLISHERS_DATABASE_EXTRA_FILTER_UNBAPTISED_PUBLISHER 35394
#define ID_PUBLISHERS_DATABASE_EXTRA_FILTER_OTHER                35395

But the menu does have a separator part way through.

Issues with Macro

I thought I would try the macro idea on a test project. I inserted this macro in the header file:

#define PROCESSFILTER(id, cond) \
        if (m_menuExtraPublisherFilter.GetMenuState(id, MF_BYCOMMAND) == MF_CHECKED) \
        { \
          bNoExtraPublisherFilter = false; \
          if (cond) \
            bAdd = true; \
        }

I added m_menuExtraPublisherFilter as a CMenu member variable of a CDiialogEx. I then created dummy resource ID ID_TEST just used a standard condition since I don't have access to my tools library in the demo. In OnInitDialog I tried:

PROCESSFILTER(ID_TEST, 5 > 4);

But it crashes. Why?

enter image description here

Andrew Truckle
  • 17,769
  • 16
  • 66
  • 164
  • 2
    You could use a loop to run through the menu items, using the `MF_BYPOSITION` flag. Or, set your command IDs so that they are consecutive values, then loop through those. – Adrian Mole May 28 '21 at 11:05
  • @AdrianMole They are sequential menu item IDs (35389 to 35395). – Andrew Truckle May 28 '21 at 11:15
  • 2
    You should not base your code on the assumption the resource IDS will be sequential. I've done the very same shit in the past. – sergiol May 28 '21 at 12:06
  • @sergiol I know what you mean. It is easy to forget and "break". I think then I just have to decide if it is worth introducing a new function to specifically test for this scenario, because it it returns saying one or more ate ticked I am only going to have to do another one by one `if` check anyway. I think I will leave it as it is. – Andrew Truckle May 28 '21 at 12:25
  • 1
    I guess I don't understand the real problem... IMHO, the state of the menus is irrelevant. What is important is the state of variables in your window or document or application. In general, you should have update command handlers that set the check based on the state of variables in your code. The CCmdUI class has a SetCheck member. IMHO, the preferred way is to write handlers like ON_COMMAND(ID_Whatever, DoWhatever) in your message map and then set the check or not set the check there. – Joseph Willcoxson May 28 '21 at 14:51
  • @Joseph Update handlers don’t apply as this is not using the MDI framework etc. And I don’t have variables to match the menu items. But your right. I could do it easy with variable. – Andrew Truckle May 28 '21 at 14:59
  • @joseph and you don’t use the COMMAND approach with MFC menu buttons. You do it as I have shown by examining the menubresult in the button click handler. – Andrew Truckle May 28 '21 at 15:00
  • 1
    It's not that easy. I would just consider simple ways to make the code looking more tidy. But you mix UI and application logic code here. You don't examine only the checked states of the menu items but you combine them with other checks, so the solutions proposed above, based on the range or position (apart from being good or not), won't work. Maybe you can add a function or macro to make the checks and call it for every item (7 calls), because the code inside the braces is of the same pattern. – Constantine Georgiou May 28 '21 at 19:55
  • @ConstantineGeorgiou Hhhm. that is a thought, using a macro. I assume this macro needs to be passed the ID and enum. But I am not sure how macros return values (eg, set the bAdd. I wonder if this is a good candidate for one of those (is lambda embedded function)? Not sure. – Andrew Truckle May 28 '21 at 20:09
  • 1
    The macro would need to be passed the ID and the condition (eg `eServing == MSAToolsLibrary::Serving_UnbaptisedPublisher`). The macro can be written in a way so as to "return" a value (eg check the min()/max() macros) but this is not necessary here, you can just include the code (`bNoExtraPublisherFilter = false;` and `if (cond) bAdd=true;`). Macros is a simple text-replacement mechanism, not functions, so you don't need to pass `bNoExtraPublisherFilter` or `bAdd`. – Constantine Georgiou May 28 '21 at 20:28
  • @ConstantineGeorgiou I updated the question as I have issue with my macro in my test project. – Andrew Truckle May 31 '21 at 09:57
  • Apparently an assertion about your menu and the `ID_TEST` item. Eg, is the menu created when you call this part of the code in your test program? You could simply check what's at line 941 of (your) win1.inl. In the program the call would be like `PROCESSFILTER(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_UNBAPTISED_PUBLISHER, eServing == MSAToolsLibrary::Serving_UnbaptisedPublisher);` No really different to what it was before, just making the code a little easier to read. – Constantine Georgiou May 31 '21 at 12:57
  • @ConstantineGeorgiou OK, maybe the test project was incomplete with respects to the menus. Must be the case. I will try in my main project instead as you suggest. – Andrew Truckle May 31 '21 at 14:16
  • @ConstantineGeorgiou I had the same error in my main project. I was running this macro before the split button had been setup so the menu handle was not valid. I have now added an answer. – Andrew Truckle May 31 '21 at 21:04
  • 1
    The only circumstance it is correct to use the menu IDs as a sequence, is when you define the sequence yourself outside the resource files, like when you have a dynamic range of items, which will be processed as entries of such range. I know you are experienced enough to already have crossed ways with `ON_COMMAND_RANGE` and `ON_UPDATE_COMMAND_UI_RANGE`. – sergiol Jun 11 '21 at 22:11
  • @sergiol Yes I use range handlers. – Andrew Truckle Jun 12 '21 at 04:06

1 Answers1

0

Based on the suggestions in the comment I ended up creating a macro which I added to the header file:

#define PROCESSFILTER(id, cond) \
        if (m_menuExtraPublisherFilter.GetMenuState(id, MF_BYCOMMAND) == MF_CHECKED) \
        { \
          bNoExtraPublisherFilter = false; \
          if (cond) \
            bAdd = true; \
        }

And I simplified the source code to this:

MSAToolsLibrary::IPublisherPtr pPublisher = nullptr;

if (theApp.MSAToolsInterface().GetPublisher(aryStrNames[i], pPublisher))
{
    bool bAdd = false;

    MSAToolsLibrary::Serving eServing;
    MSAToolsLibrary::Appointed eAppointed;

    pPublisher->get_ServingAs(&eServing);
    pPublisher->get_AppointedAs(&eAppointed);

    bool bNoExtraPublisherFilter = true;

    PROCESSFILTER(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_UNBAPTISED_PUBLISHER, eServing == MSAToolsLibrary::Serving_UnbaptisedPublisher);
    PROCESSFILTER(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_PUBLISHER, eServing == MSAToolsLibrary::Serving_Publisher);
    PROCESSFILTER(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_REGULAR_PIONEER, eServing == MSAToolsLibrary::Serving_RegularPioneer);
    PROCESSFILTER(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_OTHER, eServing == MSAToolsLibrary::Serving_Other);
    PROCESSFILTER(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_NOT_APPOINTED, eAppointed == MSAToolsLibrary::Appointed_NotAppointed);
    PROCESSFILTER(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_MINISTERIAL_SERVANT, eAppointed == MSAToolsLibrary::Appointed_MinisterialServant);
    PROCESSFILTER(ID_PUBLISHERS_DATABASE_EXTRA_FILTER_ELDER, eAppointed == MSAToolsLibrary::Appointed_Elder);

    if(bNoExtraPublisherFilter)
    {
        // None of the menu items were checked
        bAdd = true;
    }

    if(bAdd)
        m_lbPublishers.AddString(aryStrNames[i]);
}

The one thing that I had to ensure was that the CSplitButton menu was constructed before the macro is executed. Otherwise an exception is raised due to the menu having an invalid handle.

Andrew Truckle
  • 17,769
  • 16
  • 66
  • 164