[Development] QFile: writing via a temporary file

[David Faure]

On Friday 06 January 2012 19:09:26 Thiago Macieira wrote:
> On Friday, 6 de January de 2012 21.38.19, David Faure wrote:
> > > The first solution doesn't look nice. It would have to fail opening
> > > completely.
> > 
> > Well, this is just like using ReadOnly | Truncate, for instance. These are
> > incompatible. What happens? Truncate will be ignored, and you don't even
> > get a warning. (Both the code and a quick test indicate this).
> 
> I'd have expected that to give you a read-only 0-bytes file.

Nope, ReadOnly isn't supposed to make changes to the file ;-)

> In any case, both flags are specified at the same time. If we're going to go
> with this feature in QFile, then the flag should be specified in the call to
> open().

Yes, everyone agrees on that, then.

> > > But I think I prefer a QSaveFile, derived from QFile, with the proper
> > > semantics.
> > 
> > Heh, a 4th solution, to make this debate even more endless :-)
> 
> Fourth? I thought it was your first. I considered "separate class from
> QFile" and "QFile" as the two options.

Ah, but my "separate class" initial idea was the KDE/QtCreator idea, i.e. a 
class that isn't a QFile. So deriving from QFile is a new idea :) But indeed a 
better one than the KSaveFile case which requires the developer to handle two 
different class instances. So let's forget about "separate and not deriving".

> > Why would this be better? It can't be "because it's not clear which file
> > some methods should work on" (Joao's argument), since it would still have
> > all the QFile methods. It can't be because we need additional API compared
> > to QFile, or is it, for one single rollback method? ;)
> 
> QTemporaryFile derives from QFile. I think a QSaveFile is the same as a
> QTemporaryFile, except it's not temporary: when you close it, instead of it
> going "poof!", it gets made permanent.
> 
> The code that does the writing doesn't need to know that it's operating on a
> regular QFile, a QTemporaryFile or a QSaveFile.

I used to disagree, for error handling reasons, but in fact there are many way 
to do the error handling, and the best way would be inside QFile itself.

Otherwise, with the current proposal, we would have to write code like this:

class C
{
  C(QFile* file) : m_file(file) {}
  QFile *m_file;
};

One solution:

bool C::writeMoreData(const QByteArray& data) {
  if (m_file->write(data) != data.size()) {
    return false;
  }
  return true;
}

And the caller, which creates C(new QSaveFile), then has to do
if (!c.writeMoreData(data)) {
  m_saveFile->rollback();
}

A much better solution would be:

void C::writeMoreData(const QByteArray& data) {
  m_file->write(data);
}

And inside QFile::writeData(), we set a bool m_writeError if write couldn't 
write data.size() bytes. And this way in finalize()/close() we call rollback if 
an error happened. I think this is what Oswald called "latching errors".

The existing calls to setError in writeData and flush are not helping because
1) they don't detect partial writes (ret < len)
2) the error code is cleared on the next call to writeData.
So I think a new bool is needed, set by setError, and not reset until closing 
the file.

And this way we can make close() (and the destructor) do the right thing: it 
will commit (atomic rename) if there were no errors during writing, and it 
will rollback (delete temp file), if there were any errors during writing.

I think we focused the discussion too much on "forcing the developer to
confirm that he wants the file saved", which doesn't make sense to me.
If there were no errors, then yes you want it saved, of course. So if
QFile can know by itself, this makes the feature a lot easier to use.


Now that I think of it, this could be done in QFile itself or in a subclass, 
just the same. Which still leaves me with two possible designs:

A) QFile subclass, with commit() and rollback() API.

B) QFile builtin mode.
Thiago's emails made me realize that this solution is only valid if no 
additional API methods are needed (since otherwise we have methods that don't 
make sense unless we're in a specific mode, so better put them in a subclass).
But do we still need additional methods? The developer can ask close() to
roll back by simply calling setError() on the file before closing, for errors 
that QFile couldn't detect itself.

I want to make it correct, but also easy to use, for the simple case.
So I prefer B, using QFile and setting a flag. Shall I put that on gerrit or 
are there objections on the idea itself?

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5