[guilib] Decouple left/right text truncate from alignment #24799

CastagnaIT · 2024-03-03T16:41:44Z

Description

This PR introduce the left text truncate setting.

The problem is started with the GUI control improvements PR #22691 where in the past i tried to introduce the left truncate, but has caused some issues with the right text alignment. After that i had tried to fix with PR #24624, but was not so perfect solution (this PR also revert that changes).

The right solution is decouple left/right truncate so that can be configured without strictly depends from alignment.

XBFONT_TRUNCATED_LEFT was inevitably added and should extend the "alignment" settings for python addons without particular impact

Having improved the code, I took the opportunity to include an additional fix for the use case having <align>center</align> and OVER_FLOW to TRUNCATE, was already broken from oldest kodi versions (screenshots for center case below)

These changes keep same behaviours for GUI settings improvements as shown on screenshots of PR #22691

In case of rebranch the backport to Kodi 21 is mandatory for the final release.

Note 1 for <align>justify</align> / XBFONT_JUSTIFIED:
This alignment was already broken, and will not be addressed with this PR

Note 2: the new left truncate can have an additional use for RTL text but requires other improvements

Motivation and context

fix #24725

How has this been tested?

user confirmed no regressions:
#24725 (comment)
#24610 (comment)

Used custom python addon with a xml window and with:

label control <control type="label" id="10000">
fadelabel <control type="fadelabel" id="10000"> (note this control perform truncate without ellipses)
textbox <control type="textbox" id="10000">

CURRENT MASTER BRANCH (BEFORE):
Here i show only broken use cases

Multilines both lines are short, dont exceed the container width, and then text "truncate" is not applied
<align>right</align>

Multilines both lines are too long, exceed the container width, and text "truncate to RIGHT" is applied
<align>center</align> (broken already before Kodi 20)

<align>right</align>

WITH PR (NOW):

Multilines both lines are short, dont exceed the container width, and then text "truncate" is not applied
<align>left</align>

<align>center</align>

<align>right</align>

Multilines both lines are too long, exceed the container width, and text "truncate to RIGHT" is applied
<align>left</align>

<align>center</align>

<align>right</align>

Only for sake of completness, example of truncate from LEFT, on right aligned text:

What is the effect on users?

Various type of GUI controls used on skins should display text as expected

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
Student submission (PR was done for educational purposes and will be treated as such)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

xbmc/interfaces/legacy/Control.h

CrystalP · 2024-03-10T19:39:58Z

xbmc/guilib/GUIFontTTF.cpp

      }
    }
