Bug 2524 - Macros "Cancel" button is confusing - doesn't do "what it says on the tin"

src/BatchProcessDialog.cpp

@@ -69,6 +69,9 @@
 #define MacrosPaletteTitle XO("Macros Palette")
 #define ManageMacrosTitle XO("Manage Macros")
 
+
+// Separate numerical range from the additional buttons
+// in the expanded view (which start at 10,000).
 #define MacrosListID       7001
 #define CommandsListID     7002
 #define ApplyToProjectID   7003
@@ -80,6 +83,7 @@ BEGIN_EVENT_TABLE(ApplyMacroDialog, wxDialogWrapper)
    EVT_BUTTON(ApplyToProjectID, ApplyMacroDialog::OnApplyToProject)
    EVT_BUTTON(ApplyToFilesID, ApplyMacroDialog::OnApplyToFiles)
    EVT_BUTTON(wxID_CANCEL, ApplyMacroDialog::OnCancel)
+   EVT_BUTTON(wxID_CLOSE, ApplyMacroDialog::OnCancel)
    EVT_BUTTON(wxID_HELP, ApplyMacroDialog::OnHelp)
 END_EVENT_TABLE()
 
@@ -174,7 +178,7 @@ void ApplyMacroDialog::PopulateOrExchange(ShuttleGui &S)
       /* i18n-hint: The Expand button makes the dialog bigger, with more in it */
       mResize = S.Id(ExpandID).AddButton(XXO("&Expand"));
       S.AddSpace( 10,10,1 );
-      S.AddStandardButtons( eCancelButton | eHelpButton);
+      S.AddStandardButtons( eCloseButton | eHelpButton);
    }
    S.EndHorizontalLay();
 }
@@ -495,16 +499,20 @@ void ApplyMacroDialog::OnCancel(wxCommandEvent & WXUNUSED(event))
 enum {
    AddButtonID = 10000,
    RemoveButtonID,
+   RenameButtonID,
+   RestoreButtonID,
    ImportButtonID,
    ExportButtonID,
+   SaveButtonID,
+
    DefaultsButtonID,
+
    InsertButtonID,
    EditButtonID,
    DeleteButtonID,
    UpButtonID,
    DownButtonID,
-   RenameButtonID,
+
-   RestoreButtonID,
 // MacrosListID             7005
 // CommandsListID,       7002
 // Re-Use IDs from ApplyMacroDialog.
@@ -523,9 +531,10 @@ BEGIN_EVENT_TABLE(MacrosWindow, ApplyMacroDialog)
    EVT_BUTTON(RestoreButtonID, MacrosWindow::OnRestore)
    EVT_BUTTON(ImportButtonID, MacrosWindow::OnImport)
    EVT_BUTTON(ExportButtonID, MacrosWindow::OnExport)
+   EVT_BUTTON(SaveButtonID, MacrosWindow::OnSave)
+
    EVT_BUTTON(ExpandID, MacrosWindow::OnExpand)
    EVT_BUTTON(ShrinkID, MacrosWindow::OnShrink)
-
    EVT_SIZE(MacrosWindow::OnSize)
 
    EVT_LIST_ITEM_ACTIVATED(CommandsListID, MacrosWindow::OnCommandActivated)
@@ -537,6 +546,7 @@ BEGIN_EVENT_TABLE(MacrosWindow, ApplyMacroDialog)
 
    EVT_BUTTON(wxID_OK, MacrosWindow::OnOK)
    EVT_BUTTON(wxID_CANCEL, MacrosWindow::OnCancel)
+   EVT_BUTTON(wxID_CLOSE, MacrosWindow::OnCancel)
 
    EVT_KEY_DOWN(MacrosWindow::OnKeyDown)
 END_EVENT_TABLE()
@@ -628,6 +638,8 @@ void MacrosWindow::PopulateOrExchange(ShuttleGui & S)
                mRestore = S.Id(RestoreButtonID).AddButton(XXO("Re&store"));
                mImport = S.Id(ImportButtonID).AddButton(XO("I&mport..."));
                mExport = S.Id(ExportButtonID).AddButton(XO("E&xport..."));
+               mSave = S.Id(SaveButtonID).AddButton(XO("&Save"));
+               mSave->Enable( mChanged );
             }
             S.EndVerticalLay();
          }
@@ -693,11 +705,12 @@ void MacrosWindow::PopulateOrExchange(ShuttleGui & S)
       // OnOK saves without prompting.
       // That difference is too slight to merit a button, and with the OK
       // button, people might expect the dialog to apply the macro too.
-      S.AddStandardButtons( /*eOkButton |*/ eCancelButton | eHelpButton);
+      S.AddStandardButtons( /*eOkButton |*/ eCloseButton | eHelpButton);
    }
+
    S.EndHorizontalLay();
 
-
+   
    return;
 }
 
