[krita] /: Added a concept of "safe" assert
Dmitry Kazakov
dimula73 at gmail.com
Fri Apr 22 15:36:43 UTC 2016
Git commit 146283952d5a007b6cc74a7bd24d0fc4e18f792d by Dmitry Kazakov.
Committed on 22/04/2016 at 15:08.
Pushed by dkazakov into branch 'master'.
Added a concept of "safe" assert
If you add some sanity check to the code and you are 100% sure
that failing this assert and recovering after it will not break
Krita and the user will be able to continue his work, you can use
"safe asserts". By default they don't show anything to the user,
just dump a message to the terminal and take the recovery branch
almost silently.
If you want to make these assert produce real warning messages you
should set a cmake option for it:
cmake -DHIDE_SAFE_ASSERTS=OFF .
This is highly recommended for developers, and not recommended for
painters who do real work of those builds of Krita.
Now rules when to use "safe" asserts. You can use them if the following
conditions are met:
1) The condition in the assert shows that a real *bug* has happened. It
is not just a warning. It is a bug that should be fixed.
2) The recovery branch will *workaround* the bug and the user will be
able to continue his work *as if nothing has happened*. Again, please
mark the assert "safe" if and only if you are 100% sure Krita will
not crash in a minute after you faced that bug. The user is not notified
about this bug, so he is not going to take any emergency steps like
saving his work and restarting Krita!
3) If you think that Krita should be better restarted after this bug,
please use a usual KIS_ASSRT_RECOVER.
CC:kimageshop at kde.org
M +3 -1 CMakeLists.txt
A +7 -0 config-hide-safe-asserts.h.cmake
M +27 -7 libs/global/kis_assert.cpp
M +35 -0 libs/global/kis_assert.h
M +1 -1 plugins/impex/bmp/kis_bmp_export.cpp
M +1 -1 plugins/impex/brush/kis_brush_export.cpp
M +2 -2 plugins/impex/exr/exr_export.cc
M +1 -1 plugins/impex/heightmap/kis_heightmap_export.cpp
M +1 -1 plugins/impex/jp2/jp2_export.cc
M +1 -1 plugins/impex/jpeg/kis_jpeg_export.cc
M +1 -1 plugins/impex/png/kis_png_export.cc
M +1 -1 plugins/impex/ppm/kis_ppm_export.cpp
M +2 -2 plugins/impex/psd/psd_loader.cpp
M +1 -1 plugins/impex/tga/kis_tga_export.cpp
M +1 -1 plugins/impex/tiff/kis_tiff_export.cc
http://commits.kde.org/krita/146283952d5a007b6cc74a7bd24d0fc4e18f792d
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6f6f4ae..3ae6e07 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -148,6 +148,9 @@ if (WIN32)
option(USE_BREAKPAD "Build the crash handler for Krita (only on windows)" OFF)
endif ()
+option(HIDE_SAFE_ASSERTS "Don't show message box for \"safe\" asserts, just ignore them automatically and dump a message to the terminal." ON)
+configure_file(config-hide-safe-asserts.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-hide-safe-asserts.h)
+
#######################
########################
## Productset setting ##
@@ -698,4 +701,3 @@ configure_file(config-ocio.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-ocio.h )
check_function_exists(powf HAVE_POWF)
configure_file(config-powf.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-powf.h)
-
diff --git a/config-hide-safe-asserts.h.cmake b/config-hide-safe-asserts.h.cmake
new file mode 100644
index 0000000..e9fe991
--- /dev/null
+++ b/config-hide-safe-asserts.h.cmake
@@ -0,0 +1,7 @@
+/* config-hide-safe-asserts.h. Generated by cmake from config-hide-safe-asserts.h.cmake */
+
+/* Define if you have ocio, the OpenColorIO Library */
+#cmakedefine HIDE_SAFE_ASSERTS 1
+
+
+
diff --git a/libs/global/kis_assert.cpp b/libs/global/kis_assert.cpp
index df39fc7..3b3c72f 100644
--- a/libs/global/kis_assert.cpp
+++ b/libs/global/kis_assert.cpp
@@ -26,6 +26,7 @@
#include <klocalizedstring.h>
#include <kis_assert_exception.h>
#include <string>
+#include "config-hide-safe-asserts.h"
/**
* TODO: Add automatic saving of the documents
@@ -40,13 +41,14 @@
* lead to an infinite loop.
*/
-void kis_assert_common(const char *assertion, const char *file, int line, bool throwException)
+void kis_assert_common(const char *assertion, const char *file, int line, bool throwException, bool isIgnorable)
{
QString shortMessage =
- QString("ASSERT (krita): \"%1\" in file %2, line %3")
+ QString("%4ASSERT (krita): \"%1\" in file %2, line %3")
.arg(assertion)
.arg(file)
- .arg(line);
+ .arg(line)
+ .arg(isIgnorable ? "SAFE " : "");
QString longMessage =
QString(
@@ -65,7 +67,16 @@ void kis_assert_common(const char *assertion, const char *file, int line, bool t
disableAssertMsg = true;
}
- QMessageBox::StandardButton button = QMessageBox::Abort;
+#ifdef HIDE_SAFE_ASSERTS
+ const bool shouldIgnoreAsserts = HIDE_SAFE_ASSERTS;
+#else
+ const bool shouldIgnoreAsserts = false;
+#endif
+
+ disableAssertMsg |= shouldIgnoreAsserts;
+
+ QMessageBox::StandardButton button =
+ isIgnorable ? QMessageBox::Ignore : QMessageBox::Abort;
if (!disableAssertMsg) {
button =
@@ -77,6 +88,10 @@ void kis_assert_common(const char *assertion, const char *file, int line, bool t
if (button == QMessageBox::Abort) {
qFatal("%s", shortMessage.toLatin1().data());
+ } else if (isIgnorable) {
+ // Assert is a bug! Please don't change this line to warnKrita,
+ // the user must see it!
+ qWarning("%s", shortMessage.toLatin1().data());
} else if (throwException) {
throw KisAssertException(shortMessage.toLatin1().data());
}
@@ -85,12 +100,17 @@ void kis_assert_common(const char *assertion, const char *file, int line, bool t
void kis_assert_recoverable(const char *assertion, const char *file, int line)
{
- kis_assert_common(assertion, file, line, false);
+ kis_assert_common(assertion, file, line, false, false);
+}
+
+void kis_safe_assert_recoverable(const char *assertion, const char *file, int line)
+{
+ kis_assert_common(assertion, file, line, false, true);
}
void kis_assert_exception(const char *assertion, const char *file, int line)
{
- kis_assert_common(assertion, file, line, true);
+ kis_assert_common(assertion, file, line, true, false);
}
void kis_assert_x_exception(const char *assertion,
@@ -104,5 +124,5 @@ void kis_assert_x_exception(const char *assertion,
.arg(what)
.arg(assertion);
- kis_assert_common(res.toLatin1().data(), file, line, true);
+ kis_assert_common(res.toLatin1().data(), file, line, true, false);
}
diff --git a/libs/global/kis_assert.h b/libs/global/kis_assert.h
index f7e9ba5..d0e4900 100644
--- a/libs/global/kis_assert.h
+++ b/libs/global/kis_assert.h
@@ -26,6 +26,8 @@ KRITAGLOBAL_EXPORT void kis_assert_exception(const char *assertion, const char *
KRITAGLOBAL_EXPORT void kis_assert_recoverable(const char *assertion, const char *file, int line);
KRITAGLOBAL_EXPORT void kis_assert_x_exception(const char *assertion, const char *where, const char *what, const char *file, int line);
+KRITAGLOBAL_EXPORT void kis_safe_assert_recoverable(const char *assertion, const char *file, int line);
+
/**
* KIS_ASSERT family of macros allows the user to choose whether to
@@ -105,4 +107,37 @@ KRITAGLOBAL_EXPORT void kis_assert_x_exception(const char *assertion, const char
*/
#define KIS_ASSERT_RECOVER_NOOP(cond) KIS_ASSERT_RECOVER(cond) { qt_noop(); }
+
+/**
+ * This set of macros work in exactly the same way as their non-safe
+ * counterparts, but they are automatically converted into console
+ * warnings in release builds. That is the user will not see any
+ * message box, just a warning message wil be printed in a terminal
+ * and a recovery branch will be taken automatically.
+ *
+ * Rules when to use "safe" asserts. Use them if the following
+ * conditions are met:
+ *
+ * 1) The condition in the assert shows that a real *bug* has
+ * happened. It is not just a warning. It is a bug that should be
+ * fixed.
+ *
+ * 2) The recovery branch will *workaround* the bug and the user will
+ * be able to continue his work *as if nothing has
+ * happened*. Again, please mark the assert "safe" if and only if
+ * you are 100% sure Krita will not crash in a minute after you
+ * faced that bug. The user is not notified about this bug, so he
+ * is not going to take any emergency steps like saving his work
+ * and restarting Krita!
+ *
+ * 3) If you think that Krita should be better restarted after this
+ * bug, please use a usual KIS_ASSRT_RECOVER.
+ */
+
+#define KIS_SAFE_ASSERT_RECOVER(cond) if (!(cond) && (kis_safe_assert_recoverable(#cond,__FILE__,__LINE__), true))
+#define KIS_SAFE_ASSERT_RECOVER_BREAK(cond) KIS_SAFE_ASSERT_RECOVER(cond) { break; }
+#define KIS_SAFE_ASSERT_RECOVER_RETURN(cond) KIS_SAFE_ASSERT_RECOVER(cond) { return; }
+#define KIS_SAFE_ASSERT_RECOVER_RETURN_VALUE(cond, val) KIS_SAFE_ASSERT_RECOVER(cond) { return (val); }
+#define KIS_SAFE_ASSERT_RECOVER_NOOP(cond) KIS_SAFE_ASSERT_RECOVER(cond) { qt_noop(); }
+
#endif /* __KIS_ASSERT_H */
diff --git a/plugins/impex/bmp/kis_bmp_export.cpp b/plugins/impex/bmp/kis_bmp_export.cpp
index 1b88903..6202f39 100644
--- a/plugins/impex/bmp/kis_bmp_export.cpp
+++ b/plugins/impex/bmp/kis_bmp_export.cpp
@@ -60,7 +60,7 @@ KisImportExportFilter::ConversionStatus KisBMPExport::convert(const QByteArray&
QRect rc = input->image()->bounds();
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
QImage image = input->image()->projection()->convertToQImage(0, 0, 0, rc.width(), rc.height(), KoColorConversionTransformation::internalRenderingIntent(), KoColorConversionTransformation::internalConversionFlags());
image.save(filename);
return KisImportExportFilter::OK;
diff --git a/plugins/impex/brush/kis_brush_export.cpp b/plugins/impex/brush/kis_brush_export.cpp
index a5652fd..af895c4 100644
--- a/plugins/impex/brush/kis_brush_export.cpp
+++ b/plugins/impex/brush/kis_brush_export.cpp
@@ -142,7 +142,7 @@ KisImportExportFilter::ConversionStatus KisBrushExport::convert(const QByteArray
}
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
QRect rc = input->image()->bounds();
diff --git a/plugins/impex/exr/exr_export.cc b/plugins/impex/exr/exr_export.cc
index 79468d7..1b196f5 100644
--- a/plugins/impex/exr/exr_export.cc
+++ b/plugins/impex/exr/exr_export.cc
@@ -102,7 +102,7 @@ KisImportExportFilter::ConversionStatus exrExport::convert(const QByteArray& fro
if (widget.flatten->isChecked()) {
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
KisPaintDeviceSP pd = new KisPaintDevice(*image->projection());
KisPaintLayerSP l = new KisPaintLayer(image, "projection", OPACITY_OPAQUE_U8, pd);
@@ -111,7 +111,7 @@ KisImportExportFilter::ConversionStatus exrExport::convert(const QByteArray& fro
}
else {
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
res = kpc.buildFile(filename, image->rootLayer());
}
diff --git a/plugins/impex/heightmap/kis_heightmap_export.cpp b/plugins/impex/heightmap/kis_heightmap_export.cpp
index 12305f2..eeeb4ba 100644
--- a/plugins/impex/heightmap/kis_heightmap_export.cpp
+++ b/plugins/impex/heightmap/kis_heightmap_export.cpp
@@ -139,7 +139,7 @@ KisImportExportFilter::ConversionStatus KisHeightMapExport::convert(const QByteA
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(image->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(image->locked());
KisPaintDeviceSP pd = new KisPaintDevice(*image->projection());
QFile f(filename);
diff --git a/plugins/impex/jp2/jp2_export.cc b/plugins/impex/jp2/jp2_export.cc
index 0698190..a7f92b3 100644
--- a/plugins/impex/jp2/jp2_export.cc
+++ b/plugins/impex/jp2/jp2_export.cc
@@ -106,7 +106,7 @@ KisImportExportFilter::ConversionStatus jp2Export::convert(const QByteArray& fro
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
jp2Converter kpc(input);
KisPaintDeviceSP pd = new KisPaintDevice(*image->projection());
diff --git a/plugins/impex/jpeg/kis_jpeg_export.cc b/plugins/impex/jpeg/kis_jpeg_export.cc
index 4a35cfb..8169266 100644
--- a/plugins/impex/jpeg/kis_jpeg_export.cc
+++ b/plugins/impex/jpeg/kis_jpeg_export.cc
@@ -192,7 +192,7 @@ KisImportExportFilter::ConversionStatus KisJPEGExport::convert(const QByteArray&
if (filename.isEmpty()) return KisImportExportFilter::FileNotFound;
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
KisPaintDeviceSP pd = new KisPaintDevice(*image->projection());
KisJPEGConverter kpc(input, getBatchMode());
diff --git a/plugins/impex/png/kis_png_export.cc b/plugins/impex/png/kis_png_export.cc
index 7dc2cf2..13151a7 100644
--- a/plugins/impex/png/kis_png_export.cc
+++ b/plugins/impex/png/kis_png_export.cc
@@ -94,7 +94,7 @@ KisImportExportFilter::ConversionStatus KisPNGExport::convert(const QByteArray&
KisImageWSP image = input->image();
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(image->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(image->locked());
KisPaintDeviceSP pd;
pd = new KisPaintDevice(*image->projection());
KisPaintLayerSP l = new KisPaintLayer(image, "projection", OPACITY_OPAQUE_U8, pd);
diff --git a/plugins/impex/ppm/kis_ppm_export.cpp b/plugins/impex/ppm/kis_ppm_export.cpp
index b908def..1859cf3 100644
--- a/plugins/impex/ppm/kis_ppm_export.cpp
+++ b/plugins/impex/ppm/kis_ppm_export.cpp
@@ -185,7 +185,7 @@ KisImportExportFilter::ConversionStatus KisPPMExport::convert(const QByteArray&
KisImageWSP image = input->image();
Q_CHECK_PTR(image);
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
KisPaintDeviceSP pd = new KisPaintDevice(*image->projection());
// Test color space
diff --git a/plugins/impex/psd/psd_loader.cpp b/plugins/impex/psd/psd_loader.cpp
index 0e1ef02..12f7761 100644
--- a/plugins/impex/psd/psd_loader.cpp
+++ b/plugins/impex/psd/psd_loader.cpp
@@ -344,10 +344,10 @@ KisImageBuilder_Result PSDLoader::decode(const QString &filename)
new KisPSDLayerStyleCollectionResource("Embedded PSD Styles.asl");
collection->setName(i18nc("Auto-generated layer style collection name for embedded styles (collection)", "<%1> (embedded)", m_image->objectName()));
- KIS_ASSERT_RECOVER_NOOP(!collection->valid());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(!collection->valid());
collection->setLayerStyles(allStylesForServer);
- KIS_ASSERT_RECOVER_NOOP(collection->valid());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(collection->valid());
KoResourceServer<KisPSDLayerStyleCollectionResource> *server = KisResourceServerProvider::instance()->layerStyleCollectionServer();
server->addResource(collection, false);
diff --git a/plugins/impex/tga/kis_tga_export.cpp b/plugins/impex/tga/kis_tga_export.cpp
index 9d0fe00..c895d95 100644
--- a/plugins/impex/tga/kis_tga_export.cpp
+++ b/plugins/impex/tga/kis_tga_export.cpp
@@ -62,7 +62,7 @@ KisImportExportFilter::ConversionStatus KisTGAExport::convert(const QByteArray&
return KisImportExportFilter::NotImplemented;
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
QRect rc = input->image()->bounds();
QImage image = input->image()->projection()->convertToQImage(0, 0, 0, rc.width(), rc.height(), KoColorConversionTransformation::internalRenderingIntent(), KoColorConversionTransformation::internalConversionFlags());
diff --git a/plugins/impex/tiff/kis_tiff_export.cc b/plugins/impex/tiff/kis_tiff_export.cc
index 660333c..f9c2453 100644
--- a/plugins/impex/tiff/kis_tiff_export.cc
+++ b/plugins/impex/tiff/kis_tiff_export.cc
@@ -110,7 +110,7 @@ KisImportExportFilter::ConversionStatus KisTIFFExport::convert(const QByteArray&
}
// the image must be locked at the higher levels
- KIS_ASSERT_RECOVER_NOOP(input->image()->locked());
+ KIS_SAFE_ASSERT_RECOVER_NOOP(input->image()->locked());
KisTIFFConverter ktc(input);
KisImageBuilder_Result res;
More information about the kimageshop
mailing list