From 7ead74950b6f11b39ef346a7c93c59055cbc1153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Sun, 24 Sep 2023 14:35:48 +0200 Subject: [PATCH] Text: rename AbstractFont::Metrics to Properties. It won't contain just font metrics anymore. Also don't require the struct to be zero-initialized if opening fails -- simply allow the plugins to return garbage in that case and save the values only if opening actually succeeded. Strictly speaking this isn't an ABI change as the return value isn't part of the function signature and the struct is still the same, so the plugin interface version isn't bumped for this change. --- src/Magnum/Text/AbstractFont.cpp | 74 ++++++++++++------- src/Magnum/Text/AbstractFont.h | 28 ++++--- src/Magnum/Text/Test/AbstractFontTest.cpp | 22 +++--- src/Magnum/Text/Test/RendererGLTest.cpp | 2 +- src/MagnumPlugins/MagnumFont/MagnumFont.cpp | 4 +- src/MagnumPlugins/MagnumFont/MagnumFont.h | 4 +- .../Test/MagnumFontConverterTest.cpp | 2 +- 7 files changed, 80 insertions(+), 56 deletions(-) diff --git a/src/Magnum/Text/AbstractFont.cpp b/src/Magnum/Text/AbstractFont.cpp index a7ba77b72..be01f267f 100644 --- a/src/Magnum/Text/AbstractFont.cpp +++ b/src/Magnum/Text/AbstractFont.cpp @@ -105,27 +105,33 @@ bool AbstractFont::openData(Containers::ArrayView data, const Float the check doesn't be done on the plugin side) because for some file formats it could be valid (MagnumFont in particular). */ close(); - const Metrics metrics = doOpenData(Containers::arrayCast(data), size); - _size = metrics.size; - _ascent = metrics.ascent; - _descent = metrics.descent; - _lineHeight = metrics.lineHeight; - CORRADE_INTERNAL_ASSERT(isOpened() || (!_size && !_ascent && !_descent && !_lineHeight)); - return isOpened(); + const Properties properties = doOpenData(Containers::arrayCast(data), size); + + /* If opening succeeded, save the returned values. If not, the values were + set to their default values by close() already. */ + if(isOpened()) { + _size = properties.size; + _ascent = properties.ascent; + _descent = properties.descent; + _lineHeight = properties.lineHeight; + return true; + } + + return false; } -auto AbstractFont::doOpenData(Containers::ArrayView, Float) -> Metrics { +auto AbstractFont::doOpenData(Containers::ArrayView, Float) -> Properties { CORRADE_ASSERT_UNREACHABLE("Text::AbstractFont::openData(): feature advertised but not implemented", {}); } bool AbstractFont::openFile(const Containers::StringView filename, const Float size) { close(); - Metrics metrics; + Properties properties; /* If file loading callbacks are not set or the font implementation supports handling them directly, call into the implementation */ if(!_fileCallback || (doFeatures() & FontFeature::FileCallback)) { - metrics = doOpenFile(filename, size); + properties = doOpenFile(filename, size); /* Otherwise, if loading from data is supported, use the callback and pass the data through to openData(). Mark the file as ready to be closed once @@ -146,24 +152,30 @@ bool AbstractFont::openFile(const Containers::StringView filename, const Float s Error() << "Text::AbstractFont::openFile(): cannot open file" << filename; return isOpened(); } - metrics = doOpenData(*data, size); + + properties = doOpenData(*data, size); _fileCallback(filename, InputFileCallbackPolicy::Close, _fileCallbackUserData); /* Shouldn't get here, the assert is fired already in setFileCallback() */ } else CORRADE_INTERNAL_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */ - _size = metrics.size; - _ascent = metrics.ascent; - _descent = metrics.descent; - _lineHeight = metrics.lineHeight; - CORRADE_INTERNAL_ASSERT(isOpened() || (!_size && !_ascent && !_descent && !_lineHeight)); - return isOpened(); + /* If opening succeeded, save the returned values. If not, the values were + set to their default values by close() already. */ + if(isOpened()) { + _size = properties.size; + _ascent = properties.ascent; + _descent = properties.descent; + _lineHeight = properties.lineHeight; + return true; + } + + return false; } -auto AbstractFont::doOpenFile(const Containers::StringView filename, const Float size) -> Metrics { +auto AbstractFont::doOpenFile(const Containers::StringView filename, const Float size) -> Properties { CORRADE_ASSERT(features() & FontFeature::OpenData, "Text::AbstractFont::openFile(): not implemented", {}); - Metrics metrics; + Properties properties; /* If callbacks are set, use them. This is the same implementation as in openFile(), see the comment there for details. */ @@ -173,7 +185,8 @@ auto AbstractFont::doOpenFile(const Containers::StringView filename, const Float Error() << "Text::AbstractFont::openFile(): cannot open file" << filename; return {}; } - metrics = doOpenData(*data, size); + + properties = doOpenData(*data, size); _fileCallback(filename, InputFileCallbackPolicy::Close, _fileCallbackUserData); /* Otherwise open the file directly */ @@ -184,19 +197,24 @@ auto AbstractFont::doOpenFile(const Containers::StringView filename, const Float return {}; } - metrics = doOpenData(*data, size); + properties = doOpenData(*data, size); } - return metrics; + return properties; } void AbstractFont::close() { - if(isOpened()) { - doClose(); - _size = 0.0f; - _lineHeight = 0.0f; - CORRADE_INTERNAL_ASSERT(!isOpened()); - } + if(!isOpened()) return; + + doClose(); + CORRADE_INTERNAL_ASSERT(!isOpened()); + + /* Clear the saved values to avoid accidental use of stale (state even + though their public access is guarded with isOpened()) */ + _size = {}; + _lineHeight = {}; + _descent = {}; + _lineHeight = {}; } Float AbstractFont::size() const { diff --git a/src/Magnum/Text/AbstractFont.h b/src/Magnum/Text/AbstractFont.h index 07b9c6ead..a6f7b15fb 100644 --- a/src/Magnum/Text/AbstractFont.h +++ b/src/Magnum/Text/AbstractFont.h @@ -496,15 +496,15 @@ class MAGNUM_TEXT_EXPORT AbstractFont: public PluginManager::AbstractPlugin { protected: /** - * @brief Font metrics + * @brief Font properties * - * @see @ref doOpenFile(), @ref doOpenData() + * Returned from @ref doOpenFile(), @ref doOpenData(). */ - struct Metrics { + struct Properties { #ifndef DOXYGEN_GENERATING_OUTPUT /* Otherwise GCC 4.8 loudly complains about missing initializers */ - constexpr /*implicit*/ Metrics() noexcept: size{}, ascent{}, descent{}, lineHeight{} {} - constexpr /*implicit*/ Metrics(Float size, Float ascent, Float descent, Float lineHeight) noexcept: size{size}, ascent{ascent}, descent{descent}, lineHeight{lineHeight} {} + constexpr /*implicit*/ Properties() noexcept: size{}, ascent{}, descent{}, lineHeight{} {} + constexpr /*implicit*/ Properties(Float size, Float ascent, Float descent, Float lineHeight) noexcept: size{size}, ascent{ascent}, descent{descent}, lineHeight{lineHeight} {} #endif /** @@ -536,8 +536,11 @@ class MAGNUM_TEXT_EXPORT AbstractFont: public PluginManager::AbstractPlugin { /** * @brief Implementation for @ref openFile() * - * Return metrics of opened font on successful opening, zeros - * otherwise. If @ref FontFeature::OpenData is supported, default + * If @ref doIsOpened() returns @cpp true @ce after calling this + * function, it's assumed that opening was successful and the + * @ref Properties are expected to contain valid values. If + * @ref doIsOpened() returns @cpp false @ce, the returned values are + * ignored. If @ref FontFeature::OpenData is supported, default * implementation opens the file and calls @ref doOpenData() with its * contents. It is allowed to call this function from your * @ref doOpenFile() implementation --- in particular, this @@ -549,7 +552,7 @@ class MAGNUM_TEXT_EXPORT AbstractFont: public PluginManager::AbstractPlugin { * supported --- instead, file is loaded though the callback and data * passed through to @ref doOpenData(). */ - virtual Metrics doOpenFile(Containers::StringView filename, Float size); + virtual Properties doOpenFile(Containers::StringView filename, Float size); private: /** @brief Implementation for @ref features() */ @@ -572,10 +575,13 @@ class MAGNUM_TEXT_EXPORT AbstractFont: public PluginManager::AbstractPlugin { /** * @brief Implementation for @ref openData() * - * Return metrics of opened font on successful opening, zeros - * otherwise. + * If @ref doIsOpened() returns @cpp true @ce after calling this + * function, it's assumed that opening was successful and the + * @ref Properties are expected to contain valid values. If + * @ref doIsOpened() returns @cpp false @ce, the returned values are + * ignored. */ - virtual Metrics doOpenData(Containers::ArrayView data, Float size); + virtual Properties doOpenData(Containers::ArrayView data, Float size); /** @brief Implementation for @ref close() */ virtual void doClose() = 0; diff --git a/src/Magnum/Text/Test/AbstractFontTest.cpp b/src/Magnum/Text/Test/AbstractFontTest.cpp index b8baf1471..0c69334c9 100644 --- a/src/Magnum/Text/Test/AbstractFontTest.cpp +++ b/src/Magnum/Text/Test/AbstractFontTest.cpp @@ -179,7 +179,7 @@ void AbstractFontTest::openData() { bool doIsOpened() const override { return _opened; } void doClose() override {} - Metrics doOpenData(const Containers::ArrayView data, Float size) override { + Properties doOpenData(const Containers::ArrayView data, Float size) override { _opened = (data.size() == 1 && data[0] == '\xa5'); return {size, 1.0f, 2.0f, 3.0f}; } @@ -209,7 +209,7 @@ void AbstractFontTest::openFileAsData() { bool doIsOpened() const override { return _opened; } void doClose() override {} - Metrics doOpenData(const Containers::ArrayView data, Float size) override { + Properties doOpenData(const Containers::ArrayView data, Float size) override { _opened = (data.size() == 1 && data[0] == '\xa5'); return {size, 1.0f, 2.0f, 3.0f}; } @@ -509,13 +509,13 @@ void AbstractFontTest::setFileCallbackOpenFileDirectly() { bool doIsOpened() const override { return _opened; } void doClose() override { _opened = false; } - Metrics doOpenFile(Containers::StringView filename, Float size) override { + Properties doOpenFile(Containers::StringView filename, Float size) override { /* Called because FileCallback is supported */ _opened = filename == "file.dat" && fileCallback() && fileCallbackUserData(); return {size, 1.0f, 2.0f, 3.0f}; } - Metrics doOpenData(Containers::ArrayView, Float) override { + Properties doOpenData(Containers::ArrayView, Float) override { /* Shouldn't be called because FileCallback is supported */ openDataCalledNotSureWhy = true; return {}; @@ -552,12 +552,12 @@ void AbstractFontTest::setFileCallbackOpenFileThroughBaseImplementation() { bool doIsOpened() const override { return _opened; } void doClose() override { _opened = false; } - Metrics doOpenFile(Containers::StringView filename, Float size) override { + Properties doOpenFile(Containers::StringView filename, Float size) override { openFileCalled = filename == "file.dat" && fileCallback() && fileCallbackUserData(); return AbstractFont::doOpenFile(filename, size); } - Metrics doOpenData(Containers::ArrayView data, Float size) override { + Properties doOpenData(Containers::ArrayView data, Float size) override { _opened = (data.size() == 1 && data[0] == '\xb0'); return {size, 1.0f, 2.0f, 3.0f}; } @@ -610,7 +610,7 @@ void AbstractFontTest::setFileCallbackOpenFileThroughBaseImplementationFailed() bool doIsOpened() const override { return false; } void doClose() override {} - Metrics doOpenFile(Containers::StringView filename, Float size) override { + Properties doOpenFile(Containers::StringView filename, Float size) override { openFileCalled = true; return AbstractFont::doOpenFile(filename, size); } @@ -642,12 +642,12 @@ void AbstractFontTest::setFileCallbackOpenFileAsData() { bool doIsOpened() const override { return _opened; } void doClose() override { _opened = false; } - Metrics doOpenFile(Containers::StringView, Float) override { + Properties doOpenFile(Containers::StringView, Float) override { openFileCalled = true; return {}; } - Metrics doOpenData(Containers::ArrayView data, Float size) override { + Properties doOpenData(Containers::ArrayView data, Float size) override { _opened = (data.size() == 1 && data[0] == '\xb0'); return {size, 1.0f, 2.0f, 3.0f}; } @@ -701,7 +701,7 @@ void AbstractFontTest::setFileCallbackOpenFileAsDataFailed() { bool doIsOpened() const override { return false; } void doClose() override {} - Metrics doOpenFile(Containers::StringView, Float) override { + Properties doOpenFile(Containers::StringView, Float) override { openFileCalled = true; return {}; } @@ -733,7 +733,7 @@ void AbstractFontTest::properties() { bool doIsOpened() const override { return _opened; } void doClose() override {} - Metrics doOpenData(const Containers::ArrayView, Float size) override { + Properties doOpenData(const Containers::ArrayView, Float size) override { _opened = true; return {size, 1.0f, 2.0f, 3.0f}; } diff --git a/src/Magnum/Text/Test/RendererGLTest.cpp b/src/Magnum/Text/Test/RendererGLTest.cpp index f04c0faf9..702d4afba 100644 --- a/src/Magnum/Text/Test/RendererGLTest.cpp +++ b/src/Magnum/Text/Test/RendererGLTest.cpp @@ -348,7 +348,7 @@ void RendererGLTest::multiline() { bool doIsOpened() const override { return _opened; } void doClose() override { _opened = false; } - Metrics doOpenFile(Containers::StringView, Float) override { + Properties doOpenFile(Containers::StringView, Float) override { _opened = true; return {0.5f, 0.45f, -0.25f, 0.75f}; } diff --git a/src/MagnumPlugins/MagnumFont/MagnumFont.cpp b/src/MagnumPlugins/MagnumFont/MagnumFont.cpp index cc7926ec9..61bf7387d 100644 --- a/src/MagnumPlugins/MagnumFont/MagnumFont.cpp +++ b/src/MagnumPlugins/MagnumFont/MagnumFont.cpp @@ -82,7 +82,7 @@ bool MagnumFont::doIsOpened() const { return _opened && _opened->image; } void MagnumFont::doClose() { _opened = nullptr; } -auto MagnumFont::doOpenData(const Containers::ArrayView data, const Float) -> Metrics { +auto MagnumFont::doOpenData(const Containers::ArrayView data, const Float) -> Properties { if(!_opened) _opened.emplace(); if(!_opened->filePath && !fileCallback()) { @@ -137,7 +137,7 @@ auto MagnumFont::doOpenData(const Containers::ArrayView data, const _opened->conf.value("lineHeight")}; } -auto MagnumFont::doOpenFile(const Containers::StringView filename, const Float size) -> Metrics { +auto MagnumFont::doOpenFile(const Containers::StringView filename, const Float size) -> Properties { _opened.emplace(); _opened->filePath.emplace(Utility::Path::split(filename).first()); diff --git a/src/MagnumPlugins/MagnumFont/MagnumFont.h b/src/MagnumPlugins/MagnumFont/MagnumFont.h index 0e6bad56d..1c1d56268 100644 --- a/src/MagnumPlugins/MagnumFont/MagnumFont.h +++ b/src/MagnumPlugins/MagnumFont/MagnumFont.h @@ -172,8 +172,8 @@ class MAGNUM_MAGNUMFONT_EXPORT MagnumFont: public AbstractFont { private: MAGNUM_MAGNUMFONT_LOCAL FontFeatures doFeatures() const override; MAGNUM_MAGNUMFONT_LOCAL bool doIsOpened() const override; - MAGNUM_MAGNUMFONT_LOCAL Metrics doOpenData(Containers::ArrayView data, Float) override; - MAGNUM_MAGNUMFONT_LOCAL Metrics doOpenFile(Containers::StringView filename, Float) override; + MAGNUM_MAGNUMFONT_LOCAL Properties doOpenData(Containers::ArrayView data, Float) override; + MAGNUM_MAGNUMFONT_LOCAL Properties doOpenFile(Containers::StringView filename, Float) override; MAGNUM_MAGNUMFONT_LOCAL void doClose() override; MAGNUM_MAGNUMFONT_LOCAL UnsignedInt doGlyphId(char32_t character) override; diff --git a/src/MagnumPlugins/MagnumFontConverter/Test/MagnumFontConverterTest.cpp b/src/MagnumPlugins/MagnumFontConverter/Test/MagnumFontConverterTest.cpp index 0211e8f1e..37ca2ca34 100644 --- a/src/MagnumPlugins/MagnumFontConverter/Test/MagnumFontConverterTest.cpp +++ b/src/MagnumPlugins/MagnumFontConverter/Test/MagnumFontConverterTest.cpp @@ -93,7 +93,7 @@ void MagnumFontConverterTest::exportFont() { private: void doClose() override { _opened = false; } bool doIsOpened() const override { return _opened; } - Metrics doOpenFile(Containers::StringView, Float) override { + Properties doOpenFile(Containers::StringView, Float) override { _opened = true; return {16.0f, 25.0f, -10.0f, 39.7333f}; }