D14890: Remove QSaveFile in favor of plain old file saving
This revision was not accepted when it landed; it landed in state "Changes Planned". This revision was automatically updated to reflect the committed changes. Closed by commit R39:38870f3b3173: correct error handling for QFileDevice and KCompressedDevice (authored by cullmann). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D14890?vs=39962=39965#toc REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14890?vs=39962=39965 REVISION DETAIL https://phabricator.kde.org/D14890 AFFECTED FILES src/buffer/katetextbuffer.cpp To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann added a comment. My local patch already does that :) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
dfaure added a comment. 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... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann added a comment. 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::Value, REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann added a comment. Thanks! REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
dfaure added a comment. I'll look into it. I also just reported https://bugreports.qt.io/browse/QTBUG-70033 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann added a comment. > 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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
dfaure added a comment. (the alternative is to use Unbuffered on those devices, but that's probably not a great idea performance wise) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann added a comment.
For the QFile case, I added now this check:
// did save work?
// FIXME for KCompressionDevice
if (qobject_cast(saveFile.data()) &&
qobject_cast(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.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D14890
To: cullmann, dhaumann, dfaure
Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham,
bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
dfaure added a comment. 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()... REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann added a comment. 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? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann planned changes to this revision. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann added a comment.
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;
+//}
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D14890
To: cullmann, dhaumann, dfaure
Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham,
bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann reopened this revision. cullmann added a comment. This revision is now accepted and ready to land. The errorString().isEmpty doesn't work, you get unknown error even for the non-errors cases. QFile would allow to check for the set errorCode. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
This revision was automatically updated to reflect the committed changes. Closed by commit R39:681cafb74607: Remove QSaveFile in favor of plain old file saving (authored by cullmann). REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14890?vs=39956=39962 REVISION DETAIL https://phabricator.kde.org/D14890 AFFECTED FILES src/buffer/katetextbuffer.cpp src/document/katedocument.cpp src/export/exporter.cpp To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
dhaumann accepted this revision. dhaumann added a comment. This revision is now accepted and ready to land. Looks reasonable to me. REPOSITORY R39 KTextEditor BRANCH nosavefile (branched from master) REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann updated this revision to Diff 39956. cullmann added a comment. - better error handling, take a look at the file error string REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14890?vs=39921=39956 BRANCH nosavefile (branched from master) REVISION DETAIL https://phabricator.kde.org/D14890 AFFECTED FILES src/buffer/katetextbuffer.cpp src/document/katedocument.cpp src/export/exporter.cpp To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
dfaure added a comment. Fix for KCompressionDevice + addendum to this patch posted on https://bugs.kde.org/show_bug.cgi?id=397545 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann updated this revision to Diff 39921. cullmann added a comment. skip filter dev hanlding in the common case of no compression compress in memory to the buffer for the kauth case this actually leads now to the wanted error message for uncompressed files in disk full situations without any segfault for the special case of compressed files I opened bug 397545 - Merge branch 'master' into nosavefile REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14890?vs=39903=39921 BRANCH nosavefile (branched from master) REVISION DETAIL https://phabricator.kde.org/D14890 AFFECTED FILES src/buffer/katetextbuffer.cpp src/document/katedocument.cpp src/export/exporter.cpp To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann added a comment. 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 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann requested review of this revision. cullmann added a comment. There is one tiny problem with that change: The compression filter device crashs sometimes, if you have bad luck, e.g. tried like David did show above with a small tmp mount: ASSERT: "d->avail_out > 0" in file /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp, line 128 Thread 1 "kwrite" received signal SIGABRT, Aborted. 0x7156708b in raise () from /lib64/libc.so.6 (gdb) bt #0 0x7156708b in raise () from /lib64/libc.so.6 #1 0x715504e9 in abort () from /lib64/libc.so.6 #2 0x722b022b in QMessageLogger::fatal(char const*, ...) const () from /usr/lib64/libQt5Core.so.5 #3 0x722af7d9 in qt_assert(char const*, char const*, int) () from /usr/lib64/libQt5Core.so.5 #4 0x704b44fb in KNoneFilter::copyData (this=0x7fffdc0278f0) at /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp:128 #5 0x704b44c6 in KNoneFilter::compress (this=0x7fffdc0278f0, finish=true) at /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp:123 #6 0x704b277e in KCompressionDevice::writeData (this=0x7fffc220, data=0x0, len=0) at /home/cullmann/kde/src/frameworks/karchive/src/kcompressiondevice.cpp:343 #7 0x723d in QIODevice::write(char const*, long long) () from /usr/lib64/libQt5Core.so.5 #8 0x704b1f1d in KCompressionDevice::close (this=0x7fffc220) at /home/cullmann/kde/src/frameworks/karchive/src/kcompressiondevice.cpp:165 #9 0x777ee7fd in Kate::TextBuffer::save (this=0x72a930, filename=...) at /home/cullmann/kde/src/frameworks/ktexteditor/src/buffer/katetextbuffer.cpp:858 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cullmann updated this revision to Diff 39903. cullmann added a comment. - improve the error message, add vim like hint that one might loose data - Merge branch 'master' into nosavefile REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14890?vs=39884=39903 BRANCH nosavefile (branched from master) REVISION DETAIL https://phabricator.kde.org/D14890 AFFECTED FILES src/buffer/katetextbuffer.cpp src/document/katedocument.cpp src/export/exporter.cpp To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R39 KTextEditor BRANCH nosavefile (branched from master) REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann, dfaure Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: Remove QSaveFile in favor of plain old file saving
cfeck retitled this revision from "remove QSaveFile in favor of plain old file saving rational: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it should other ways would be start to add workarounds to..." to "Remove QSaveFile in favor of plain old file saving". cfeck edited the summary of this revision. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: remove QSaveFile in favor of plain old file saving rational: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it sh
cullmann added a comment. That sounds reasonable. The hint is actually very good, at the moment in the best case the user get some "saving failed" message like: 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. 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. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: remove QSaveFile in favor of plain old file saving rational: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it sh
dfaure added a comment. 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?) WARNING: Original file may be lost or damaged don't quit the editor until the file is successfully written! 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 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: remove QSaveFile in favor of plain old file saving rational: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it sh
cullmann added a comment. 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). REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: remove QSaveFile in favor of plain old file saving rational: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it sh
dfaure added a comment. 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? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann Cc: dfaure, kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: remove QSaveFile in favor of plain old file saving rational: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it sh
cullmann added a comment. Relevant bugs bug 379818 bug 333577 bug 366583 bug 354405 bug 358457 REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D14890 To: cullmann, dhaumann Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14890: remove QSaveFile in favor of plain old file saving rational: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it sh
cullmann created this revision. cullmann added a reviewer: dhaumann. Herald added projects: Kate, Frameworks. Herald added subscribers: kde-frameworks-devel, kwrite-devel. cullmann requested review of this revision. REVISION SUMMARY ...all cases, which is hard, e.g. if that is something shared via SMB... TEST PLAN make && make test REPOSITORY R39 KTextEditor BRANCH nosavefile (branched from master) REVISION DETAIL https://phabricator.kde.org/D14890 AFFECTED FILES src/buffer/katetextbuffer.cpp src/export/exporter.cpp To: cullmann, dhaumann Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
