From 3e195db27eceaafa6cd9c91b5c726c11994728b4 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Sun, 10 Jul 2022 20:45:00 +0100 Subject: [PATCH 1/4] Correct default OnCloseDocument() implementation documentation Don't say that the version of this function in wxDocument checks whether the document is modified and asks the user if it should be saved because this is not the case: this is done in a separate OnSaveModified() function called earlier. Also document that this function should always return true. --- interface/wx/docview.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/interface/wx/docview.h b/interface/wx/docview.h index 76b56045a6..bd0a82618f 100644 --- a/interface/wx/docview.h +++ b/interface/wx/docview.h @@ -1472,10 +1472,12 @@ public: case since wxWidgets 2.9.0. Returning @false from this function prevents the document from closing. - The default implementation does this if the document is modified and - the user didn't confirm discarding the modifications to it. - - Return @true to allow the document to be closed. + Note that there is no need to ask the user if the changes to the + document should be saved, as this was already checked by + OnSaveModified() by the time this function is called, if necessary, and + so, typically, this function should always return @true to allow the + document to be closed, as leaving it open after asking the user about + saving the changes would be confusing. */ virtual bool OnCloseDocument(); From ee6d58abe20ac0195932941f46cc8ab633b86d6a Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Thu, 14 Jul 2022 23:52:03 +0100 Subject: [PATCH 2/4] Don't call wxDocument::OnCloseDocument() twice when closing When closing the document using wxDocManager::CloseDocument(), which is what happens when using the standard wxID_CLOSE menu command handler, OnCloseDocument() was called twice: first when wxDocument::Close() was called directly from CloseDocument() itself and once again later when it was implicitly called from DeleteAllViews(). This was unexpected and also resulted in calling DeleteContents() twice, which is even more so for a function supposed to perform cleanup. Fix this by not calling Close() itself but instead a new CanClose() function from CloseDocument() to check whether we can close and then relying on DeleteAllViews() to actually close the document. --- docs/changes.txt | 6 ++++++ include/wx/docview.h | 6 ++++++ src/common/docview.cpp | 18 +++++++++++++----- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index 969bb9ee95..898516b8f5 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -11,6 +11,12 @@ INCOMPATIBLE CHANGES SINCE 3.2.x: Changes in behaviour not resulting in compilation errors -------------------------------------------------------- +- wxDocument::OnCloseDocument() was called twice in previous versions when + closing the document from the menu. Now it is only called once and after + destroying all the existing document views. If you overrode this function, + please check that you don't rely on any views existing when it's called. + + Changes in behaviour which may result in build errors ----------------------------------------------------- diff --git a/include/wx/docview.h b/include/wx/docview.h index c969da7fe3..1d7d329a3c 100644 --- a/include/wx/docview.h +++ b/include/wx/docview.h @@ -188,6 +188,12 @@ public: // part of the parent document and not a disk file as usual. bool IsChildDocument() const { return m_documentParent != NULL; } + // Ask the user if the document should be saved if it's modified and save + // it if necessary. + // + // Returns false if the user cancelled closing or if saving failed. + bool CanClose(); + protected: wxList m_documentViews; wxString m_documentFile; diff --git a/src/common/docview.cpp b/src/common/docview.cpp index d6a7349cde..fc6ecd445a 100644 --- a/src/common/docview.cpp +++ b/src/common/docview.cpp @@ -144,15 +144,14 @@ wxDocument::~wxDocument() //DeleteAllViews(); } -bool wxDocument::Close() +bool wxDocument::CanClose() { if ( !OnSaveModified() ) return false; // When the parent document closes, its children must be closed as well as - // they can't exist without the parent. + // they can't exist without the parent, so ask them too. - // As usual, first check if all children can be closed. DocsList::const_iterator it = m_childDocuments.begin(); for ( DocsList::const_iterator end = m_childDocuments.end(); it != end; ++it ) { @@ -163,6 +162,15 @@ bool wxDocument::Close() } } + return true; +} + +bool wxDocument::Close() +{ + // First check if this document itself and all its children can be closed. + if ( !CanClose() ) + return false; + // Now that they all did, do close them: as m_childDocuments is modified as // we iterate over it, don't use the usual for-style iteration here. while ( !m_childDocuments.empty() ) @@ -996,12 +1004,12 @@ wxDocManager::~wxDocManager() // closes the specified document bool wxDocManager::CloseDocument(wxDocument* doc, bool force) { - if ( !doc->Close() && !force ) + if ( !doc->CanClose() && !force ) return false; // To really force the document to close, we must ensure that it isn't // modified, otherwise it would ask the user about whether it should be - // destroyed (again, it had been already done by Close() above) and might + // destroyed (again, it had been already done by CanClose() above) and might // not destroy it at all, while we must do it here. doc->Modify(false); From dcee1cd025d72ce560e0d171960b3c9aaf5caf93 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 15 Jul 2022 00:36:45 +0100 Subject: [PATCH 3/4] Improve behaviour of "force closing" wxDocuments When the document was forced to close, OnSaveModified() was still called and allowed the user to cancel closing the document -- but it was still closed after OnSaveModified() returned. Be more upfront about it and tell the user that the document will be closed anyhow, but still propose them to save it if necessary. An alternative solution might be to just deprecate "force closing" entirely, as this seems very user-unfriendly. --- include/wx/docview.h | 4 +++ interface/wx/docview.h | 13 ++++++++ samples/docview/docview.cpp | 9 ++++++ samples/docview/docview.h | 3 ++ src/common/docview.cpp | 63 ++++++++++++++++++++++++++++++++----- 5 files changed, 85 insertions(+), 7 deletions(-) diff --git a/include/wx/docview.h b/include/wx/docview.h index 1d7d329a3c..f14e71f18c 100644 --- a/include/wx/docview.h +++ b/include/wx/docview.h @@ -117,6 +117,10 @@ public: // modified to false) virtual bool OnSaveModified(); + // Similar to OnSaveModified() but doesn't allow the user to prevent the + // document from closing as it will be closed unconditionally. + virtual void OnSaveBeforeForceClose(); + // if you override, remember to call the default // implementation (wxDocument::OnChangeFilename) virtual void OnChangeFilename(bool notifyViews); diff --git a/interface/wx/docview.h b/interface/wx/docview.h index bd0a82618f..82cd6d4c64 100644 --- a/interface/wx/docview.h +++ b/interface/wx/docview.h @@ -1544,6 +1544,19 @@ public: */ virtual bool OnSaveModified(); + /** + This function is called when a document is forced to close. + + The default implementation asks the user whether to save the changes + but, unlike OnSaveModified(), does not allow to cancel closing. + + The document is force closed when wxDocManager::CloseDocument() is + called with its @c force argument set to @true. + + @since 3.3.0 + */ + virtual void OnSaveBeforeForceClose(); + /** Removes the view from the document's list of views. diff --git a/samples/docview/docview.cpp b/samples/docview/docview.cpp index bc05ed6b7a..b2aa4eafb0 100644 --- a/samples/docview/docview.cpp +++ b/samples/docview/docview.cpp @@ -75,6 +75,7 @@ wxIMPLEMENT_APP(MyApp); wxBEGIN_EVENT_TABLE(MyApp, wxApp) EVT_MENU(wxID_ABOUT, MyApp::OnAbout) + EVT_MENU(wxID_CLEAR, MyApp::OnForceCloseAll) wxEND_EVENT_TABLE() MyApp::MyApp() @@ -315,6 +316,7 @@ void MyApp::AppendDocumentFileCommands(wxMenu *menu, bool supportsPrinting) menu->Append(wxID_SAVE); menu->Append(wxID_SAVEAS); menu->Append(wxID_REVERT, _("Re&vert...")); + menu->Append(wxID_CLEAR, "&Force close all"); if ( supportsPrinting ) { @@ -437,6 +439,13 @@ wxFrame *MyApp::CreateChildFrame(wxView *view, bool isCanvas) return subframe; } +void MyApp::OnForceCloseAll(wxCommandEvent& WXUNUSED(event)) +{ + // Pass "true" here to force closing just for testing this functionality, + // there is no real reason to force the issue here. + wxDocManager::GetDocumentManager()->CloseDocuments(true); +} + void MyApp::OnAbout(wxCommandEvent& WXUNUSED(event)) { wxString modeName; diff --git a/samples/docview/docview.h b/samples/docview/docview.h index 27d5175f1f..008ec7fe76 100644 --- a/samples/docview/docview.h +++ b/samples/docview/docview.h @@ -71,6 +71,9 @@ private: void CreateMenuBarForFrame(wxFrame *frame, wxMenu *file, wxMenu *edit); + // force close all windows + void OnForceCloseAll(wxCommandEvent& event); + // show the about box: as we can have different frames it's more // convenient, even if somewhat less usual, to handle this in the // application object itself diff --git a/src/common/docview.cpp b/src/common/docview.cpp index fc6ecd445a..e871b5d95f 100644 --- a/src/common/docview.cpp +++ b/src/common/docview.cpp @@ -554,6 +554,50 @@ bool wxDocument::OnSaveModified() return true; } +void wxDocument::OnSaveBeforeForceClose() +{ + if ( !IsModified() ) + return; + + wxMessageDialog dialogSave + ( + GetDocumentWindow(), + wxString::Format + ( + _("Do you want to save changes to %s before closing it?"), + GetUserReadableName() + ), + wxTheApp->GetAppDisplayName(), + wxYES_NO | wxICON_QUESTION | wxCENTRE + ); + dialogSave.SetExtendedMessage(_("The document must be closed.")); + dialogSave.SetYesNoLabels(_("&Save"), _("&Discard changes")); + + if ( dialogSave.ShowModal() == wxID_YES ) + { + while ( !Save() ) + { + wxMessageDialog dialogRetry + ( + GetDocumentWindow(), + wxString::Format + ( + _("Saving %s failed, would you like to retry?"), + GetUserReadableName() + ), + wxTheApp->GetAppDisplayName(), + wxYES_NO | wxICON_ERROR | wxCENTRE + ); + dialogRetry.SetYesNoLabels(_("Retry"), _("Discard changes")); + + if ( dialogRetry.ShowModal() != wxID_YES ) + break; + } + } + + Modify(false); +} + bool wxDocument::Draw(wxDC& WXUNUSED(context)) { return true; @@ -1004,14 +1048,19 @@ wxDocManager::~wxDocManager() // closes the specified document bool wxDocManager::CloseDocument(wxDocument* doc, bool force) { - if ( !doc->CanClose() && !force ) - return false; + if ( force ) + { + // We need to close, but at least ask the user if the document should + // be saved before doing it. + doc->OnSaveBeforeForceClose(); + } + else // Allow the user to cancel closing too. + { + if ( !doc->CanClose() ) + return false; + } - // To really force the document to close, we must ensure that it isn't - // modified, otherwise it would ask the user about whether it should be - // destroyed (again, it had been already done by CanClose() above) and might - // not destroy it at all, while we must do it here. - doc->Modify(false); + // Note that by now the document is certain not to be modified any longer. // Implicitly deletes the document when // the last view is deleted From 525c01a0e21791caf1bab860cb5659ba871d0f94 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 15 Jul 2022 00:42:04 +0100 Subject: [PATCH 4/4] Improve message box shown by wxDocument::OnSaveModified() Use more clear labels for the choices instead of just Yes/No/Cancel. --- src/common/docview.cpp | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/common/docview.cpp b/src/common/docview.cpp index e871b5d95f..8de09298b2 100644 --- a/src/common/docview.cpp +++ b/src/common/docview.cpp @@ -527,26 +527,34 @@ bool wxDocument::OnSaveModified() { if ( IsModified() ) { - switch ( wxMessageBox - ( - wxString::Format - ( - _("Do you want to save changes to %s?"), - GetUserReadableName() - ), - wxTheApp->GetAppDisplayName(), - wxYES_NO | wxCANCEL | wxICON_QUESTION | wxCENTRE, - GetDocumentWindow() - ) ) + wxMessageDialog dialogSave + ( + GetDocumentWindow(), + wxString::Format + ( + _("Do you want to save changes to %s?"), + GetUserReadableName() + ), + wxTheApp->GetAppDisplayName(), + wxYES_NO | wxCANCEL | wxICON_QUESTION | wxCENTRE + ); + dialogSave.SetYesNoCancelLabels + ( + _("&Save"), + _("&Discard changes"), + _("Do&n't close") + ); + + switch ( dialogSave.ShowModal() ) { - case wxNO: + case wxID_NO: Modify(false); break; - case wxYES: + case wxID_YES: return Save(); - case wxCANCEL: + case wxID_CANCEL: return false; } }