I wondered if anyone could shed some light on the root cause of sporadic behaviour in my application. This is the function so far which behaves correctly:
template<typename GetterFunction, typename SetterFunction>
void CWeekendMeetingDlg::OnSwapWith(GetterFunction&& getter,
SetterFunction&& setter,
UINT nID, UINT nReplacementCtrlID, UINT nReplacementDescID)
{
// We need to locate the name for this assignment
const auto iter = m_mapSwapWeekendAssignments.find(nID);
if (iter != m_mapSwapWeekendAssignments.end())
{
// We found it
//const gsl::not_null<CChristianLifeMinistryEntry*> pReplacement = m_mapSwapWeekendAssignments[nID];
CChristianLifeMinistryEntry* pReplacement = m_mapSwapWeekendAssignments[nID];
CString strExistingName, strReplacementName;
CString strExistingDescription, strReplacementDescription;
GetDlgItemText(m_iSwapAssignmentSourceID, strExistingName);
if (m_iSwapAssignmentSourceDescID == IDS_STR_HOST)
{
CChristianLifeMinistryLabelsDlg dlgLabels(this);
strExistingDescription = dlgLabels.GetTrimmedHostLabel();
}
else if (m_iSwapAssignmentSourceDescID == IDS_STR_COHOST)
{
CChristianLifeMinistryLabelsDlg dlgLabels(this);
strExistingDescription = dlgLabels.GetTrimmedCoHostLabel();
}
else if (m_iSwapAssignmentSourceDescID == IDS_STR_ZOOM_ATTENDANT)
{
CChristianLifeMinistryLabelsDlg dlgLabels(this);
strExistingDescription = dlgLabels.GetTrimmedZoomAttendantLabel();
}
else
strExistingDescription.LoadString(m_iSwapAssignmentSourceDescID);
if (nReplacementDescID == IDS_STR_HOST)
{
CChristianLifeMinistryLabelsDlg dlgLabels(this);
strReplacementDescription = dlgLabels.GetTrimmedHostLabel();
}
else if (nReplacementDescID == IDS_STR_COHOST)
{
CChristianLifeMinistryLabelsDlg dlgLabels(this);
strReplacementDescription = dlgLabels.GetTrimmedCoHostLabel();
}
else if (nReplacementDescID == IDS_STR_ZOOM_ATTENDANT)
{
CChristianLifeMinistryLabelsDlg dlgLabels(this);
strReplacementDescription = dlgLabels.GetTrimmedZoomAttendantLabel();
}
else
strReplacementDescription.LoadString(nReplacementDescID);
CString strPrompt;
CString strExistingDate = m_pCurrentEntry->GetMeetingDateAsString();
CString strReplacementDate = pReplacement->GetMeetingDateAsString();
strReplacementName = getter(pReplacement);
strPrompt.FormatMessage(IDS_TPL_SWAP_CONFIRMATION,
(LPCTSTR)strExistingDate,
(LPCTSTR)strExistingName,
(LPCTSTR)strExistingDescription,
(LPCTSTR)strReplacementDate,
(LPCTSTR)strReplacementName,
(LPCTSTR)strReplacementDescription);
if (AfxMessageBox(strPrompt, MB_ICONQUESTION | MB_YESNO) == IDYES)
{
strReplacementName = getter(pReplacement);
setter(pReplacement, strExistingName);
if (pReplacement == m_pCurrentEntry) // Swapping assignments on same meeting!
SetDlgItemText(nReplacementCtrlID, strExistingName);
SetDlgItemText(m_iSwapAssignmentSourceID, strReplacementName);
GetParent()->PostMessage(UM_SM_EDITOR_SET_MODIFIED, gsl::narrow<WPARAM>(TRUE));
}
}
}
The reason it behaves correctly is because I stopped using the gsl::not_null
code:
//const gsl::not_null<CChristianLifeMinistryEntry*> pReplacement = m_mapSwapWeekendAssignments[nID];
CChristianLifeMinistryEntry* pReplacement = m_mapSwapWeekendAssignments[nID];
I appreciate that this is not a minimum working example, but it is tricky to supply that. This function is used to build by a context menu.
What I found was that when I used the not_null
approach there were instances where the pReplacement
pointer was not correct. In fact, once it went wrong, it would stay on this other pointer in the list. Have I described it well?
Anyway, when I stop using not_null
it works as expected with no odd behaviour. It is always the right pReplacement
pointer, every time. Why the odd behaviour when using not_null
?
I ought to point out that the reason I tried to use gsl::not_null
was to address a C26429 code analysis warning.
Here is an example of the calling code where I define the getter
and setter
:
void CWeekendMeetingDlg::OnSwapWithZoomAttendantAssignment(UINT nID)
{
auto getter = [](gsl::not_null<CChristianLifeMinistryEntry*> pReplacement)
{
S_TALK_INFO sPTI = pReplacement->GetPublicTalkInfo();
return sPTI.strZoomAttendant;
};
auto setter = [](gsl::not_null<CChristianLifeMinistryEntry*> pReplacement, CString& strExistingName)
{
S_TALK_INFO sPTI = pReplacement->GetPublicTalkInfo();
sPTI.strZoomAttendant = strExistingName;
pReplacement->SetPublicTalkInfo(sPTI);
};
CWeekendMeetingDlg::OnSwapWith(
getter, setter, nID, IDC_COMBO_WEEKEND_ZOOM_ATTENDANT, IDS_STR_ZOOM_ATTENDANT);
}
It turns out that my initial thoughts about not using gsl::not_null
was a red herring because the odd behaviour was still happening.
I did another close inspection of my code since I have a similar Swap Assignment context menu in another editor in the software. And I stumbled on the bug.
I needed to add the clear()
call here to reset the map:
BOOL CWeekendMeetingDlg::PreTranslateMessage(MSG* pMsg)
{
m_ToolTips.RelayEvent(pMsg);
if (IsCTRLpressed() && pMsg->message == WM_LBUTTONDOWN)
{
m_mapSwapWeekendAssignments.clear();
if (ValidateAssignmentControl(pMsg->hwnd))
{
DisplaySwapAssignmentsPopupMenu(pMsg->pt);
return true;
}
}
return CResizingDialog::PreTranslateMessage(pMsg);
}
When I was clicking on another control to start beginning a new swap process it was not resetting the map, so it was getting messed up and confused. Clearing the map resolves the issue.