Skip to content

Commit

Permalink
2539: Autosave Aborted Spams Console (TrenchBroom#2541)
Browse files Browse the repository at this point in the history 
* 2539: Code style.

* 2539: Cleanup and documentation.

* 2539: If a path does not exist, make it relative to the first FS in an FS hierarchy.

* 2539: Ignore Xcode-qt project path.

* 2539: Add test case for resolving absolute paths against file system chains.
  • Loading branch information
@kduske
kduske authored Jan 21, 2019
1 parent 3a8baac commit 75b0029
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 88 deletions.
2 changes: 1 addition & 1 deletion .gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Thumbs.db
/build-debug
/build-release
/codeblocks
/Xcode
/Xcode*
/vs2010
/vs2012
/vs2013
Expand Down
28 changes: 15 additions & 13 deletions common/src/IO/FileSystem.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,22 @@ namespace TrenchBroom {
}

bool FileSystem::canMakeAbsolute(const Path& path) const {
if (path.isAbsolute()) {
return false;
} else {
return _canMakeAbsolute(path);
}
return !path.isAbsolute();
}

Path FileSystem::makeAbsolute(const Path& path) const {
try {
if (!canMakeAbsolute(path)) {
throw FileSystemException("Cannot make absolute path of '" + path.asString() + "'");
throw FileSystemException("Cannot make absolute path of: '" + path.asString() + "'");
}

return _makeAbsolute(path);
const auto result = _makeAbsolute(path);
if (!result.isEmpty()) {
return result;
} else {
// The path does not exist in any file system, make it absolute relative to this file system.
return doMakeAbsolute(path);
}
} catch (const PathException& e) {
throw FileSystemException("Invalid path: '" + path.asString() + "'", e);
}
Expand Down Expand Up @@ -137,17 +139,17 @@ namespace TrenchBroom {
}
}

bool FileSystem::_canMakeAbsolute(const Path& path) const {
return doCanMakeAbsolute(path) || (m_next && m_next->_canMakeAbsolute(path));
}

Path FileSystem::_makeAbsolute(const Path& path) const {
if (doCanMakeAbsolute(path)) {
if (doFileExists(path) || doDirectoryExists(path)) {
// If the file is present in this file system, make it absolute here.
return doMakeAbsolute(path);
} else if (m_next) {
// Otherwise, try the next one.
return m_next->_makeAbsolute(path);
} else {
throw FileSystemException("Path does not exist: '" + path.asString() + "'");
// Otherwise, the file does not exist in any file system in this hierarchy.
// Return the empty path.
return Path();
}
}

Expand Down
123 changes: 63 additions & 60 deletions common/src/View/Autosaver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,16 @@
#include "Autosaver.h"

#include "StringUtils.h"
#include "TemporarilySetAny.h"
#include "IO/DiskFileSystem.h"
#include "View/MapDocument.h"

#include <algorithm>
#include <cassert>

namespace TrenchBroom {
namespace View {
Autosaver::Autosaver(View::MapDocumentWPtr document, const time_t saveInterval, const time_t idleInterval, const size_t maxBackups) :
Autosaver::Autosaver(View::MapDocumentWPtr document, const std::time_t saveInterval, const std::time_t idleInterval, const size_t maxBackups) :
m_document(document),
m_logger(nullptr),
m_saveInterval(saveInterval),
m_idleInterval(idleInterval),
m_maxBackups(maxBackups),
Expand All @@ -42,74 +41,76 @@ namespace TrenchBroom {

Autosaver::~Autosaver() {
unbindObservers();
triggerAutosave(nullptr);
NullLogger logger;
triggerAutosave(logger);
}

void Autosaver::triggerAutosave(Logger* logger) {
const time_t currentTime = time(nullptr);
void Autosaver::triggerAutosave(Logger& logger) {
const auto currentTime = std::time(nullptr);

MapDocumentSPtr document = lock(m_document);
if (!document->modified())
auto document = lock(m_document);
if (!document->modified()) {
return;
if (document->modificationCount() == m_lastModificationCount)
}
if (document->modificationCount() == m_lastModificationCount) {
return;
if (currentTime - m_lastModificationTime < m_idleInterval)
}
if (currentTime - m_lastModificationTime < m_idleInterval) {
return;
if (currentTime - m_lastSaveTime < m_saveInterval)
}
if (currentTime - m_lastSaveTime < m_saveInterval) {
return;
}

const IO::Path documentPath = document->path();
if (!documentPath.isAbsolute())
const auto documentPath = document->path();
if (!documentPath.isAbsolute()) {
return;
if (!IO::Disk::fileExists(IO::Disk::fixPath(document->path())))
}
if (!IO::Disk::fileExists(IO::Disk::fixPath(document->path()))) {
return;

TemporarilySetAny<Logger*> setLogger(m_logger, logger);
autosave(document);
}

autosave(logger, document);
}

void Autosaver::autosave(MapDocumentSPtr document) {
const IO::Path& mapPath = document->path();
void Autosaver::autosave(Logger& logger, MapDocumentSPtr document) {
const auto& mapPath = document->path();
assert(IO::Disk::fileExists(IO::Disk::fixPath(mapPath)));

const IO::Path mapFilename = mapPath.lastComponent();
const IO::Path mapBasename = mapFilename.deleteExtension();
const auto mapFilename = mapPath.lastComponent();
const auto mapBasename = mapFilename.deleteExtension();

try {
IO::WritableDiskFileSystem fs = createBackupFileSystem(mapPath);
IO::Path::List backups = collectBackups(fs, mapBasename);
auto fs = createBackupFileSystem(logger, mapPath);
auto backups = collectBackups(fs, mapBasename);

thinBackups(fs, backups);
thinBackups(logger, fs, backups);
cleanBackups(fs, backups, mapBasename);

assert(backups.size() < m_maxBackups);
const size_t backupNo = backups.size() + 1;
const auto backupNo = backups.size() + 1;

const IO::Path backupFilePath = fs.makeAbsolute(makeBackupName(mapBasename, backupNo));
const auto backupFilePath = fs.makeAbsolute(makeBackupName(mapBasename, backupNo));

m_lastSaveTime = time(nullptr);
m_lastSaveTime = std::time(nullptr);
m_lastModificationCount = document->modificationCount();
document->saveDocumentTo(backupFilePath);

if (m_logger != nullptr)
m_logger->info("Created autosave backup at %s", backupFilePath.asString().c_str());

} catch (const FileSystemException&) {
if (m_logger != nullptr)
m_logger->error("Aborting autosave");
logger.info() << "Created autosave backup at " << backupFilePath;
} catch (const FileSystemException& e) {
logger.error() << "Aborting autosave: " << e.what();
}
}

IO::WritableDiskFileSystem Autosaver::createBackupFileSystem(const IO::Path& mapPath) const {
const IO::Path basePath = mapPath.deleteLastComponent();
const IO::Path autosavePath = basePath + IO::Path("autosave");
IO::WritableDiskFileSystem Autosaver::createBackupFileSystem(Logger& logger, const IO::Path& mapPath) const {
const auto basePath = mapPath.deleteLastComponent();
const auto autosavePath = basePath + IO::Path("autosave");

try {
// ensures that the directory exists or is created if it doesn't
return IO::WritableDiskFileSystem(autosavePath, true);
} catch (const FileSystemException& e) {
if (m_logger != nullptr)
m_logger->error("Cannot create autosave directory at %s", autosavePath.asString().c_str());
logger.error() << "Cannot create autosave directory at " << autosavePath;
throw e;
}
}
Expand All @@ -121,17 +122,20 @@ namespace TrenchBroom {
mapBasename(i_mapBasename) {}

bool operator()(const IO::Path& path, const bool directory) const {
if (directory)
if (directory) {
return false;
if (!StringUtils::caseInsensitiveEqual(path.extension(), "map"))
}
if (!StringUtils::caseInsensitiveEqual(path.extension(), "map")) {
return false;

const IO::Path backupName = path.lastComponent().deleteExtension();
const IO::Path backupBasename = backupName.deleteExtension();
if (backupBasename != mapBasename)
}

const auto backupName = path.lastComponent().deleteExtension();
const auto backupBasename = backupName.deleteExtension();
if (backupBasename != mapBasename) {
return false;

const size_t no = StringUtils::stringToSize(backupName.extension());
}

const auto no = StringUtils::stringToSize(backupName.extension());
return no > 0;

}
Expand All @@ -143,34 +147,33 @@ namespace TrenchBroom {
}

IO::Path::List Autosaver::collectBackups(const IO::WritableDiskFileSystem& fs, const IO::Path& mapBasename) const {
IO::Path::List backups = fs.findItems(IO::Path(""), BackupFileMatcher(mapBasename));
auto backups = fs.findItems(IO::Path(""), BackupFileMatcher(mapBasename));
std::sort(std::begin(backups), std::end(backups), compareBackupsByNo);
return backups;
}

void Autosaver::thinBackups(IO::WritableDiskFileSystem& fs, IO::Path::List& backups) const {
void Autosaver::thinBackups(Logger& logger, IO::WritableDiskFileSystem& fs, IO::Path::List& backups) const {
while (backups.size() > m_maxBackups - 1) {
const IO::Path filename = backups.front();
const auto filename = backups.front();
try {
fs.deleteFile(filename);
if (m_logger != nullptr)
m_logger->debug("Deleted autosave backup %s", filename.asString().c_str());
logger.debug() << "Deleted autosave backup " << filename;
backups.erase(std::begin(backups));
} catch (const FileSystemException& e) {
if (m_logger != nullptr)
m_logger->error("Cannot delete autosave backup %s", filename.asString().c_str());
logger.error() << "Cannot delete autosave backup " << filename;
throw e;
}
}
}

void Autosaver::cleanBackups(IO::WritableDiskFileSystem& fs, IO::Path::List& backups, const IO::Path& mapBasename) const {
for (size_t i = 0; i < backups.size(); ++i) {
const IO::Path& oldName = backups[i].lastComponent();
const IO::Path newName = makeBackupName(mapBasename, i + 1);
const auto& oldName = backups[i].lastComponent();
const auto newName = makeBackupName(mapBasename, i + 1);

if (oldName != newName)
if (oldName != newName) {
fs.moveFile(oldName, newName, false);
}
}
}

Expand All @@ -181,25 +184,25 @@ namespace TrenchBroom {
}

size_t extractBackupNo(const IO::Path& path) {
const size_t no = StringUtils::stringToSize(path.deleteExtension().extension());
const auto no = StringUtils::stringToSize(path.deleteExtension().extension());
assert(no > 0);
return no;
}

void Autosaver::bindObservers() {
MapDocumentSPtr document = lock(m_document);
auto document = lock(m_document);
document->documentModificationStateDidChangeNotifier.addObserver(this, &Autosaver::documentModificationCountDidChangeNotifier);
}

void Autosaver::unbindObservers() {
if (!expired(m_document)) {
MapDocumentSPtr document = lock(m_document);
auto document = lock(m_document);
document->documentModificationStateDidChangeNotifier.removeObserver(this, &Autosaver::documentModificationCountDidChangeNotifier);
}
}

void Autosaver::documentModificationCountDidChangeNotifier() {
m_lastModificationTime = time(nullptr);
m_lastModificationTime = std::time(nullptr);
}
}
}
47 changes: 34 additions & 13 deletions common/src/View/Autosaver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,26 +38,47 @@ namespace TrenchBroom {
class Autosaver {
private:
View::MapDocumentWPtr m_document;
Logger* m_logger;

time_t m_saveInterval;
time_t m_idleInterval;

/**
* The time after which a new autosave is attempted, in seconds.
*/
std::time_t m_saveInterval;
/**
* The idle time. The editor must have been idle for this interval before a new autosave is attempted.
* In seconds.
*/
std::time_t m_idleInterval;

/**
* The maximum number of backups to create. When this number is exceeded, old backups are deleted until
* the number of backups is equal to the number of backups again.
*/
size_t m_maxBackups;

time_t m_lastSaveTime;
time_t m_lastModificationTime;

/**
* The time at which the last autosave has succeeded. POSIX timestamp.
*/
std::time_t m_lastSaveTime;

/**
* The time at which the map was modified last. POSIX timestamp.
*/
std::time_t m_lastModificationTime;

/**
* The modification count that was last recorded.
*/
size_t m_lastModificationCount;
public:
Autosaver(View::MapDocumentWPtr document, time_t saveInterval = 10 * 60, time_t idleInterval = 3, size_t maxBackups = 50);
explicit Autosaver(View::MapDocumentWPtr document, std::time_t saveInterval = 10 * 60, std::time_t idleInterval = 3, size_t maxBackups = 50);
~Autosaver();

void triggerAutosave(Logger* logger);
void triggerAutosave(Logger& logger);
private:
void autosave(View::MapDocumentSPtr document);
IO::WritableDiskFileSystem createBackupFileSystem(const IO::Path& mapPath) const;
void autosave(Logger& logger, View::MapDocumentSPtr document);
IO::WritableDiskFileSystem createBackupFileSystem(Logger& logger, const IO::Path& mapPath) const;
IO::Path::List collectBackups(const IO::WritableDiskFileSystem& fs, const IO::Path& mapBasename) const;
bool isBackup(const IO::Path& backupPath, const IO::Path& mapBasename) const;
void thinBackups(IO::WritableDiskFileSystem& fs, IO::Path::List& backups) const;
void thinBackups(Logger& logger, IO::WritableDiskFileSystem& fs, IO::Path::List& backups) const;
void cleanBackups(IO::WritableDiskFileSystem& fs, IO::Path::List& backups, const IO::Path& mapBasename) const;
IO::Path makeBackupName(const IO::Path& mapBasename, const size_t index) const;
private:
Expand Down
2 changes: 1 addition & 1 deletion common/src/View/MapFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2074,7 +2074,7 @@ namespace TrenchBroom {
void MapFrame::OnAutosaveTimer(wxTimerEvent& event) {
if (IsBeingDeleted()) return;

m_autosaver->triggerAutosave(logger());
m_autosaver->triggerAutosave(*logger());
}

int MapFrame::indexForGridSize(const int gridSize) {
Expand Down
15 changes: 15 additions & 0 deletions test/src/IO/DiskFileSystemTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,21 @@ namespace TrenchBroom {
return deleteDirectory(m_dir);
}
};

TEST(FileSystemTest, makeAbsolute) {
TestEnvironment env;

auto fs = std::make_unique<DiskFileSystem>(env.dir() + Path("anotherDir"));
fs = std::make_unique<DiskFileSystem>(std::move(fs), env.dir() + Path("dir1"));

// Existing files should be resolved against the first file system in the chain that contains them:
const auto absPathExisting = fs->makeAbsolute(Path("test3.map"));
ASSERT_EQ(env.dir() + Path("anotherDir/test3.map"), absPathExisting);

// Non existing files should be resolved against the first filesystem in the fs chain:
const auto absPathNotExisting = fs->makeAbsolute(Path("asdf.map"));
ASSERT_EQ(env.dir() + Path("dir1/asdf.map"), absPathNotExisting);
}

TEST(DiskTest, fixPath) {
TestEnvironment env;
Expand Down

0 comments on commit 75b0029

Please sign in to comment.