Skip to content

Commit

Permalink
[Core] (Partially?) Fix data loss on dir rename (#4996)
Browse files Browse the repository at this point in the history 
* Fix lost filename in err msg

In some circumstances, FileExceptions are constructed empty, then have a
filename assigned to them, but the error message in these scenarios is
left as the default "unknown" one, which is sometimes shown to users.
This change fixes that case to be consistent with instances that are
constructed with the filename.

The exception can happen when trying to save the file in a location that does
not exist, or when the user does not have permission to write there. If it
comes when saving after closing the document, all previous changes can be lost.

Partially fixes issue #4098.

Co-authored-by: Heewa Barfchin <heewa.b@gmail.com>
  • Loading branch information
@AjinkyaDahale @heewa
AjinkyaDahale and heewa committed Sep 16, 2021
1 parent 3ab5dad commit 820e88f
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 14 deletions.
5 changes: 5 additions & 0 deletions src/App/Document.cpp
View file
Expand Up @@ -66,6 +66,7 @@ recompute path. Also, it enables more complicated dependencies beyond trees.
# include <climits>
# include <bitset>
# include <random>
# include <boost/filesystem.hpp>
#endif

#include <boost/algorithm/string.hpp>
Expand Down Expand Up @@ -142,6 +143,8 @@ using namespace zipios;
# define FC_LOGFEATUREUPDATE
#endif

namespace fs = boost::filesystem;

// typedef boost::property<boost::vertex_root_t, DocumentObject* > VertexProperty;
typedef boost::adjacency_list <
boost::vecS, // class OutEdgeListS : a Sequence or an AssociativeContainer
Expand Down Expand Up @@ -2610,6 +2613,8 @@ bool Document::saveToFile(const char* filename) const
fn += uuid;
}
Base::FileInfo tmp(fn);
// In case some folders in the path do not exist
fs::create_directories(fs::path(filename).parent_path());

