From 8ab9e4544e0485477fdc3c53df31eaa86fedfce9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C4=81rlis=20Se=C5=86ko?= Date: Fri, 12 Dec 2025 19:12:52 +0200 Subject: [PATCH] Estimate cursor position within ligatures For ligatures like ffi or ff it should still be possible to select each individual letter. Estimate the position by dividing glyph width. Not a perfect solution since the ligature can visually look completely different than side by side character which created it. Not as bad as it might since the underlying text layout libraries already filter out good chunk of cases that shouldn't allow placing cursor in the middle. This commit does not change the set of possible cursor positions as it was already possible to select them using keyboard. It only improves the visual cursor position which allows selecting them using mouse as well and makes keyboard navigation clearer. Fixes https://gitlab.com/inkscape/inkscape/-/issues/5972 --- src/libnrtype/Layout-TNG-Compute.cpp | 39 +++++ testfiles/CMakeLists.txt | 10 ++ .../src/libnrtype/Layout-TNG-Compute.cpp | 147 ++++++++++++++++++ 3 files changed, 196 insertions(+) create mode 100644 testfiles/src/libnrtype/Layout-TNG-Compute.cpp diff --git a/src/libnrtype/Layout-TNG-Compute.cpp b/src/libnrtype/Layout-TNG-Compute.cpp index be77c7f188..1f7b3bcc1e 100644 --- a/src/libnrtype/Layout-TNG-Compute.cpp +++ b/src/libnrtype/Layout-TNG-Compute.cpp @@ -11,6 +11,7 @@ */ #include +#include #include "Layout-TNG.h" #include "style.h" @@ -281,6 +282,8 @@ class Layout::Calculator return para.char_attributes[span_pos.iter_span->char_index_in_para + span_pos.char_index]; } + void _estimateLigatureSubcomponents(std::span characters, Glyph &glyph, int positions, float direction); + #ifdef DEBUG_LAYOUT_TNG_COMPUTE static void dumpPangoItemsOut(ParagraphInfo *para); static void dumpUnbrokenSpans(ParagraphInfo *para); @@ -1117,6 +1120,8 @@ void Layout::Calculator::_outputLine(ParagraphInfo const ¶, } double advance_width = new_glyph.advance; + int cursor_positions = 0; // number of selectable positions within glyph + auto first_char_pos = _flow._characters.size(); while (char_byte < end_byte) { /* Hack to survive ligatures: in log_cluster keep the number of available chars >= number of glyphs remaining. @@ -1135,6 +1140,9 @@ void Layout::Calculator::_outputLine(ParagraphInfo const ¶, new_character.char_attributes = para.char_attributes[unbroken_span.char_index_in_para + char_index_in_unbroken_span]; new_character.in_glyph = (hidden ? -1 : _flow._glyphs.size() - 1); _flow._characters.push_back(new_character); + if (new_character.char_attributes.is_cursor_position) { + cursor_positions++; + } // Letter/word spacing and justification if (new_character.char_attributes.is_expandable_space) @@ -1149,6 +1157,10 @@ void Layout::Calculator::_outputLine(ParagraphInfo const ¶, char_byte = iter_source_text.base() - unbroken_span.input_stream_first_character.base(); log_cluster_size_chars--; } + _estimateLigatureSubcomponents( + std::span(_flow._characters.begin() + first_char_pos, _flow._characters.end()), + _flow._glyphs.back(), cursor_positions, + direction_sign * (new_span.direction != para.direction ? -1 : 1)); // Update x position variables advance_width *= direction_sign; @@ -2060,6 +2072,33 @@ bool Layout::Calculator::_buildChunksInScanRun(ParagraphInfo const ¶, return true; } +/** + * Estimate subcomponent positions within ligatures + * + * Some fonts use ligatures like ff ffi fi ij for finetuning the look of certain character sequences. In such cases + * it should still be possible select individual letters even though they are merged into single glyph. + * + * For various non latin based script the result is less consistent. Some of them stack the symbols in every possible + * direction. CJK ones tend to stack in groups of 4-6. "is_cursor_position" helps filter out good chunk of cases + * where splitting ligature doesn't make sense. + */ +void Layout::Calculator::_estimateLigatureSubcomponents(std::span characters, Glyph &glyph, int positions, + float direction) +{ + if (positions <= 1) { + return; + } + int index = 0; + for (auto &character : characters) { + if (character.char_attributes.is_cursor_position) { + if (index > 0) { + character.x += direction * (glyph.advance * index) / positions; + } + index++; + } + } +} + #ifdef DEBUG_LAYOUT_TNG_COMPUTE /** * For debugging, not called in distributed code diff --git a/testfiles/CMakeLists.txt b/testfiles/CMakeLists.txt index 3ab9ede887..d0ea62c51e 100644 --- a/testfiles/CMakeLists.txt +++ b/testfiles/CMakeLists.txt @@ -55,6 +55,10 @@ if(${CMAKE_SIZEOF_VOID_P} EQUAL 8) ) endif() +set(LIBNRTYPE_TESTS + libnrtype/Layout-TNG-Compute + ) + # Integration tests (linking all of Inkscape) set(TEST_SOURCES actions-svg-processing @@ -120,6 +124,7 @@ set(TEST_SOURCES object-colors-test multi-marker-color-wheel-test ${LPE_TESTS_64bit} + ${LIBNRTYPE_TESTS} ) if(WITH_CAPYPDF) @@ -142,6 +147,11 @@ foreach(test_source ${TEST_SOURCES}) set_tests_properties(${testname} PROPERTIES ENVIRONMENT "${INKSCAPE_TEST_PROFILE_DIR_ENV}/${testname};${CMAKE_CTEST_ENV}") add_dependencies(tests ${testname}) endforeach() +foreach(test_source ${LIBNRTYPE_TESTS}) + string(REPLACE "/" "_" testname "test_${test_source}") + set_property(TEST ${testname} APPEND PROPERTY ENVIRONMENT "INKSCAPE_FONTCONFIG=${CMAKE_CURRENT_SOURCE_DIR}/rendering_tests/fonts/isolated.conf") +endforeach() + include(${CMAKE_SOURCE_DIR}/CMakeScripts/UnitTest.cmake) ### Unit tests diff --git a/testfiles/src/libnrtype/Layout-TNG-Compute.cpp b/testfiles/src/libnrtype/Layout-TNG-Compute.cpp new file mode 100644 index 0000000000..07fac635e1 --- /dev/null +++ b/testfiles/src/libnrtype/Layout-TNG-Compute.cpp @@ -0,0 +1,147 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Unit tests for Layout computations. + * + * Copyright (C) 2025 Authors + * + * Released under GNU GPL v2+, read the file 'COPYING' for more information. + */ + +#include + +#include "document.h" +#include "inkscape.h" +#include "object/sp-item.h" +#include "object/sp-text.h" +#include "svg/svg.h" + +using namespace std::literals; + +struct LayoutPositionTestData +{ + char const *id; + int expected_glyphs; + int expected_positions; + std::vector permutation = {}; + int expected_characters = -1; + Geom::Dim2 direction = Geom::Dim2::X; +}; + +// clang-format off +LayoutPositionTestData const LIGATURE_CURSOR_TESTS[] = { + {"id0", 3, 6}, + {"id1", 4, 7}, + {"id2", 4, 5, {4, 3, 2, 1, 0}}, + {"id3", 3, 5, {4, 3, 2, 1, 0}}, + {"id5_precheck", 5, 6, {5, 1, 3, 4, 1, 0}}, + {"id5", 5, 7, {6, 1, 3, 4, 5, 1, 0}}, + {"id6", 3, 5, {0, 2, 1, 2, 4}}, + {"id7", 3, 4, {}, 4}, + {"id8", 3, 4, {}, 5}, + {"id9", 2, 6, {}, -1, Geom::Dim2::Y}, + {"id10", 4, 6, {}, -1, Geom::Dim2::Y}, + {"id11", 2, 6, {}, -1, Geom::Dim2::Y}, +}; +// clang-format on + +class LayoutTNGComputeTestFixture : public ::testing::TestWithParam +{ +public: + static void SetUpTestSuite() + { + // SPDocument currently depends on this + Inkscape::Application::create(false); + + constexpr auto svg = R"""( + + affib + affibb + ﮎﮎﮎﮎ + ﮎﻋﺞﮎ + + + צabcצ + + צafiaצ + + + aﻋﺞa + + + aǪa + aᄀᆞᆮa + + + ffi明治 + ffi明治 + ffi明治 + + + )"""sv; + + document = SPDocument::createNewDocFromMem(svg); + document->ensureUpToDate(); + } + + static void TearDownTestSuite() { document.reset(); } + +protected: + static std::unique_ptr document; + SPText *text = nullptr; + + void SetUp() override + { + text = cast(document->getObjectById(GetParam().id)); + ASSERT_NE(text, nullptr); + } + + Inkscape::Text::Layout &GetLayout() { return text->layout; } +}; + +std::unique_ptr LayoutTNGComputeTestFixture::document; + +INSTANTIATE_TEST_SUITE_P(LayoutTNGComputeTest, LayoutTNGComputeTestFixture, testing::ValuesIn(LIGATURE_CURSOR_TESTS)); + +TEST_P(LayoutTNGComputeTestFixture, CursorPositionsInsideLigature) +{ + auto &config = GetParam(); + auto &layout = GetLayout(); + + // Incorrect glyph count implies that ligature didn't get applied. + // This check is important, if the ligature isn't applied rest of the test will succeed without detecting + // any errors in code it was supposed test. + ASSERT_EQ(layout.glyphs().size(), config.expected_glyphs); + + auto it = layout.begin(); + auto last_x = layout.characterAnchorPoint(it)[config.direction]; + auto position_count = 1; // start + std::vector positions; + positions.push_back(last_x); + while (it.nextCursorPosition()) { + position_count++; + auto pos = layout.characterAnchorPoint(it); + if (config.permutation.empty()) { + EXPECT_GT(pos[config.direction], last_x); + } + last_x = pos[config.direction]; + positions.push_back(last_x); + }; + positions.push_back(layout.characterAnchorPoint(it)[config.direction]); + position_count += 1; // end position + EXPECT_EQ(position_count, config.expected_positions); + if (config.expected_characters > 0) { + auto last_char = it; + last_char.prevCharacter(); + EXPECT_EQ(layout.iteratorToCharIndex(it), config.expected_characters); + } + if (config.permutation.size()) { + ASSERT_EQ(positions.size(), config.permutation.size()); + std::vector actual_permutation(positions.size()); + for (int i = 0; i < positions.size(); i++) { + auto actual_pos = + std::count_if(positions.begin(), positions.end(), [&](float v) { return v < positions[i]; }); + actual_permutation[i] = actual_pos; + } + EXPECT_EQ(actual_permutation, config.permutation); + } +} -- GitLab