Differential D4847 Diff 12185 src/buffer/katetextbuffer.cpp

Changeset View

Standalone View

View Options

src/buffer/katetextbuffer.cpp

Context not available.
19 */ 19​ */
20 20​
21#include "config.h" 21​#include "config.h"
22​#include "kateglobal.h"
22 23​
23#include "katetextbuffer.h" 24​#include "katetextbuffer.h"
25​#include "katesecuretextbuffer.h"
24#include "katetextloader.h" 26​#include "katetextloader.h"
25 27​
26// this is unfortunate, but needed for performance 28​// this is unfortunate, but needed for performance
Context not available.
37#endif 39​#endif
38 40​
39#include <QSaveFile> 41​#include <QSaveFile>
42​#include <QTemporaryFile>
43​#include <QFileDevice>
44​#include <QFileInfo>
40 45​
41#if 0 46​#if 0
42#define BUFFER_DEBUG qCDebug(LOG_KTE) 47​#define BUFFER_DEBUG qCDebug(LOG_KTE)
Context not available.
47namespace Kate 52​namespace Kate
48{ 53​{
49 54​
50TextBuffer::TextBuffer(KTextEditor::DocumentPrivate *parent, int blockSize) 55​TextBuffer::TextBuffer(KTextEditor::DocumentPrivate *parent, int blockSize, bool alwaysUseKAuth)
51 : QObject(parent) 56​ : QObject(parent)
52 , m_document(parent) 57​ , m_document(parent)
53 , m_history(*this) 58​ , m_history(*this)
Context not available.
67 , m_endOfLineMode(eolUnix) 72​ , m_endOfLineMode(eolUnix)
68 , m_newLineAtEof(false) 73​ , m_newLineAtEof(false)
69 , m_lineLengthLimit(4096) 74​ , m_lineLengthLimit(4096)
75​ , m_alwaysUseKAuthForSave(KTextEditor::EditorPrivate::unitTestMode() ? alwaysUseKAuth : false)
dfaureUnsubmitted
Done

Isn't it weird to ignore the constructor argument in some cases?

dfaure: Isn't it weird to ignore the constructor argument in some cases?
70{ 76​{
71 // minimal block size must be > 0 77​ // minimal block size must be > 0
72 Q_ASSERT(m_blockSize > 0); 78​ Q_ASSERT(m_blockSize > 0);
dhaumannUnsubmitted
Done

const QString textCodec = ...
const int eolMode = ...

Please use const whenever possible for locally defined variables.

dhaumann: const QString textCodec = ... const int eolMode = ... Please use const whenever possible for…
Context not available.
763 // codec must be set! 769​ // codec must be set!
764 Q_ASSERT(m_textCodec); 770​ Q_ASSERT(m_textCodec);
765 771​
772​ uint ownerId = -2;
773​ uint groupId = -2;
774​
766#ifndef Q_OS_WIN 775​#ifndef Q_OS_WIN
767 const bool newFile = !QFile::exists(filename); 776​ const bool newFile = !QFile::exists(filename);
777​
778​ /**
779​ * Memorize owner and group. Due to design of QSaveFile we will have to re-set them after save is complete.
780​ */
781​ if (!newFile) {
782​ QFileInfo fileInfo(filename);
783​ ownerId = fileInfo.ownerId();
784​ groupId = fileInfo.groupId();
785​ }
768#endif 786​#endif
769 787​
770 /** 788​ /**
771 * use QSaveFile for save write + rename 789​ * use QSaveFile for save write + rename
772 */ 790​ */
773 QSaveFile saveFile(filename); 791​ QScopedPointer<QFileDevice> saveFile(new QSaveFile(filename));
anthonyfieroniUnsubmitted
Done

use QScopedPointer, remove manual delete

anthonyfieroni: use QScopedPointer, remove manual delete
774 saveFile.setDirectWriteFallback(true); 792​ static_cast<QSaveFile *>(saveFile.data())->setDirectWriteFallback(true);
775 793​
776 if (!saveFile.open(QIODevice::WriteOnly | QIODevice::Truncate)) { 794​ bool usingTemporaryFile = false;
777 return false; 795​
796​ // open QSaveFile for write
797​ if (m_alwaysUseKAuthForSave || !saveFile->open(QIODevice::WriteOnly | QIODevice::Truncate)) {
798​ saveFile.reset(new QTemporaryFile());
799​ usingTemporaryFile = true;
800​
801​ // open QTemporaryFile for write
802​ if (!saveFile->open(QIODevice::WriteOnly | QIODevice::Truncate)) {
803​ return false;
804​ }
805​
806​#ifndef Q_OS_WIN
807​ // set original file's permissions
dfaureUnsubmitted
Done

The thing about using Qt's setPermissions() is that it should work on Windows too.

dfaure: The thing about using Qt's setPermissions() is that it should work on Windows too.
808​ if (!newFile) {
809​ saveFile->setPermissions(QFile(filename).permissions());
810​ }
811​#endif
778 } 812​ }
779 813​
780 /** 814​ /**
781 * construct correct filter device and try to open 815​ * construct correct filter device and try to open
dfaureUnsubmitted
Done

Doesn't QSaveFile do this?

dfaure: Doesn't QSaveFile do this?
782 */ 816​ */
783 KCompressionDevice::CompressionType type = KFilterDev::compressionTypeForMimeType(m_mimeTypeForFilterDev); 817​ KCompressionDevice::CompressionType type = KFilterDev::compressionTypeForMimeType(m_mimeTypeForFilterDev);
784 KCompressionDevice file(&saveFile, false, type); 818​ KCompressionDevice file(saveFile.data(), false, type);
785 819​
786 if (!file.open(QIODevice::WriteOnly)) { 820​ if (!file.open(QIODevice::WriteOnly)) {
787 return false; 821​ return false;
Context not available.
836 file.close(); 870​ file.close();
837 871​
838 // flush file 872​ // flush file
839 if (!saveFile.flush()) { 873​ if (!saveFile->flush()) {
840 return false; 874​ return false;
841 } 875​ }
842 876​
843#ifndef Q_OS_WIN 877​#ifndef Q_OS_WIN
844 // ensure that the file is written to disk 878​ // ensure that the file is written to disk
845#ifdef HAVE_FDATASYNC 879​#ifdef HAVE_FDATASYNC
846 fdatasync(saveFile.handle()); 880​ fdatasync(saveFile->handle());
dfaureUnsubmitted
Done

(pre-existing) done by QSaveFile already

dfaure: (pre-existing) done by QSaveFile already
847#else 881​#else
848 fsync(saveFile.handle()); 882​ fsync(saveFile->handle());
849#endif 883​#endif
850#endif 884​#endif
851 885​
852 // did save work? 886​ // did save work?
anthonyfieroniUnsubmitted
Done

Should be -> instead of .

anthonyfieroni: Should be -> instead of .
853 // only finalize if stream status == OK 887​ // only finalize if stream status == OK
854 bool ok = (stream.status() == QTextStream::Ok) && saveFile.commit(); 888​ bool ok = (stream.status() == QTextStream::Ok);
855 889​
856 // remember this revision as last saved if we had success! 890​ // commit changes
857 if (ok) { 891​ if (ok) {
858 m_history.setLastSavedRevision(); 892​
859 } 893​ if (usingTemporaryFile) {
894​
895​ // temporary file was used to save the file
896​ // -> moving this file to original location with KAuth action
897​
898​ QVariantMap args;
899​ args[QLatin1String("sourceFile")] = saveFile->fileName();
dfaureUnsubmitted
Done

use .insert() to avoid the creation of a temporary.

dfaure: use .insert() to avoid the creation of a temporary.
apolUnsubmitted
Not Done

Or better use an initializer list?

apol: Or better use an initializer list?
900​ args[QLatin1String("targetFile")] = filename;
901​ args[QLatin1String("ownerId")] = ownerId;
902​ args[QLatin1String("groupId")] = groupId;
903​
904​ // call save with elevated privileges
905​ if (KTextEditor::EditorPrivate::unitTestMode()) {
906​ ok = SecureTextBuffer::movefileAction(args).succeeded();
907​ } else {
908​ KAuth::Action saveAction(QLatin1String("org.kde.ktexteditor.katetextbuffer.movefile"));
909​ saveAction.setHelperId(QLatin1String("org.kde.ktexteditor.katetextbuffer"));
910​ saveAction.setArguments(args);
911​ KAuth::ExecuteJob *job = saveAction.execute();
912​ ok = job->exec();
dfaureUnsubmitted
Not Done

Nested event loops are evil. Any chance to make this asynchronous (connecting to a slot instead, for anything that needs to be done after completion of the job) ?

dfaure: Nested event loops are evil. Any chance to make this asynchronous (connecting to a slot instead…
913​ }
914​
915​ } else {
916​
917​ // standard save without elevated privileges
918​
919​ ok = static_cast<QSaveFile *>(saveFile.data())->commit();
860 920​
861#ifndef Q_OS_WIN 921​#ifndef Q_OS_WIN
862 if (ok && newFile) { // QTemporaryFile sets permissions to 0600, so fixing this 922​ if (ok) {
863 const mode_t mask = umask(0); 923​ SecureTextBuffer::setUnixPermissionsAndOwner(filename, newFile, ownerId, groupId);
864 umask(mask); 924​ }
925​#endif
865 926​
866 const mode_t fileMode = 0666 & ~mask; 927​ }
867 chmod(QFile::encodeName(filename).constData(), fileMode); 928​ }
929​
930​ // remember this revision as last saved if we had success!
931​ if (ok) {
932​ m_history.setLastSavedRevision();
868 } 933​ }
869#endif ​
870 934​
871 // report CODEC + ERRORS 935​ // report CODEC + ERRORS
872 BUFFER_DEBUG << "Saved file " << filename << "with codec" << m_textCodec->name() << (ok ? "without" : "with") << "errors"; 936​ BUFFER_DEBUG << "Saved file " << filename << "with codec" << m_textCodec->name() << (ok ? "without" : "with") << "errors";
Context not available.