Relevant bugs

bug 379818
bug 333577
bug 366583
bug 354405
bug 358457
bug 385596

But what about the issue where you try to save over an existing file, and the partition is full (or the app crashes), and the user loses the file?

Actually, that the application crashs during that actions is highly unlikely, I can't remember any crash during save being reported in the last 5 years.
The partition full is more ugly, that is true.
(and in any case, for sure is direct writing more unsafe than the write in a copy)

But I don't see that we will be able to workaround all the issues people face with the trivial save-as-copy and rename scheme we use.
Actually having the wrong ACLs can be very bad if you have that in your network share and suddenly the rights change.
Same for the static link stuff.
I looked a bit how other editors handle that, but that is more or less very non-trivial (and I am not sure I have seen senseful handling on Windows).

I just tested how vim handles this. No QSaveFile-like behaviour (direct write, says strace), and in case of partition full, this error message appears:

"myfile" E514: write error (file system full?)

That's the beautiful difference between a text editor and a program saving a config file or whatever else: you can leave the editor open so the data is not lost!
I recommend that kate implements something similar, if not already done.

For the curious, here's how I investigated vim's behaviour without filling my real partition:

cd /tmp
dd if=/dev/zero of=loopback count=200
sudo mkfs.ext2 loopback
sudo mount -o loop loopback mnt
sudo chown $USER:$GID mnt
cd mnt
yes > myfile
  # (this ends with "No space left on device")
vim myfile

# elsewhere: strace -e file -p `pidof vim`

in vim, paste a whole bunch of additional text into the file, save

That sounds reasonable.
The hint is actually very good, at the moment in the best case the user get some "saving failed" message like:

<snip>
The document could not be saved, as it was not possible to write to /XML-slowdown-KWRITE.xml.

Check that you have write access to this file or that enough disk space is available.
</snip>

An additional hint the "WARNING: Original file may be lost or damaged don't quit the editor until the file is successfully written!" would make that message more clear for the average user what the implications are.

I think the integration must wait until we resolve the KArchive issue.
I have taken a short look but seen no "trivial" fix, opened a new bug for that, https://bugs.kde.org/show_bug.cgi?id=397545

Fix for KCompressionDevice + addendum to this patch posted on https://bugs.kde.org/show_bug.cgi?id=397545

Atm reverted the last addition of

// did save work?

• if (!saveFile->errorString().isEmpty()) {
• BUFFER_DEBUG << "Saving file " << filename << "failed with error" << saveFile->errorString();
• return false;
• }

+ FIXME if (!saveFile->errorString().isEmpty()) {
+ BUFFER_DEBUG << "Saving file " << filename << "failed with error" << saveFile->errorString();
+ return false;
+ }

For the QFile case, I could query http://doc.qt.io/qt-5/qfiledevice.html#error, but for the FilterDev case I have no such function available, or?

Yeah that's the problem.

The way I see it, the QIODevice API assumes that one will call errorString() only after some method returns an error, e.g. after QAbstractSocket::error() is emitted, or after socket.waitForEncrypted() returns false.

QFile[Device] and QAbstractSocket add their own errorcode enums on top of that, but that's not part of the base QIODevice API, which is all we have in KCompressionDevice.

Ideally QIODevice::close() would return a bool, and I'm more and more thinking that this should be done for Qt 6.
But pending that, I'm afraid that we'll have to downcast to KCompressionDevice to call a new error() accessor after close()...

For the QFile case, I added now this check:

// did save work?
  // FIXME for KCompressionDevice
  if (qobject_cast<QFileDevice *>(saveFile.data()) && qobject_cast<QFileDevice *>(saveFile.data())->error() != QFileDevice::NoError) {
      BUFFER_DEBUG << "Saving file " << filename << "failed with error" << saveFile->errorString();
      return false;
  }

that seems to handle the QFile case well.

(the alternative is to use Unbuffered on those devices, but that's probably not a great idea performance wise)

But pending that, I'm afraid that we'll have to downcast to KCompressionDevice to call a new error() accessor after close()...

I can live with that, see comment above, I already added the QFileDevice case.
It is not that nice but at least one can handle the error.
If you add the missing API, I will adapt the code here.

I'll look into it. I also just reported https://bugreports.qt.io/browse/QTBUG-70033

Thanks!

Just wanted to add the alternative code path, might it be that the Q_OBJECT stuff is really missing by accident? Or is this intentional?

/usr/include/qt5/QtCore/qobject.h: In instantiation of ‘T qobject_cast(QObject*) [with T = KCompressionDevice*]’:
/home/cullmann/kde/src/frameworks/ktexteditor/src/buffer/katetextbuffer.cpp:863:82: required from here
/usr/include/qt5/QtCore/qobject.h:502:5: error: static assertion failed: qobject_cast requires the type to have a Q_OBJECT macro

Q_STATIC_ASSERT_X(QtPrivate::HasQ_OBJECT_Macro<ObjType>::Value,

Oops fixed.

BTW, doing a qobject_cast twice (for each type) seems a bit wasteful, you might want to split the if() like I did in KArchive...

My local patch already does that :)