From eaaa34b3fb46765dad2cdfdac7c442f6ddc1408b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Thu, 12 Oct 2023 18:49:05 +0200 Subject: [PATCH] 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 | 9 +-- src/Magnum/Text/AbstractShaper.h | 35 +++++------ src/Magnum/Text/Test/AbstractShaperTest.cpp | 63 +++++-------------- .../MagnumFont/Test/MagnumFontTest.cpp | 20 +++++- 4 files changed, 53 insertions(+), 74 deletions(-) diff --git a/src/Magnum/Text/AbstractShaper.cpp b/src/Magnum/Text/AbstractShaper.cpp index 0712787a6..024d129ee 100644 --- a/src/Magnum/Text/AbstractShaper.cpp +++ b/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; } diff --git a/src/Magnum/Text/AbstractShaper.h b/src/Magnum/Text/AbstractShaper.h index 779158956..f332b732c 100644 --- a/src/Magnum/Text/AbstractShaper.h +++ b/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; diff --git a/src/Magnum/Text/Test/AbstractShaperTest.cpp b/src/Magnum/Text/Test/AbstractShaperTest.cpp index 7a0eef195..5c6a616bd 100644 --- a/src/Magnum/Text/Test/AbstractShaperTest.cpp +++ b/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&, const Containers::StridedArrayView1D&, const Containers::StridedArrayView1D&) 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) override { - CORRADE_FAIL("This shouldn't be called"); - return 5; - } - - void doGlyphsInto(const Containers::StridedArrayView1D&, const Containers::StridedArrayView1D&, const Containers::StridedArrayView1D&) 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; diff --git a/src/MagnumPlugins/MagnumFont/Test/MagnumFontTest.cpp b/src/MagnumPlugins/MagnumFont/Test/MagnumFontTest.cpp index a32864f19..b1440fce0 100644 --- a/src/MagnumPlugins/MagnumFont/Test/MagnumFontTest.cpp +++ b/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 font = _fontManager.instantiate("MagnumFont"); + CORRADE_VERIFY(font->openFile(Utility::Path::join(MAGNUMFONT_TEST_DIR, "font.conf"), 0.0f)); + + Containers::Pointer shaper = font->createShaper(); + + /* Shouldn't crash or do anything rogue */ + CORRADE_COMPARE(shaper->shape("Wave", 2, 2), 0); +} + void MagnumFontTest::shaperReuse() { Containers::Pointer font = _fontManager.instantiate("MagnumFont"); CORRADE_VERIFY(font->openFile(Utility::Path::join(MAGNUMFONT_TEST_DIR, "font.conf"), 0.0f)); Containers::Pointer 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];