Text: rethink AbstractShaper::shape() empty input and failure behavior.

Empty input was asserting originally, but such strict behavior doesn't
seem to be useful in practice as all application code would need to
explicitly make sure that shape() isn't called if the input or the input
range is empty. In particular, I hit this assert as soon as I rebuilt
and ran magnum-player, and the assertion didn't really uncover any bug
or make anything better by firing.

Treating zero glyph output as a failure also isn't good -- rendering
for example a string of whitespace may also result in zero glyphs on the
output, which isn't a failure. Plus it's currently unclear what should
actually be a failure -- neither FreeType nor HarfBuzz really have a
concept of the shaping operation failing, hb_shape() returns void -- so
special-casing glyphCount() being zero as a failure just doesn't make
sense.

This thus means the doScript(), doLanguage() and doDirection()
implementations are now called always, regardless of whether shape() was
called at all, or whether it produced any glyphs. This allows the shaper
for example to give back a detected script etc even if the actual
input range to shape was empty.

src/Magnum/Text/AbstractShaper.cpp

@@ -80,8 +80,6 @@ UnsignedInt AbstractShaper::shape(const Containers::StringView text, const Unsig
     CORRADE_ASSERT((end == ~UnsignedInt{} && begin <= text.size()) ||
                    (begin <= end && end <= text.size()),
         "Text::AbstractShaper::shape(): begin" << begin << "and end" << end << "out of range for a text of" << text.size() << "bytes", {});
-    CORRADE_ASSERT(begin < text.size() && begin != end,
-        "Text::AbstractShaper::shape(): shaped text at begin" << begin << "is empty", {});
     #ifndef CORRADE_NO_ASSERT
     for(std::size_t i = 0; i != features.size(); ++i) {
         const FeatureRange& feature = features[i];
@@ -89,7 +87,6 @@ UnsignedInt AbstractShaper::shape(const Containers::StringView text, const Unsig
             (feature._end == ~UnsignedInt{} && feature._begin <= text.size()) ||
             (feature._begin <= feature._end && feature._end <= text.size()),
             "Text::AbstractShaper::shape(): feature" << i << "begin" << feature._begin << "and end" << feature._end << "out of range for a text of" << text.size() << "bytes", {});
-        /** @todo catch empty feature ranges too? or not important */
     }
     #endif
     return _glyphCount = doShape(text, begin, end, features);
@@ -116,19 +113,19 @@ UnsignedInt AbstractShaper::shape(const Containers::StringView text) {
 }
 
 Script AbstractShaper::script() const {
-    return _glyphCount ? doScript() : Script::Unspecified;
+    return doScript();
 }
 
 Script AbstractShaper::doScript() const { return Script::Unspecified; }
 
 Containers::StringView AbstractShaper::language() const {
-    return _glyphCount ? doLanguage() : Containers::StringView{};
+    return doLanguage();
 }
 
 Containers::StringView AbstractShaper::doLanguage() const { return {}; }
 
 Direction AbstractShaper::direction() const {
-    return _glyphCount ? doDirection() : Direction::Unspecified;
+    return doDirection();
 }
 
 Direction AbstractShaper::doDirection() const { return Direction::Unspecified; }

src/Magnum/Text/AbstractShaper.h

@@ -344,18 +344,13 @@ class MAGNUM_TEXT_EXPORT AbstractShaper {
          * @param features  OpenType features to apply for the whole text or
          *      its subranges
          *
-         * Expects that @p text is non-empty, that both @p begin and all
-         * @ref FeatureRange::begin() are contained within @p text, and that
-         * @p end and all @ref FeatureRange::end() are either contained within
-         * @p text or have a value of @cpp 0xffffffffu @ce. On success returns
-         * the number of shaped glyphs (which is also subsequently available
-         * through @ref glyphCount() const) and updates the @ref script() const,
-         * @ref language() const and @ref direction() const values.
-         *
-         * On failure, such as when the input is not valid UTF-8 or when
-         * specified combination or script, language and direction is
-         * unsupported, prints a message to @relativeref{Magnum,Error} and
-         * returns @cpp 0 @ce.
+         * Expects that both @p begin and all @ref FeatureRange::begin() are
+         * contained within @p text, and that @p end and all
+         * @ref FeatureRange::end() are either contained within @p text or have
+         * a value of @cpp 0xffffffffu @ce. Returns the number of shaped glyphs
+         * (which is also subsequently available through @ref glyphCount() const)
+         * and updates the @ref script() const, @ref language() const and
+         * @ref direction() const values.
          *
          * Whether @p features are used depends on a particular
          * @ref AbstractFont plugin implementation and the font file itself as
@@ -415,9 +410,9 @@ class MAGNUM_TEXT_EXPORT AbstractShaper {
         /**
          * @brief Script used for the last @ref shape() call
          *
-         * If the last @ref shape() call failed, it hasn't been called yet or
-         * if the @ref AbstractFont doesn't implement any script-specific
-         * behavior, returns @ref Script::Unspecified.
+         * May return @ref Script::Unspecified if @ref shape() hasn't been
+         * called yet or if the @ref AbstractFont doesn't implement any
+         * script-specific behavior.
          * @see @ref setScript(), @ref language(), @ref direction()
          */
         Script script() const;
@@ -425,9 +420,9 @@ class MAGNUM_TEXT_EXPORT AbstractShaper {
         /**
          * @brief Language used for the last @ref shape() call
          *
-         * If the last @ref shape() call failed, it hasn't been called yet or
+         * May return an empty string if @ref shape() hasn't been called yet or
          * if the @ref AbstractFont doesn't implement any language-specific
-         * behavior, returns an empty string.
+         * behavior.
          *
          * The returned view is generally neither
          * @relativeref{Corrade,Containers::StringViewFlag::Global} nor
@@ -443,9 +438,9 @@ class MAGNUM_TEXT_EXPORT AbstractShaper {
         /**
          * @brief Language used for the last @ref shape() call
          *
-         * If the last @ref shape() call failed, it hasn't been called yet or
-         * if the @ref AbstractFont doesn't implement any direction-specific
-         * behavior, returns @ref Direction::Unspecified.
+         * May return @ref Direction::Unspecified if @ref shape() hasn't been
+         * called yet or if the @ref AbstractFont doesn't implement any
+         * script-specific behavior.
          * @see @ref setDirection(), @ref script(), @ref language()
          */
         Direction direction() const;

src/Magnum/Text/Test/AbstractShaperTest.cpp

@@ -64,9 +64,8 @@ struct AbstractShaperTest: TestSuite::Tester {
     void shapeNoBeginEnd();
     void shapeNoBeginEndFeatures();
     void shapeScriptLanguageDirectionNotImplemented();
-    void shapeFailed();
+    void shapeZeroGlyphs();
     void shapeBeginEndOutOfRange();
-    void shapeEmptyText();
 
     /* glyphsInto() tested in shape() already */
     void glyphsIntoEmpty();
@@ -97,9 +96,8 @@ AbstractShaperTest::AbstractShaperTest() {
               &AbstractShaperTest::shapeNoBeginEnd,
               &AbstractShaperTest::shapeNoBeginEndFeatures,
               &AbstractShaperTest::shapeScriptLanguageDirectionNotImplemented,
-              &AbstractShaperTest::shapeFailed,
+              &AbstractShaperTest::shapeZeroGlyphs,
               &AbstractShaperTest::shapeBeginEndOutOfRange,
-              &AbstractShaperTest::shapeEmptyText,
 
               &AbstractShaperTest::glyphsIntoEmpty,
               &AbstractShaperTest::glyphsIntoInvalidViewSizes});
@@ -352,13 +350,15 @@ void AbstractShaperTest::shape() {
         bool shapeCalled = false;
     } shaper{FakeFont};
 
-    /* Initially it shouldn't call into any of the implementations */
+    /* There's no special behavior, it calls into the implementations even if
+       nothing has been shaped yet */
     CORRADE_COMPARE(shaper.glyphCount(), 0);
-    CORRADE_COMPARE(shaper.script(), Script::Unspecified);
-    CORRADE_COMPARE(shaper.language(), "");
-    CORRADE_COMPARE(shaper.direction(), Direction::Unspecified);
+    CORRADE_COMPARE(shaper.script(), Script::LinearA);
+    CORRADE_COMPARE(shaper.language(), "eh-UH");
+    CORRADE_COMPARE(shaper.direction(), Direction::BottomToTop);
 
-    /* Shaping fills glyph count and allows calling into the implementations */
+    /* Shaping fills glyph count. A real implementation would then return
+       (different) detected script/language/direction values, for example. */
     CORRADE_COMPARE(shaper.shape("some text", 3, 8, {
         Feature::ContextualLigatures,
         {Feature::Kerning, 2, 5, false}
@@ -496,7 +496,7 @@ void AbstractShaperTest::shapeScriptLanguageDirectionNotImplemented() {
     CORRADE_COMPARE(shaper.direction(), Direction::Unspecified);
 }
 
-void AbstractShaperTest::shapeFailed() {
+void AbstractShaperTest::shapeZeroGlyphs() {
     struct: AbstractShaper {
         using AbstractShaper::AbstractShaper;
 
@@ -519,16 +519,16 @@ void AbstractShaperTest::shapeFailed() {
         void doGlyphsInto(const Containers::StridedArrayView1D<UnsignedInt>&, const Containers::StridedArrayView1D<Vector2>&, const Containers::StridedArrayView1D<Vector2>&) const override {}
     } shaper{FakeFont};
 
-    /* The implementation is responsible for printing a message, the base class
-       doesn't */
     CORRADE_COMPARE(shaper.shape("some text", 3, 8), 0);
 
-    /* After a failure it shouldn't call into any of the implementations
-       either */
+    /* It calls into the implementations even in case no glyphs were actually
+       shaped. It could be for example a zero-length slice of a larger string
+       for which script/language/direction detection was performed, so it's
+       still useful to get the values after */
     CORRADE_COMPARE(shaper.glyphCount(), 0);
-    CORRADE_COMPARE(shaper.script(), Script::Unspecified);
-    CORRADE_COMPARE(shaper.language(), "");
-    CORRADE_COMPARE(shaper.direction(), Direction::Unspecified);
+    CORRADE_COMPARE(shaper.script(), Script::LinearA);
+    CORRADE_COMPARE(shaper.language(), "eh-UH");
+    CORRADE_COMPARE(shaper.direction(), Direction::BottomToTop);
 }
 
 void AbstractShaperTest::shapeBeginEndOutOfRange() {
@@ -586,35 +586,6 @@ void AbstractShaperTest::shapeBeginEndOutOfRange() {
         TestSuite::Compare::String);
 }
 
-void AbstractShaperTest::shapeEmptyText() {
-    CORRADE_SKIP_IF_NO_ASSERT();
-
-    struct: AbstractShaper {
-        using AbstractShaper::AbstractShaper;
-
-        UnsignedInt doShape(Containers::StringView, UnsignedInt, UnsignedInt, Containers::ArrayView<const FeatureRange>) override {
-            CORRADE_FAIL("This shouldn't be called");
-            return 5;
-        }
-
-        void doGlyphsInto(const Containers::StridedArrayView1D<UnsignedInt>&, const Containers::StridedArrayView1D<Vector2>&, const Containers::StridedArrayView1D<Vector2>&) const override {}
-    } shaper{FakeFont};
-
-    /* Capture correct function name */
-    CORRADE_VERIFY(true);
-
-    std::ostringstream out;
-    Error redirectError{&out};
-    shaper.shape("");
-    shaper.shape("hello", 3, 3);
-    shaper.shape("hello", 5, ~UnsignedInt{});
-    CORRADE_COMPARE_AS(out.str(),
-        "Text::AbstractShaper::shape(): shaped text at begin 0 is empty\n"
-        "Text::AbstractShaper::shape(): shaped text at begin 3 is empty\n"
-        "Text::AbstractShaper::shape(): shaped text at begin 5 is empty\n",
-        TestSuite::Compare::String);
-}
-
 void AbstractShaperTest::glyphsIntoEmpty() {
     struct: AbstractShaper {
         using AbstractShaper::AbstractShaper;

src/MagnumPlugins/MagnumFont/Test/MagnumFontTest.cpp

@@ -54,6 +54,7 @@ struct MagnumFontTest: TestSuite::Tester {
     void properties();
 
     void shape();
+    void shapeEmpty();
     void shaperReuse();
 
     void fileCallbackImage();
@@ -83,7 +84,8 @@ MagnumFontTest::MagnumFontTest() {
     addInstancedTests({&MagnumFontTest::shape},
         Containers::arraySize(ShapeData));
 
-    addTests({&MagnumFontTest::shaperReuse,
+    addTests({&MagnumFontTest::shapeEmpty,
+              &MagnumFontTest::shaperReuse,
 
               &MagnumFontTest::fileCallbackImage,
               &MagnumFontTest::fileCallbackImageNotFound});
@@ -169,14 +171,28 @@ void MagnumFontTest::shape() {
     }), TestSuite::Compare::Container);
 }
 
+void MagnumFontTest::shapeEmpty() {
+    Containers::Pointer<AbstractFont> font = _fontManager.instantiate("MagnumFont");
+    CORRADE_VERIFY(font->openFile(Utility::Path::join(MAGNUMFONT_TEST_DIR, "font.conf"), 0.0f));
+
+    Containers::Pointer<AbstractShaper> shaper = font->createShaper();
+
+    /* Shouldn't crash or do anything rogue */
+    CORRADE_COMPARE(shaper->shape("Wave", 2, 2), 0);
+}
+
 void MagnumFontTest::shaperReuse() {
     Containers::Pointer<AbstractFont> font = _fontManager.instantiate("MagnumFont");
     CORRADE_VERIFY(font->openFile(Utility::Path::join(MAGNUMFONT_TEST_DIR, "font.conf"), 0.0f));
 
     Containers::Pointer<AbstractShaper> shaper = font->createShaper();
 
-    /* Short text */
+    /* Empty text */
     {
+        CORRADE_COMPARE(shaper->shape("Wave", 2, 2), 0);
+
+    /* Short text. Empty shape shouldn't have caused any broken state. */
+    } {
         CORRADE_COMPARE(shaper->shape("We"), 2);
         UnsignedInt ids[2];
         Vector2 offsets[2];