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.
3 years ago · 7ead74950b
7 changed files with 80 additions and 56 deletions
--- a/src/Magnum/Text/AbstractFont.cpp
+++ b/src/Magnum/Text/AbstractFont.cpp
@ -105,27 +105,33 @@ bool AbstractFont::openData(Containers::ArrayView<const void> 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<const char>(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<const char>(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<const char>, Float) -> Metrics {
+auto AbstractFont::doOpenData(Containers::ArrayView<const char>, 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 {
--- 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<const char> data, Float size);
+        virtual Properties doOpenData(Containers::ArrayView<const char> data, Float size);

        /** @brief Implementation for @ref close() */
        virtual void doClose() = 0;
--- 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<const char> data, Float size) override {
+        Properties doOpenData(const Containers::ArrayView<const char> 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<const char> data, Float size) override {
+        Properties doOpenData(const Containers::ArrayView<const char> 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<const char>, Float) override {
+        Properties doOpenData(Containers::ArrayView<const char>, 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<const char> data, Float size) override {
+        Properties doOpenData(Containers::ArrayView<const char> 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<const char> data, Float size) override {
+        Properties doOpenData(Containers::ArrayView<const char> 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<const char>, Float size) override {
+        Properties doOpenData(const Containers::ArrayView<const char>, Float size) override {
            _opened = true;
            return {size, 1.0f, 2.0f, 3.0f};
        }
--- 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};
            }
--- 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<const char> data, const Float) -> Metrics {
+auto MagnumFont::doOpenData(const Containers::ArrayView<const char> data, const Float) -> Properties {
    if(!_opened) _opened.emplace();

    if(!_opened->filePath && !fileCallback()) {
@ -137,7 +137,7 @@ auto MagnumFont::doOpenData(const Containers::ArrayView<const char> data, const
            _opened->conf.value<Float>("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());

--- 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<const char> data, Float) override;
-        MAGNUM_MAGNUMFONT_LOCAL Metrics doOpenFile(Containers::StringView filename, Float) override;
+        MAGNUM_MAGNUMFONT_LOCAL Properties doOpenData(Containers::ArrayView<const char> 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;
--- 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};
            }