// open extra scope to close ZipWriter properly
{
Expand Down
68 changes: 64 additions & 4 deletions src/Gui/Document.cpp
View file
Expand Up @@ -1158,6 +1158,25 @@ bool Document::save(void)
if(gdoc) gdoc->setModified(false);
}
}
catch (const Base::FileException& e) {
int ret = QMessageBox::question(
getMainWindow(),
QObject::tr("Could not Save Document"),
QObject::tr("There was an issue trying to save the file. "
"This may be because some of the parent folders do not exist, "
"or you do not have sufficient permissions, "
"or for other reasons. Error details:\n\n\"%1\"\n\n"
"Would you like to save the file with a different name?")
.arg(QString::fromLatin1(e.what())),
QMessageBox::Yes, QMessageBox::No);
if (ret == QMessageBox::No) {
// TODO: Understand what exactly is supposed to be returned here
getMainWindow()->showMessage(QObject::tr("Saving aborted"), 2000);
return false;
} else if (ret == QMessageBox::Yes) {
return saveAs();
}
}
catch (const Base::Exception& e) {
QMessageBox::critical(getMainWindow(), QObject::tr("Saving document failed"),
QString::fromLatin1(e.what()));
Expand Down Expand Up @@ -1196,6 +1215,25 @@ bool Document::saveAs(void)
setModified(false);
getMainWindow()->appendRecentFile(fi.filePath());
}
catch (const Base::FileException& e) {
int ret = QMessageBox::question(
getMainWindow(),
QObject::tr("Could not Save Document"),
QObject::tr("There was an issue trying to save the file. "
"This may be because some of the parent folders do not exist, "
"or you do not have sufficient permissions, "
"or for other reasons. Error details:\n\n\"%1\"\n\n"
"Would you like to save the file with a different name?")
.arg(QString::fromLatin1(e.what())),
QMessageBox::Yes, QMessageBox::No);
if (ret == QMessageBox::No) {
// TODO: Understand what exactly is supposed to be returned here
getMainWindow()->showMessage(QObject::tr("Saving aborted"), 2000);
return false;
} else if (ret == QMessageBox::Yes) {
return saveAs();
}
}
catch (const Base::Exception& e) {
QMessageBox::critical(getMainWindow(), QObject::tr("Saving document failed"),
QString::fromLatin1(e.what()));
Expand Down Expand Up @@ -1942,11 +1980,33 @@ bool Document::canClose (bool checkModify, bool checkLink)

bool ok = true;
if (checkModify && isModified() && !getDocument()->testStatus(App::Document::PartialDoc)) {
int res = getMainWindow()->confirmSave(getDocument()->Label.getValue(),getActiveView());
if(res>0)
const char *docName = getDocument()->Label.getValue();
int res = getMainWindow()->confirmSave(docName, getActiveView());
switch (res)
{
case MainWindow::ConfirmSaveResult::Cancel:
ok = false;
break;
case MainWindow::ConfirmSaveResult::SaveAll:
case MainWindow::ConfirmSaveResult::Save:
ok = save();
else
ok = res<0;
if (!ok) {
int ret = QMessageBox::question(
getActiveView(),
QObject::tr("Document not saved"),
QObject::tr("The document%1 could not be saved. Do you want to cancel closing it?")
.arg(docName?(QString::fromUtf8(" ")+QString::fromUtf8(docName)):QString()),
QMessageBox::Discard | QMessageBox::Cancel,
QMessageBox::Discard);
if (ret == QMessageBox::Discard)
ok = true;
}
break;
case MainWindow::ConfirmSaveResult::DiscardAll:
case MainWindow::ConfirmSaveResult::Discard:
ok = true;
break;
}
}

if (ok) {
Expand Down
38 changes: 28 additions & 10 deletions src/Gui/MainWindow.cpp
View file
Expand Up @@ -612,15 +612,15 @@ int MainWindow::confirmSave(const char *docName, QWidget *parent, bool addCheckb
discardBtn->setShortcut(QKeySequence::mnemonic(text));
}

int res = 0;
int res = ConfirmSaveResult::Cancel;
box.adjustSize(); // Silence warnings from Qt on Windows
switch (box.exec())
{
case QMessageBox::Save:
res = checkBox.isChecked()?2:1;
res = checkBox.isChecked()?ConfirmSaveResult::SaveAll:ConfirmSaveResult::Save;
break;
case QMessageBox::Discard:
res = checkBox.isChecked()?-2:-1;
res = checkBox.isChecked()?ConfirmSaveResult::DiscardAll:ConfirmSaveResult::Discard;
break;
}
if(addCheckbox && res)
Expand All @@ -632,12 +632,13 @@ bool MainWindow::closeAllDocuments (bool close)
{
auto docs = App::GetApplication().getDocuments();
try {
docs = App::Document::getDependentDocuments(docs,true);
docs = App::Document::getDependentDocuments(docs, true);
}catch(Base::Exception &e) {
e.ReportException();
}
bool checkModify = true;
bool saveAll = false;
int failedSaves = 0;
for(auto doc : docs) {
auto gdoc = Application::Instance->getDocument(doc);
if(!gdoc)
Expand All @@ -650,19 +651,36 @@ bool MainWindow::closeAllDocuments (bool close)
continue;
bool save = saveAll;
if(!save && checkModify) {
int res = confirmSave(doc->Label.getStrValue().c_str(),this,docs.size()>1);
if(res==0)
int res = confirmSave(doc->Label.getStrValue().c_str(), this, docs.size()>1);
switch (res)
{
case ConfirmSaveResult::Cancel:
return false;
if(res>0) {
case ConfirmSaveResult::SaveAll:
saveAll = true;
case ConfirmSaveResult::Save:
save = true;
if(res==2)
saveAll = true;
} else if(res==-2)
break;
case ConfirmSaveResult::DiscardAll:
checkModify = false;
}
}

if(save && !gdoc->save())
failedSaves++;
}

if (failedSaves > 0) {
int ret = QMessageBox::question(
getMainWindow(),
QObject::tr("%1 Document(s) not saved").arg(QString::number(failedSaves)),
QObject::tr("Some documents could not be saved. Do you want to cancel closing?"),
QMessageBox::Discard | QMessageBox::Cancel,
QMessageBox::Discard);
if (ret == QMessageBox::Cancel)
return false;
}

if(close)
App::GetApplication().closeAllDocuments();
// d->mdiArea->closeAllSubWindows();
Expand Down
7 changes: 7 additions & 0 deletions src/Gui/MainWindow.h
View file
Expand Up @@ -77,6 +77,13 @@ class GuiExport MainWindow : public QMainWindow
Q_OBJECT

public:
enum ConfirmSaveResult {
Cancel = 0,
Save,
SaveAll,
Discard,
DiscardAll
};
/**
* Constructs an empty main window. For default \a parent is 0, as there usually is
* no toplevel window there.
Expand Down

6 comments on commit 820e88f

@AjinkyaDahale
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chennes this commit should definitely have had all the messages from the squashed commits. Ideally, it should probably have stayed as separate commits for App, Gui and Base. I don't believe we can force on master, right?

@chennes
Copy link
Member

@chennes chennes commented on 820e88f Sep 17, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies -- I think it should be possible to edit the commit message, at any rate. The tree had gotten a little messy and I didn't want to pollute the trunk with "working" commits. I thought I'd gotten all the info necessary into the message, but if you can get me a better commit message I believe I can amend it.

ETA - No, I see that I'm wrong here, I can't. I'm sorry for that, I'll try to be more careful going forward.

@AjinkyaDahale
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the policy here should be to just dump in all the commit messages as they are, e.g. PR #4920. Too much information seems better than too little information.

@AjinkyaDahale
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ETA - No, I see that I'm wrong here, I can't. I'm sorry for that, I'll try to be more careful going forward.

I must acknowledge I'm also at fault, leaving caveats like "changes made in multiple modules" in the obscure task list instead of pro-actively making separate PRs.

Would it be worthwhile to push a revert and re-do the commits?

@chennes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the policy here should be to just dump in all the commit messages as they are, e.g. PR #4920. Too much information seems better than too little information.

That seems reasonable -- for the foreseeable future I think I'll just work more closely with PR authors to get commits that I am comfortable committing as-is, rather than squashing them myself.

Regarding rolling it back and re-doing, I can't really make that call, only you can. Since what I was trying to avoid was a messy commit history, a rollback is counter to my goals, but if there's important information missing from the commit message, then we can do it.

@yorikvanhavre
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't know if it's worth the hassle redoing the whole thing just for cleanliness... That would be neither the first nor the last time this happens... I would let it be, and do our best next time :)

Please sign in to comment.