Re: [patch] improve error output

[Georg Baum]

Abdelrazak Younes wrote:

I Abdel,

I'll do what you suggested, with one exception:

> LYXERR0("FileName::copyTo(): Could not copy file " << *this << " to " <<
> name); sting const error = fromqstr(f.errorString());
> if (!error.empty())
>  LYXERR0(": " << error);

This would add an unwanted line break.


Georg

[patch] improve error output

[Georg Baum]

The investigation of bug 9139 showed that the error message we give when a 
file operation fails is not too clever. The attached patch improves this. It 
is still not optimal (since qt has a very limited set of error causes that 
are reported), but if we want to get the real error from the OS we have to 
implement it in a platform specific way.

In my experience error messages that are as exact as possible help a lot 
when investigating bug reports: You need to ask less questions, since the 
bug report itself will contain more information. OK to go in?

Georgdiff --git a/src/support/FileName.cpp b/src/support/FileName.cpp
index 950..3423972 100644
--- a/src/support/FileName.cpp
+++ b/src/support/FileName.cpp
@@ -238,10 +238,17 @@ bool FileName::copyTo(FileName const & name, bool keepsymlink,
 		return copyTo(target, true);
 	}
 	QFile::remove(name.d->fi.absoluteFilePath());
-	bool success = QFile::copy(d->fi.absoluteFilePath(), name.d->fi.absoluteFilePath());
-	if (!success)
-		LYXERR0("FileName::copyTo(): Could not copy file "
-			<< *this << " to " << name);
+	QFile f(d->fi.absoluteFilePath());
+	bool const success = f.copy(name.d->fi.absoluteFilePath());
+	if (!success) {
+		QString const error(f.errorString());
+		if (error.isEmpty())
+			LYXERR0("FileName::copyTo(): Could not copy file "
+<< *this << " to " << name);
+		else
+			LYXERR0("FileName::copyTo(): Could not copy file "
+<< *this << " to " << name << ": " << fromqstr(error));
+	}
 	return success;
 }
 
@@ -249,9 +256,16 @@ bool FileName::copyTo(FileName const & name, bool keepsymlink,
 bool FileName::renameTo(FileName const & name) const
 {
 	LYXERR(Debug::FILES, "Renaming " << name << " as " << *this);
-	bool success = QFile::rename(d->fi.absoluteFilePath(), name.d->fi.absoluteFilePath());
-	if (!success)
-		LYXERR0("Could not rename file " << *this << " to " << name);
+	QFile f(d->fi.absoluteFilePath());
+	bool const success = f.rename( name.d->fi.absoluteFilePath());
+	if (!success) {
+		QString const error(f.errorString());
+		if (error.isEmpty())
+			LYXERR0("Could not rename file " << *this << " to " << name);
+		else
+			LYXERR0("Could not rename file " << *this << " to "
+<< name << ": " << fromqstr(error));
+	}
 	return success;
 }
 
@@ -261,10 +275,16 @@ bool FileName::moveTo(FileName const & name) const
 	LYXERR(Debug::FILES, "Moving " << name << " to " << *this);
 	QFile::remove(name.d->fi.absoluteFilePath());
 
-	bool success = QFile::rename(d->fi.absoluteFilePath(),
-		name.d->fi.absoluteFilePath());
-	if (!success)
-		LYXERR0("Could not move file " << *this << " to " << name);
+	QFile f(d->fi.absoluteFilePath());
+	bool const success = f.rename(name.d->fi.absoluteFilePath());
+	if (!success) {
+		QString const error(f.errorString());
+		if (error.isEmpty())
+			LYXERR0("Could not move file " << *this << " to " << name);
+		else
+			LYXERR0("Could not move file " << *this << " to "
+<< name << ": " << fromqstr(error));
+	}
 	return success;
 }
 

Re: [patch] improve error output

[Abdelrazak Younes]

Hi Georg,

Few nitpicks inline.

On 29/11/2015 18:53, Georg Baum wrote:

The investigation of bug 9139 showed that the error message we give when a
file operation fails is not too clever. The attached patch improves this. It
is still not optimal (since qt has a very limited set of error causes that
are reported), but if we want to get the real error from the OS we have to
implement it in a platform specific way.

In my experience error messages that are as exact as possible help a lot
when investigating bug reports: You need to ask less questions, since the
bug report itself will contain more information.


Fully agreed.


  OK to go in?

Georg
diff --git a/src/support/FileName.cpp b/src/support/FileName.cpp
index 950..3423972 100644
--- a/src/support/FileName.cpp
+++ b/src/support/FileName.cpp
@@ -238,10 +238,17 @@ bool FileName::copyTo(FileName const & name, bool 
keepsymlink,
return copyTo(target, true);
}
QFile::remove(name.d->fi.absoluteFilePath());
-   bool success = QFile::copy(d->fi.absoluteFilePath(), 
name.d->fi.absoluteFilePath());
-   if (!success)
-   LYXERR0("FileName::copyTo(): Could not copy file "
-   << *this << " to " << name);
+   QFile f(d->fi.absoluteFilePath());
+   bool const success = f.copy(name.d->fi.absoluteFilePath());


if (f.copy(name.d->fi.absoluteFilePath()))
return true;


+   if (!success) {

remove


+   QString const error(f.errorString());
+   if (error.isEmpty())
+   LYXERR0("FileName::copyTo(): Could not copy file "
+   << *this << " to " << name);
+   else
+   LYXERR0("FileName::copyTo(): Could not copy file "
+   << *this << " to " << name << ": " << 
fromqstr(error));
+   }


LYXERR0("FileName::copyTo(): Could not copy file " << *this << " to " << name);
sting const error = fromqstr(f.errorString());
if (!error.empty())
LYXERR0(": " << error);



return success;


return false;

Same comments for renameTo() and moveTo()

Cheers,
Abdel.

PS: congratz to all for LyX 2.2 alpha delivery, looks very promising!