+    else
+    {
+      // Determines the maximum width the text can have within the maximum width


not very clear, defines the max width with the max width?

"Calculates the text width based on the characters that can be contained within the maximum width"
better?

xbmc/guilib/GUIFontTTF.cpp

CrystalP · 2024-03-10T19:42:14Z

xbmc/guilib/GUIFontTTF.cpp

+    if (alignment & XBFONT_CENTER_X && !(alignment & XBFONT_RIGHT) && !(alignment & XBFONT_LEFT))
+    {
+      if (alignment & XBFONT_CENTER_X)


Which case is the first if() meant to filter? There is overlap with the second if()

A validation that alignment doesn't contain conflicting flags could be good but would belong to the beginning of the function (or its caller), would have some error logging and would override the flags to go back to a normal situation for the rest of the code.

add a validation would be nice,but the callers are many and come from different components, i dont think there is a good common place to add a validation you need to add it on each use case component, not feasible here, too much changes to do and prone to easy oversights (and i dont want to do much big PR since need to be backported)

if you thing to force change the alignment for validation purpose in to CGUIFontTTF::DrawTextInternal will make a bit of mess with parent callers behaviours that use same align value to do other things,
maybe a kind of "middle ground" is in the CGUIFont and you need to add the validation before each CGUIFontTTF::DrawTextInternal call and alignment value uses
for now are both cases: CGUIFont::DrawText and CGUIFont::DrawScrollingText
I can try adding align validation on these two methods, hoping it won't be printed in the log millions of error lines when the validation fails

i made an attempt and as i was thinking it print on log millions lines of validating errors

so i will add a kid of validation check in to CGUIFontTTF::DrawTextInternal inside the if (dirtyCache),
so that when cached dont print anymore tons of log lines
maybe not perfect but at least users/devs can know if has set an invalid alignment that can produce wrong rendering

added validation as said on my previous comment, please try give a look

CrystalP · 2024-03-10T19:43:31Z

xbmc/guilib/GUIFontTTF.cpp

@@ -516,10 +538,10 @@ void CGUIFontTTF::DrawTextInternal(CGraphicContext& context,
      {
        float nextCursorX = cursorX;

-        if (alignment & XBFONT_TRUNCATED)
+        if (alignment & XBFONT_TRUNCATED || alignment & XBFONT_TRUNCATED_LEFT)


Is this addition necessary? cursorX is initialized with ellipsesWidth in case of left truncation and the iterator of the loop begins at the first character that fits with left truncation

Alternatively mirror the Render loop more closely, which does require this condition and an exception treatment for the first char.

CrystalP · 2024-03-10T19:47:11Z

xbmc/guilib/GUIFontTTF.cpp

@@ -554,13 +576,13 @@ void CGUIFontTTF::DrawTextInternal(CGraphicContext& context,
        continue;
      }

-      if (alignment & XBFONT_TRUNCATED)
+      if (alignment & XBFONT_TRUNCATED || alignment & XBFONT_TRUNCATED_LEFT)
      {


since the XBFONT_TRUNCATED and the XBFONT_TRUNCATED_LEFT case don't share anything, might be clearer to do

if (alignment & XBFONT_TRUNCATED) { existing code } else if (alignment & XBFONT_TRUNCATED_LEFT && itGlyph == glypghBegin) { new code }

CrystalP · 2024-03-10T20:43:14Z

xbmc/guilib/GUIEditControl.cpp

@@ -538,7 +537,7 @@ void CGUIEditControl::ProcessText(unsigned int currentTime)
    if (HasFocus() || leftTextWidth == 0)
      changed |= m_label2.SetOverflow(CGUILabel::OVER_FLOW_CLIP);
    else
-      changed |= m_label2.SetOverflow(CGUILabel::OVER_FLOW_TRUNCATE);
+      changed |= m_label2.SetOverflow(CGUILabel::OVER_FLOW_TRUNCATE_LEFT);


Doesn't left or right truncation depend on the offset of the first character displayed and text length vs space available?
Worst case there could be both left and right truncation. probably best left for another PR though, this one does not mean to change the behavior.

left or right truncation currently depends on:

How you want to display a text on a control

If the text is LTR / RTL (this can be a future improvement)

if case of edit control we want always display the last chars on screen and so truncate on left,
otherwise it will result in incomprehensible text displayed if you use truncate from right

maybe here i can try re-use SetAlign instead to add the new OVER_FLOW_TRUNCATE_LEFT
in theory it should not cause problems, i will try

no not good, better as is otherwise break the overflow behaviour, so always set the truncate align should not be a big problem but idk

CrystalP · 2024-03-10T20:53:03Z

I don't have much skinning skills, my limited runtime testing didn't find issues in Estuary but a skinner's feedback would be preferable.
For completeness, in the before/after screenshots, could you add "Multilines both lines are too long, exceed the container width, and text "truncate to LEFT" is applied" with both left and center alignment.

CastagnaIT · 2024-03-12T12:34:57Z

could you add "Multilines both lines are too long, exceed the container width, and text "truncate to LEFT" is applied" with both left and center alignment.

before this PR you cannot use truncate from LEFT with left/center alignment, because truncate to LEFT was linked to right alignment only, so no screenshots

Hitcher

Found some time to test this today and everything is working as expected.

Thanks.

CastagnaIT · 2024-03-14T11:54:37Z

jenkins build this please

CastagnaIT added Type: Fix non-breaking change which fixes an issue WIP PR that is still being worked on Component: GUI rendering labels Mar 3, 2024

CastagnaIT force-pushed the truncate_text_edit branch 4 times, most recently from e29ee53 to 125e3b1 Compare March 4, 2024 13:11

CastagnaIT marked this pull request as ready for review March 4, 2024 13:18

CastagnaIT removed the WIP PR that is still being worked on label Mar 4, 2024

CastagnaIT force-pushed the truncate_text_edit branch from 125e3b1 to 78004ff Compare March 4, 2024 13:29

CastagnaIT requested a review from enen92 March 6, 2024 13:10

CastagnaIT added the v21 Omega label Mar 7, 2024

CastagnaIT added this to the Omega 21.0 milestone Mar 7, 2024

CastagnaIT requested a review from CrystalP March 7, 2024 16:49

CrystalP reviewed Mar 9, 2024

View reviewed changes

xbmc/interfaces/legacy/Control.h Outdated Show resolved Hide resolved

CastagnaIT force-pushed the truncate_text_edit branch from 78004ff to 1bdf694 Compare March 10, 2024 12:30

CastagnaIT added Backport: Needed v22 "P" and removed v21 Omega labels Mar 10, 2024

CastagnaIT modified the milestones: Omega 21.0, "P" 22.0 Alpha 1 Mar 10, 2024

CrystalP reviewed Mar 10, 2024

View reviewed changes

xbmc/guilib/GUIFontTTF.cpp Outdated Show resolved Hide resolved

CrystalP reviewed Mar 10, 2024

View reviewed changes

CastagnaIT force-pushed the truncate_text_edit branch 2 times, most recently from ce9b30a to 0d100af Compare March 12, 2024 12:26

CastagnaIT force-pushed the truncate_text_edit branch from 0d100af to 4db197f Compare March 12, 2024 12:32

CastagnaIT added 2 commits March 12, 2024 16:02

[guilib] Decouple left/right text truncate from alignment

798068a

[guilib][GUILabel] Fix align Center x with overflow mode

17af1e2

CastagnaIT force-pushed the truncate_text_edit branch from 4db197f to 17af1e2 Compare March 12, 2024 15:02

Hitcher self-requested a review March 14, 2024 10:32

Hitcher approved these changes Mar 14, 2024

View reviewed changes

CastagnaIT mentioned this pull request Mar 15, 2024

[backport][guilib] Decouple left/right text truncate from alignment #24851

Merged

14 tasks

CastagnaIT added Backport: Done and removed Backport: Needed labels Mar 15, 2024

CastagnaIT merged commit 9901571 into xbmc:master Mar 15, 2024
2 checks passed

CastagnaIT deleted the truncate_text_edit branch March 15, 2024 08:35

CrystalP mentioned this pull request May 7, 2024

Splash screen text not centered #25155

Closed

7 tasks

CastagnaIT mentioned this pull request May 8, 2024

[GUIFontTTF] Fix regressions on centered GUI text #25163

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[guilib] Decouple left/right text truncate from alignment #24799

[guilib] Decouple left/right text truncate from alignment #24799

CastagnaIT commented Mar 3, 2024 •

edited

CrystalP Mar 10, 2024

CastagnaIT Mar 12, 2024 •

edited

CastagnaIT Mar 12, 2024

CrystalP Mar 10, 2024

CastagnaIT Mar 12, 2024 •

edited

CastagnaIT Mar 12, 2024

CastagnaIT Mar 12, 2024

CrystalP Mar 10, 2024

CastagnaIT Mar 12, 2024

CrystalP Mar 10, 2024 •

edited

CastagnaIT Mar 12, 2024

CrystalP Mar 10, 2024 •

edited

CastagnaIT Mar 12, 2024

CastagnaIT Mar 12, 2024 •

edited

CrystalP commented Mar 10, 2024

CastagnaIT commented Mar 12, 2024

Hitcher left a comment

CastagnaIT commented Mar 14, 2024

[guilib] Decouple left/right text truncate from alignment #24799

[guilib] Decouple left/right text truncate from alignment #24799

Conversation

CastagnaIT commented Mar 3, 2024 • edited

Description

Motivation and context

How has this been tested?

What is the effect on users?

Screenshots (if appropriate):

Types of change

Checklist:

Choose a reason for hiding this comment

CastagnaIT Mar 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CastagnaIT Mar 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrystalP Mar 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CrystalP Mar 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CastagnaIT Mar 12, 2024 • edited

Choose a reason for hiding this comment

CrystalP commented Mar 10, 2024

CastagnaIT commented Mar 12, 2024

Hitcher left a comment

Choose a reason for hiding this comment

CastagnaIT commented Mar 14, 2024

CastagnaIT commented Mar 3, 2024 •

edited

CastagnaIT Mar 12, 2024 •

edited

CastagnaIT Mar 12, 2024 •

edited

CrystalP Mar 10, 2024 •

edited

CrystalP Mar 10, 2024 •

edited

CastagnaIT Mar 12, 2024 •

edited