@@ -761,14 +774,22 @@ void MacrosWindow::UpdateMenus()
 
 void MacrosWindow::UpdateDisplay( bool bExpanded )
 {
+   // If we failed to save changes, we abandon the attempt to 
+   // change the expand/shrink state of the GUI.
    if( !SaveChanges() )
       return;
 
    mbExpanded = bExpanded;
+
+   mChanged = false;
+   // if we try to access the about to be destroyed mSave button 
+   // inappropriately, we need to crash rather than (sometimes) silently 
+   // succeed.
+   mSave = nullptr;
+
    DestroyChildren();
    SetSizer( nullptr );
-   
+
-   mChanged = false;
    mSelectedCommand = 0;
    SetMinSize( wxSize( 200,200 ));
 
@@ -795,7 +816,10 @@ void MacrosWindow::OnExpand(wxCommandEvent &WXUNUSED(event))
 {  UpdateDisplay( true );}
 
 void MacrosWindow::OnShrink(wxCommandEvent &WXUNUSED(event))
-{  UpdateDisplay( false );}
+{  
+   if( ChangeOK() )
+      UpdateDisplay( false );
+}
 
 
 bool MacrosWindow::ChangeOK()
@@ -821,6 +845,7 @@ bool MacrosWindow::ChangeOK()
       }
 
       mChanged = false;
+      mSave->Enable( mChanged );
    }
 
    return true;
@@ -1044,6 +1069,7 @@ void MacrosWindow::OnRemove(wxCommandEvent & WXUNUSED(event))
    // changed.  Since we've just deleted the macro, we should
    // forget about that change.
    mChanged = false;
+   mSave->Enable( mChanged );
    mActiveMacro = mMacros->GetItemText(item);
 
    PopulateMacros();
@@ -1070,6 +1096,7 @@ void MacrosWindow::OnRestore(wxCommandEvent & WXUNUSED(event))
    mMacroCommands.RestoreMacro(mActiveMacro);
 
    mChanged = true;
+   mSave->Enable( mChanged );
 
    PopulateList();
 }
@@ -1114,6 +1141,12 @@ void MacrosWindow::OnExport(wxCommandEvent & WXUNUSED(event))
    mMacroCommands.WriteMacro(mMacros->GetItemText(item), this);
 }
 
+void MacrosWindow::OnSave(wxCommandEvent & WXUNUSED(event))
+{
+   SaveChanges();
+}
+
+
 /// An item in the list has been selected.
 /// Bring up a dialog to allow its parameters to be edited.
 void MacrosWindow::OnCommandActivated(wxListEvent & WXUNUSED(event))
@@ -1154,6 +1187,8 @@ void MacrosWindow::InsertCommandAt(int item)
                                 d.mSelectedParameters,
                                 item);
       mChanged = true;
+      mSave->Enable( mChanged );
+
       mSelectedCommand = item + 1;
       PopulateList();
    }
@@ -1191,7 +1226,10 @@ void MacrosWindow::OnEditCommandParams(wxCommandEvent & WXUNUSED(event))
    mMacroCommands.AddToMacro(command,
       params,
       item);
+
    mChanged = true;
+   mSave->Enable( mChanged );
+
    mSelectedCommand = item;
    PopulateList();
 }
@@ -1207,7 +1245,9 @@ void MacrosWindow::OnDelete(wxCommandEvent & WXUNUSED(event))
    }
 
    mMacroCommands.DeleteFromMacro(item);
+
    mChanged = true;
+   mSave->Enable( mChanged );
 
    if (item >= (mList->GetItemCount() - 2) && item >= 0) {
       item--;
@@ -1230,7 +1270,10 @@ void MacrosWindow::OnUp(wxCommandEvent & WXUNUSED(event))
                              mMacroCommands.GetParams(item),
                              item - 1);
    mMacroCommands.DeleteFromMacro(item + 1);
+
    mChanged = true;
+   mSave->Enable( mChanged );
+
    mSelectedCommand = item - 1;
    PopulateList();
 }
@@ -1249,7 +1292,10 @@ void MacrosWindow::OnDown(wxCommandEvent & WXUNUSED(event))
                              mMacroCommands.GetParams(item),
                              item + 2);
    mMacroCommands.DeleteFromMacro(item);
+
    mChanged = true;
+   mSave->Enable( mChanged );
+
    mSelectedCommand = item + 1;
    PopulateList();
 }
@@ -1277,7 +1323,11 @@ bool MacrosWindow::SaveChanges(){
          return false;
       }
    }
+
    mChanged = false;
+   if( mSave )
+      mSave->Enable( mChanged );
+
    return true;
 }
 

src/BatchProcessDialog.h

@@ -107,6 +107,8 @@ private:
    void OnRestore(wxCommandEvent &event);
    void OnImport(wxCommandEvent &event);
    void OnExport(wxCommandEvent &event);
+   void OnSave(wxCommandEvent &event);
+
    void OnExpand(wxCommandEvent &event);
    void OnShrink(wxCommandEvent &event);
    void OnSize(wxSizeEvent &event);
@@ -136,6 +138,7 @@ private:
    wxButton *mRestore;
    wxButton *mImport;
    wxButton *mExport;
+   wxButton *mSave;
 
    int mSelectedCommand;
    bool mChanged;