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
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, dha
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: dfau
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/cu
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
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,
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 e
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-de
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
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
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: d
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
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->error
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 D
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&id=39962
R
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:
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&id=39956
BRANCH
nosavefile (branched from master)
REVI
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,
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 w
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.kd
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
/ho
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&id=39
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, k
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 Q
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 wri
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 th
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 th
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, kwr
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,
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 v
30 matches
Mail list logo
