Commit 97087ccd authored by Dmitry Kazakov's avatar Dmitry Kazakov

Implemented saving of the autosave file in a background thread

Ref T5868
parent 540cd599
......@@ -297,7 +297,7 @@ Document* Krita::openDocument(const QString &filename)
{
KisDocument *document = KisPart::instance()->createDocument();
KisPart::instance()->addDocument(document);
document->openUrl(QUrl::fromLocalFile(filename), KisDocument::OPEN_URL_FLAG_DO_NOT_ADD_TO_RECENT_FILES);
document->openUrl(QUrl::fromLocalFile(filename), KisDocument::DontAddToRecent);
return new Document(document);
}
......
......@@ -619,39 +619,39 @@ void KisApplication::checkAutosaveFiles()
#endif
// all autosave files for our application
m_autosaveFiles = dir.entryList(filters, QDir::Files | QDir::Hidden);
QStringList autosaveFiles = dir.entryList(filters, QDir::Files | QDir::Hidden);
// Allow the user to make their selection
if (m_autosaveFiles.size() > 0) {
if (autosaveFiles.size() > 0) {
if (d->splashScreen) {
// hide the splashscreen to see the dialog
d->splashScreen->hide();
}
m_autosaveDialog = new KisAutoSaveRecoveryDialog(m_autosaveFiles, activeWindow());
m_autosaveDialog = new KisAutoSaveRecoveryDialog(autosaveFiles, activeWindow());
QDialog::DialogCode result = (QDialog::DialogCode) m_autosaveDialog->exec();
if (result == QDialog::Accepted) {
QStringList filesToRecover = m_autosaveDialog->recoverableFiles();
Q_FOREACH (const QString &autosaveFile, m_autosaveFiles) {
Q_FOREACH (const QString &autosaveFile, autosaveFiles) {
if (!filesToRecover.contains(autosaveFile)) {
QFile::remove(dir.absolutePath() + "/" + autosaveFile);
}
}
m_autosaveFiles = filesToRecover;
autosaveFiles = filesToRecover;
} else {
m_autosaveFiles.clear();
autosaveFiles.clear();
}
if (m_autosaveFiles.size() > 0) {
if (autosaveFiles.size() > 0) {
QList<QUrl> autosaveUrls;
Q_FOREACH (const QString &autoSaveFile, m_autosaveFiles) {
Q_FOREACH (const QString &autoSaveFile, autosaveFiles) {
const QUrl url = QUrl::fromLocalFile(dir.absolutePath() + QLatin1Char('/') + autoSaveFile);
autosaveUrls << url;
}
if (m_mainWindow) {
Q_FOREACH (const QUrl &url, autosaveUrls) {
KisMainWindow::OpenFlags flags = m_batchRun ? KisMainWindow::BatchMode : KisMainWindow::None;
m_mainWindow->openDocument(url, flags);
m_mainWindow->openDocument(url, flags | KisMainWindow::RecoveryFile);
}
}
}
......
......@@ -116,7 +116,6 @@ private:
class ResetStarting;
friend class ResetStarting;
KisAutoSaveRecoveryDialog *m_autosaveDialog;
QStringList m_autosaveFiles;
QPointer<KisMainWindow> m_mainWindow; // The first mainwindow we create on startup
bool m_batchRun;
};
......
......@@ -72,6 +72,8 @@
#include <QtGlobal>
#include <QTimer>
#include <QWidget>
#include <QFuture>
#include <QFutureWatcher>
// Krita Image
#include <kis_config.h>
......@@ -351,6 +353,11 @@ public:
StdLockableWrapper<QMutex> savingLock;
QScopedPointer<KisDocument> backgroundSaveDocument;
QFuture<KisImportExportFilter::ConversionStatus> childSavingFuture;
bool isRecovered = false;
void setImageAndInitIdleWatcher(KisImageSP _image) {
image = _image;
......@@ -365,6 +372,7 @@ public:
}
class SafeSavingLocker;
class StrippedSafeSavingLocker;
};
class KisDocument::Private::SafeSavingLocker {
......@@ -431,6 +439,53 @@ private:
KisImageBarrierLockAdapter m_imageLock;
};
class KisDocument::Private::StrippedSafeSavingLocker {
public:
StrippedSafeSavingLocker(QMutex *savingMutex, KisImageSP image)
: m_locked(false)
, m_image(image)
, m_savingLock(savingMutex)
, m_imageLock(image, true)
{
/**
* Initial try to lock both objects. Locking the image guards
* us from any image composition threads running in the
* background, while savingMutex guards us from entering the
* saving code twice by autosave and main threads.
*
* Since we are trying to lock multiple objects, so we should
* do it in a safe manner.
*/
m_locked = std::try_lock(m_imageLock, m_savingLock) < 0;
if (!m_locked) {
m_image->requestStrokeEnd();
QApplication::processEvents();
// one more try...
m_locked = std::try_lock(m_imageLock, m_savingLock) < 0;
}
}
~StrippedSafeSavingLocker() {
if (m_locked) {
m_imageLock.unlock();
m_savingLock.unlock();
}
}
bool successfullyLocked() const {
return m_locked;
}
private:
bool m_locked;
KisImageSP m_image;
StdLockableWrapper<QMutex> m_savingLock;
KisImageBarrierLockAdapter m_imageLock;
};
KisDocument::KisDocument()
: d(new Private(this))
{
......@@ -795,33 +850,102 @@ void KisDocument::setFileBatchMode(const bool batchMode)
void KisDocument::slotAutoSave()
{
//qDebug() << "slotAutoSave. Modified:" << d->modified << "modifiedAfterAutosave" << d->modified << "url" << url() << localFilePath();
if (!d->modified || !d->modifiedAfterAutosave) return;
if (!d->isAutosaving && d->modified && d->modifiedAfterAutosave) {
QScopedPointer<KisDocument> clonedDocument;
connect(this, SIGNAL(sigProgress(int)), KisPart::instance()->currentMainwindow(), SLOT(slotProgress(int)));
emit statusBarMessage(i18n("Autosaving..."));
d->isAutosaving = true;
QString autoSaveFileName = generateAutoSaveFileName(localFilePath());
{
Private::StrippedSafeSavingLocker locker(&d->savingMutex, d->image);
if (!locker.successfullyLocked()) {
const int emergencyAutoSaveInterval = 10; // sec
setAutoSaveDelay(emergencyAutoSaveInterval);
return;
}
clonedDocument.reset(new KisDocument(*this));
}
KIS_SAFE_ASSERT_RECOVER_RETURN(clonedDocument);
QByteArray mimetype = d->outputMimeType;
d->outputMimeType = nativeFormatMimeType();
bool ret = exportDocument(QUrl::fromLocalFile(autoSaveFileName));
d->outputMimeType = mimetype;
// we block saving until the current saving is finished!
if (!d->savingMutex.tryLock()) return;
if (ret) {
d->modifiedAfterAutosave = false;
d->autoSaveTimer.stop(); // until the next change
}
d->isAutosaving = false;
KIS_ASSERT_RECOVER_RETURN(!d->backgroundSaveDocument);
d->backgroundSaveDocument.reset(clonedDocument.take());
emit statusBarMessage(i18n("Autosaving..."));
connect(d->backgroundSaveDocument.data(), SIGNAL(sigBackgroundSavingFinished(KisImportExportFilter::ConversionStatus)), SLOT(slotChildCompletedSavingInBackground(KisImportExportFilter::ConversionStatus)));
const QString autoSaveFileName = generateAutoSaveFileName(localFilePath());
bool started =
d->backgroundSaveDocument->startExportInBackground(QUrl::fromLocalFile(autoSaveFileName),
nativeFormatMimeType());
if (!started) {
const int emergencyAutoSaveInterval = 10; // sec
setAutoSaveDelay(emergencyAutoSaveInterval);
}
}
void KisDocument::slotChildCompletedSavingInBackground(KisImportExportFilter::ConversionStatus status)
{
KIS_SAFE_ASSERT_RECOVER(!d->savingMutex.tryLock()) {
d->savingMutex.unlock();
return;
}
KIS_SAFE_ASSERT_RECOVER_RETURN(d->backgroundSaveDocument);
d->backgroundSaveDocument.take()->deleteLater();
d->savingMutex.unlock();
if (status != KisImportExportFilter::OK) {
const int emergencyAutoSaveInterval = 10; // sec
setAutoSaveDelay(emergencyAutoSaveInterval);
emit statusBarMessage(i18n("Error during autosave! Partition full?"));
} else {
d->modifiedAfterAutosave = false;
KisConfig cfg;
d->autoSaveDelay = cfg.autoSaveInterval();
d->autoSaveTimer.stop(); // until the next change
emit clearStatusBarMessage();
disconnect(this, SIGNAL(sigProgress(int)), KisPart::instance()->currentMainwindow(), SLOT(slotProgress(int)));
}
}
if (!ret && !d->disregardAutosaveFailure) {
emit statusBarMessage(i18n("Error during autosave! Partition full?"));
}
bool KisDocument::startExportInBackground(const QUrl &url,
const QByteArray &mimeType)
{
const QString filePath = url.toLocalFile();
d->savingImage = d->image;
d->childSavingFuture =
d->importExportManager->exportDocumentAsyc(filePath,
filePath,
mimeType,
false, 0);
if (d->childSavingFuture.isCanceled()) return false;
typedef QFutureWatcher<KisImportExportFilter::ConversionStatus> StatusWatcher;
StatusWatcher *watcher = new StatusWatcher();
watcher->setFuture(d->childSavingFuture);
connect(watcher, SIGNAL(finished()), SLOT(finishExportInBackground()));
connect(watcher, SIGNAL(finished()), watcher, SLOT(deleteLater()));
return true;
}
void KisDocument::finishExportInBackground()
{
KIS_SAFE_ASSERT_RECOVER(d->childSavingFuture.isFinished()) {
emit sigBackgroundSavingFinished(KisImportExportFilter::InternalError);
return;
}
KisImportExportFilter::ConversionStatus status =
d->childSavingFuture.result();
d->savingImage.clear();
d->childSavingFuture = QFuture<KisImportExportFilter::ConversionStatus>();
emit sigBackgroundSavingFinished(status);
}
void KisDocument::setReadWrite(bool readwrite)
......@@ -926,7 +1050,7 @@ bool KisDocument::importDocument(const QUrl &_url)
}
bool KisDocument::openUrl(const QUrl &_url, KisDocument::OpenUrlFlags flags)
bool KisDocument::openUrl(const QUrl &_url, OpenFlags flags)
{
if (!_url.isLocalFile()) {
return false;
......@@ -970,13 +1094,13 @@ bool KisDocument::openUrl(const QUrl &_url, KisDocument::OpenUrlFlags flags)
bool ret = openUrlInternal(url);
if (autosaveOpened) {
resetURL(); // Force save to act like 'Save As'
if (autosaveOpened || flags & RecoveryFile) {
setReadWrite(true); // enable save button
setModified(true);
setRecovered(true);
}
else {
if( !(flags & OPEN_URL_FLAG_DO_NOT_ADD_TO_RECENT_FILES) ) {
if (!(flags & DontAddToRecent)) {
KisPart::instance()->addRecentURLToAllMainWindows(_url);
}
......@@ -985,7 +1109,10 @@ bool KisDocument::openUrl(const QUrl &_url, KisDocument::OpenUrlFlags flags)
QFileInfo fi(url.toLocalFile());
setReadWrite(fi.isWritable());
}
setRecovered(false);
}
return ret;
}
......@@ -1176,6 +1303,16 @@ void KisDocument::setModified(bool mod)
emit modified(mod);
}
void KisDocument::setRecovered(bool value)
{
d->isRecovered = value;
}
bool KisDocument::isRecovered() const
{
return d->isRecovered;
}
void KisDocument::updateEditingTime(bool forceStoreElapsed)
{
QDateTime now = QDateTime::currentDateTime();
......
......@@ -31,6 +31,7 @@
#include <KoDocumentBase.h>
#include <kundo2stack.h>
#include <KisImportExportFilter.h>
#include <kis_properties_configuration.h>
#include <kis_types.h>
#include <kis_painting_assistant.h>
......@@ -90,11 +91,12 @@ protected:
explicit KisDocument(const KisDocument &rhs);
public:
enum OpenUrlFlags {
OPEN_URL_FLAG_NONE = 1 << 0,
OPEN_URL_FLAG_DO_NOT_ADD_TO_RECENT_FILES = 1 << 1,
enum OpenFlag {
None = 0,
DontAddToRecent = 0x1,
RecoveryFile = 0x2
};
Q_DECLARE_FLAGS(OpenFlags, OpenFlag)
/**
* Destructor.
......@@ -116,7 +118,7 @@ public:
* @param flags Control specific behavior
* @return success status
*/
bool openUrl(const QUrl &url, OpenUrlFlags flags = OPEN_URL_FLAG_NONE);
bool openUrl(const QUrl &url, OpenFlags flags = None);
/**
* Opens the document given by @p url, without storing the URL
......@@ -359,6 +361,9 @@ public:
*/
void setModified(bool _mod);
void setRecovered(bool value);
bool isRecovered() const;
void updateEditingTime(bool forceStoreElapsed);
/**
......@@ -446,11 +451,19 @@ Q_SIGNALS:
void sigGuidesConfigChanged(const KisGuidesConfig &config);
void sigBackgroundSavingFinished(KisImportExportFilter::ConversionStatus status);
private Q_SLOTS:
void finishExportInBackground();
void slotChildCompletedSavingInBackground(KisImportExportFilter::ConversionStatus status);
private:
friend class KisPart;
friend class SafeSavingLocker;
bool startExportInBackground(const QUrl &url,
const QByteArray &mimeType);
/**
* Generate a name for the document.
*/
......@@ -607,6 +620,7 @@ private:
Private *const d;
};
Q_DECLARE_OPERATORS_FOR_FLAGS(KisDocument::OpenFlags)
Q_DECLARE_METATYPE(KisDocument*)
#endif
......@@ -33,6 +33,8 @@
#include <QCheckBox>
#include <QSaveFile>
#include <QGroupBox>
#include <QFuture>
#include <QtConcurrent>
#include <klocalizedstring.h>
#include <ksqueezedtextlabel.h>
......@@ -72,6 +74,42 @@ public:
QPointer<KoProgressUpdater> progressUpdater {0};
};
struct KisImportExportManager::ConversionResult {
ConversionResult()
{
}
ConversionResult(const QFuture<KisImportExportFilter::ConversionStatus> &futureStatus)
: m_isAsync(true),
m_futureStatus(futureStatus)
{
}
ConversionResult(KisImportExportFilter::ConversionStatus status)
: m_isAsync(false),
m_status(status)
{
}
bool isAsync() const {
return m_isAsync;
}
QFuture<KisImportExportFilter::ConversionStatus> futureStatus() const {
return m_futureStatus;
}
KisImportExportFilter::ConversionStatus status() const {
return m_status;
}
private:
bool m_isAsync = false;
QFuture<KisImportExportFilter::ConversionStatus> m_futureStatus;
KisImportExportFilter::ConversionStatus m_status = KisImportExportFilter::UsageError;
};
KisImportExportManager::KisImportExportManager(KisDocument* document)
: m_document(document)
, d(new Private)
......@@ -85,12 +123,26 @@ KisImportExportManager::~KisImportExportManager()
KisImportExportFilter::ConversionStatus KisImportExportManager::importDocument(const QString& location, const QString& mimeType)
{
return convert(Import, location, location, mimeType, false, 0);
ConversionResult result = convert(Import, location, location, mimeType, false, 0, false);
KIS_SAFE_ASSERT_RECOVER_RETURN_VALUE(!result.isAsync(), KisImportExportFilter::UsageError);
return result.status();
}
KisImportExportFilter::ConversionStatus KisImportExportManager::exportDocument(const QString& location, const QString& realLocation, QByteArray& mimeType, bool showWarnings, KisPropertiesConfigurationSP exportConfiguration)
KisImportExportFilter::ConversionStatus KisImportExportManager::exportDocument(const QString& location, const QString& realLocation, const QByteArray& mimeType, bool showWarnings, KisPropertiesConfigurationSP exportConfiguration)
{
return convert(Export, location, realLocation, mimeType, showWarnings, exportConfiguration);
ConversionResult result = convert(Export, location, realLocation, mimeType, showWarnings, exportConfiguration, false);
KIS_SAFE_ASSERT_RECOVER_RETURN_VALUE(!result.isAsync(), KisImportExportFilter::UsageError);
return result.status();
}
QFuture<KisImportExportFilter::ConversionStatus> KisImportExportManager::exportDocumentAsyc(const QString &location, const QString &realLocation, const QByteArray &mimeType, bool showWarnings, KisPropertiesConfigurationSP exportConfiguration)
{
ConversionResult result = convert(Export, location, realLocation, mimeType, showWarnings, exportConfiguration, true);
KIS_SAFE_ASSERT_RECOVER_RETURN_VALUE(result.isAsync(), QFuture<KisImportExportFilter::ConversionStatus>());
return result.futureStatus();
}
// The static method to figure out to which parts of the
......@@ -216,7 +268,7 @@ QString KisImportExportManager::askForAudioFileName(const QString &defaultDir, Q
return dialog.filename();
}
KisImportExportFilter::ConversionStatus KisImportExportManager::convert(KisImportExportManager::Direction direction, const QString &location, const QString& realLocation, const QString &mimeType, bool showWarnings, KisPropertiesConfigurationSP exportConfiguration)
KisImportExportManager::ConversionResult KisImportExportManager::convert(KisImportExportManager::Direction direction, const QString &location, const QString& realLocation, const QString &mimeType, bool showWarnings, KisPropertiesConfigurationSP exportConfiguration, bool isAsync)
{
// export configuration is supported for export only
KIS_SAFE_ASSERT_RECOVER_NOOP(direction == Export || !bool(exportConfiguration));
......@@ -257,13 +309,16 @@ KisImportExportFilter::ConversionStatus KisImportExportManager::convert(KisImpor
KisImportExportFilter::ConversionStatus status = KisImportExportFilter::OK;
ConversionResult result = KisImportExportFilter::OK;
if (direction == Import) {
// async importing is not yet supported!
KIS_SAFE_ASSERT_RECOVER_NOOP(!isAsync);
if (!batchMode()) {
KisAsyncActionFeedback f(i18n("Opening document..."), 0);
status = f.runAction(std::bind(&KisImportExportManager::doImport, this, location, filter));
result = f.runAction(std::bind(&KisImportExportManager::doImport, this, location, filter));
} else {
status = doImport(location, filter);
result = doImport(location, filter);
}
} else /* if (direction == Export) */ {
if (!exportConfiguration) {
......@@ -283,11 +338,13 @@ KisImportExportFilter::ConversionStatus KisImportExportManager::convert(KisImpor
return KisImportExportFilter::UserCancelled;
}
if (!batchMode()) {
if (isAsync) {
result = QtConcurrent::run(std::bind(&KisImportExportManager::doExport, this, location, filter, exportConfiguration, alsoAsKra));
} else if (!batchMode()) {
KisAsyncActionFeedback f(i18n("Saving document..."), 0);
status = f.runAction(std::bind(&KisImportExportManager::doExport, this, location, filter, exportConfiguration, alsoAsKra));
result = f.runAction(std::bind(&KisImportExportManager::doExport, this, location, filter, exportConfiguration, alsoAsKra));
} else {
status = doExport(location, filter, exportConfiguration, alsoAsKra);
result = doExport(location, filter, exportConfiguration, alsoAsKra);
}
if (exportConfiguration) {
......@@ -295,7 +352,7 @@ KisImportExportFilter::ConversionStatus KisImportExportManager::convert(KisImpor
}
}
return status;
return result;
}
void KisImportExportManager::fillStaticExportConfigurationProperties(KisPropertiesConfigurationSP exportConfiguration)
......
......@@ -32,6 +32,9 @@
class KisDocument;
class KoProgressUpdater;
template <class T>
class QFuture;
/**
* @brief The class managing all the filters.
*
......@@ -83,7 +86,9 @@ public:
* and when the method returns @p mimeType contains this mimetype.
* Oh, well, export is a C++ keyword ;)
*/
KisImportExportFilter::ConversionStatus exportDocument(const QString &location, const QString& realLocation, QByteArray &mimeType, bool showWarnings = true, KisPropertiesConfigurationSP exportConfiguration = 0);
KisImportExportFilter::ConversionStatus exportDocument(const QString &location, const QString& realLocation, const QByteArray &mimeType, bool showWarnings = true, KisPropertiesConfigurationSP exportConfiguration = 0);
QFuture<KisImportExportFilter::ConversionStatus> exportDocumentAsyc(const QString &location, const QString& realLocation, const QByteArray &mimeType, bool showWarnings = true, KisPropertiesConfigurationSP exportConfiguration = 0);
///@name Static API
//@{
......@@ -127,7 +132,8 @@ private Q_SLOTS:
private:
KisImportExportFilter::ConversionStatus convert(Direction direction, const QString &location, const QString& realLocation, const QString &mimeType, bool showWarnings, KisPropertiesConfigurationSP exportConfiguration);
struct ConversionResult;
ConversionResult convert(Direction direction, const QString &location, const QString& realLocation, const QString &mimeType, bool showWarnings, KisPropertiesConfigurationSP exportConfiguration, bool isAsync);
void fillStaticExportConfigurationProperties(KisPropertiesConfigurationSP exportConfiguration);
......
......@@ -771,7 +771,15 @@ bool KisMainWindow::openDocumentInternal(const QUrl &url, OpenFlags flags)
connect(newdoc, SIGNAL(sigProgress(int)), this, SLOT(slotProgress(int)));
connect(newdoc, SIGNAL(completed()), this, SLOT(slotLoadCompleted()));
connect(newdoc, SIGNAL(canceled(const QString &)), this, SLOT(slotLoadCanceled(const QString &)));
bool openRet = !(flags & Import) ? newdoc->openUrl(url) : newdoc->importDocument(url);
KisDocument::OpenFlags openFlags = KisDocument::None;
if (flags & RecoveryFile) {
openFlags |= KisDocument::RecoveryFile;
}
bool openRet = !(flags & Import) ? newdoc->openUrl(url, openFlags) : newdoc->importDocument(url);
if (!openRet) {
delete newdoc;
return false;
......@@ -898,6 +906,10 @@ bool KisMainWindow::saveDocument(KisDocument *document, bool saveas, bool isExpo
saveas = true;
}
if (document->isRecovered()) {
saveas = true;
}
bool reset_url;
if (document->url().isEmpty()) {
......@@ -1090,6 +1102,9 @@ bool KisMainWindow::saveDocument(KisDocument *document, bool saveas, bool isExpo
}
}
if (ret && !isExporting) {
document->setRecovered(false);
}
if (!ret && reset_url)
document->resetURL(); //clean the suggested filename as the save dialog was rejected
......
......@@ -62,9 +62,10 @@ class KRITAUI_EXPORT KisMainWindow : public KXmlGuiWindow, public KoCanvasSuperv
public:
enum OpenFlag {
None,
Import,
BatchMode
None = 0,
Import = 0x1,
BatchMode = 0x2,
RecoveryFile = 0x4
};
Q_DECLARE_FLAGS(OpenFlags, OpenFlag)
......
......@@ -85,7 +85,7 @@ KisImportExportFilter::ConversionStatus KisAnimationImporter::import(QStringList
KisRasterKeyframeChannel *contentChannel = 0;
Q_FOREACH(QString file, files) {
bool successfullyLoaded = importDoc->openUrl(QUrl::fromLocalFile(file), KisDocument::OPEN_URL_FLAG_DO_NOT_ADD_TO_RECENT_FILES);
bool successfullyLoaded = importDoc->openUrl(QUrl::fromLocalFile(file), KisDocument::DontAddToRecent);
if (!successfullyLoaded) {
status = KisImportExportFilter::InternalError;
break;
......
......@@ -151,7 +151,7 @@ void KisSafeDocumentLoader::delayedLoadStart()
m_d->doc.reset(KisPart::instance()->createDocument());
successfullyLoaded = m_d->doc->openUrl(QUrl::fromLocalFile(m_d->temporaryPath),
KisDocument::OPEN_URL_FLAG_DO_NOT_ADD_TO_RECENT_FILES);
KisDocument::DontAddToRecent);
} else {
dbgKrita << "File was modified externally. Restarting.";
dbgKrita << ppVar(m_d->fileChangedFlag);
......
......@@ -441,7 +441,7 @@ KisImageBuilder_Result CSVLoader::setLayer(CSVLayerRecord* layer, KisDocument *i
filename.append(layer->last);
result = importDoc->openUrl(QUrl::fromLocalFile(filename),
KisDocument::OPEN_URL_FLAG_DO_NOT_ADD_TO_RECENT_FILES);
KisDocument::DontAddToRecent);
if (result)
layer->channel->importFrame(layer->frame, importDoc->image()->projection(), 0);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment