feat: add the `lineBreak` option #86

toku-sa-n · 2022-02-18T06:23:13Z

This commit adds the lineBreak option to specify how to break texts into lines. OnSpaces is the default and breaks texts at spaces. OnCharacters breaks texts at characters, even in the middle of a word. It is useful for handling CJK texts, though it does not follow the line breaking rules. It is equivalent to anywhere of CSS' line-break property.

Below is the example output and its source code.

{-# LANGUAGE OverloadedStrings #-}

module Main
  ( main
  ) where

import           Data.Text (replicate)
import           Monomer   (AppEventResponse, CmbBgColor (bgColor),
                            CmbMultiline (multiline),
                            CmbStyleBasic (styleBasic), LineBreak (..),
                            WidgetEnv, WidgetNode, appFontDef, appTheme, black,
                            blue, darkTheme, green, hgrid, label_, red,
                            startApp, textLineBreak)
import           Prelude   hiding (replicate)

build :: WidgetEnv () () -> () -> WidgetNode () ()
build _ _ =
  hgrid
    [ longTextLabel OnSpaces red "This is an English text."
    , longTextLabel OnSpaces blue "これは日本語の文章です．"
    , longTextLabel OnCharacters green "This is an English text."
    , longTextLabel OnCharacters black "これは日本語の文章です．"
    ]
  where
    longTextLabel how color c =
      label_ (replicate 200 c) [multiline] `styleBasic`
      [textLineBreak how, bgColor color]

handle ::
     WidgetEnv () () -> WidgetNode () () -> () -> () -> [AppEventResponse () ()]
handle _ _ _ _ = []

main :: IO ()
main =
  startApp
    ()
    handle
    build
    [appFontDef "Regular" "NotoSansJP-Light.otf", appTheme darkTheme]

Discussed in #84

This commit adds `lineBreak` option to specify how to break texts into lines. `OnSpaces` is the default and breaks texts at spaces. `OnCharacters` breaks texts at characters, even in the middle of a word. This is useful for handling CJK texts, though it does not follow [the line breaking rules](https://en.wikipedia.org/wiki/Line_breaking_rules_in_East_Asian_languages).

toku-sa-n · 2022-02-18T06:26:03Z

src/Monomer/Graphics/Text.hs

+splitGroups :: LineBreak -> Seq GlyphPos -> Seq GlyphGroup
+splitGroups _ Empty = Empty
+splitGroups break glyphs = group <| splitGroups break rest where
  g :<| gs = glyphs
  groupWordFn = not . isWordDelimiter . _glpGlyph
+  atAnyPos = break == OnCharacters
  (group, rest)
-    | isWordDelimiter (_glpGlyph g) = (Seq.singleton g, gs)
+    | atAnyPos || isWordDelimiter (_glpGlyph g) = (Seq.singleton g, gs)


fitGroups will take times if a long text and the OnCharacters option (like the example code I pasted) are given because this function splits the text into characters. I don't think this affects the most use cases, though. Should I make this fast?

Yes, please!

This is something I was supposed to optimize at some point but, since it worked well enough for my uses cases, I never went back to review it.

Sorry I couldn't come up with a good idea.

I tried a couple of things, and this looks a bit faster (I have not benchmarked it with Criterion or similar):

fitLineToW fontMgr font fSize fSpcH fSpcV metrics break top width trim text = res where spaces = T.replicate 4 " " newText = T.replace "\t" spaces text !glyphs = computeGlyphsPos fontMgr font fSize fSpcH newText -- Do not break line on trailing spaces, they are removed in the next step -- In the case of KeepSpaces, lines with only spaces (empty looking) are valid keepTailSpaces = trim == TrimSpaces groups | break == OnCharacters = splitGroups break width glyphs | otherwise = fitGroups (splitGroups break width glyphs) width keepTailSpaces resetGroups | trim == TrimSpaces = fmap (resetGlyphs . trimGlyphs) groups | otherwise = fmap resetGlyphs groups buildLine = buildTextLine font fSize fSpcH fSpcV metrics top res | text /= "" = Seq.mapWithIndex buildLine resetGroups | otherwise = Seq.singleton (buildLine 0 Empty) ... splitGroups :: LineBreak -> Double -> Seq GlyphPos -> Seq GlyphGroup splitGroups _ _ Empty = Empty splitGroups break width glyphs = group <| splitGroups break width rest where g :<| gs = glyphs groupWordFn = not . isWordDelimiter . _glpGlyph groupWidthFn g2 = _glpXMax g2 - _glpXMin g <= width atWord = break == OnSpaces atAnyPos = break == OnCharacters (group, rest) | atWord && isWordDelimiter (_glpGlyph g) = (Seq.singleton g, gs) | atWord = Seq.spanl groupWordFn glyphs | otherwise = Seq.spanl groupWidthFn glyphs

I checked your code, and I feel it's faster, although I haven't run any benchmarks (and TBH, I don't know how to run benchmarks in Haskell).

fjvallarino · 2022-02-18T08:05:28Z

src/Monomer/Graphics/Text.hs

@@ -53,25 +53,26 @@ calcTextSize
  -> Text          -- ^ The text to calculate.
  -> Size          -- ^ The calculated size.
 calcTextSize fontMgr style !text = size where
-  size = calcTextSize_ fontMgr style SingleLine KeepSpaces Nothing Nothing text
+  size = calcTextSize_ fontMgr style SingleLine KeepSpaces OnSpaces Nothing Nothing text


I think in this case it would be worth trying to use the new attribute from StyleState.

There are a few utilities in StyleUtil.hs for accessing StyleState fields (styleTextAlignH, styleTextAlignV, etc). Adding one for styleTextLineBreak would simplify retrieving this value (or the default if not available).

fjvallarino · 2022-02-18T08:09:35Z

src/Monomer/Widgets/Containers/Tooltip.hs

@@ -197,7 +197,7 @@ makeTooltip caption config state = widget where
      targetW = fromMaybe maxW (_ttcMaxWidth config)
      targetH = fromMaybe maxH (_ttcMaxHeight config)
      targetSize = Size targetW targetH
-      fittedLines = fitTextToSize fontMgr style Ellipsis MultiLine TrimSpaces
+      fittedLines = fitTextToSize fontMgr style Ellipsis MultiLine TrimSpaces OnSpaces


In this case, it would be nice to use styleTextLineBreak (the new utility mentioned earlier).

fjvallarino · 2022-02-18T08:12:22Z

src/Monomer/Widgets/Singles/Base/InputField.hs

@@ -919,7 +919,7 @@ updatePlaceholder wenv node !state !config = newState where
    & L.text . non def . L.alignH ?~ inputFieldAlignH style
    & L.text . non def . L.alignV ?~ inputFieldAlignV style
  text = _ifcPlaceholder config
-  fitText = fitTextToSize fontMgr pstyle Ellipsis MultiLine KeepSpaces Nothing
+  fitText = fitTextToSize fontMgr pstyle Ellipsis MultiLine KeepSpaces OnSpaces Nothing


Just to be clear, I think it's fine if we ignore the new text attribute here since it's a single line field

fjvallarino · 2022-02-18T08:13:28Z

src/Monomer/Widgets/Singles/TextArea.hs

@@ -762,7 +762,7 @@ stateFromText wenv node state text = newState where
  style = currentStyle wenv node
  fontMgr = wenv ^. L.fontManager
  newTextMetrics = getTextMetrics wenv style
-  tmpTextLines = fitTextToWidth fontMgr style maxNumericValue KeepSpaces text
+  tmpTextLines = fitTextToWidth fontMgr style maxNumericValue KeepSpaces OnSpaces text


It's also fine if we ignore the new text attribute here since it never forces a break unless a \n is provided (it passes the largest number available as the line break width)

fjvallarino · 2022-02-18T08:15:47Z

src/Monomer/Widgets/Util/Text.hs

@@ -40,21 +40,22 @@ getTextMetrics wenv style = textMetrics where
 getTextSize :: WidgetEnv s e -> StyleState -> Text -> Size
 getTextSize wenv style text = size where
  fontMgr = wenv ^. L.fontManager
-  size = calcTextSize_ fontMgr style SingleLine KeepSpaces Nothing Nothing text
+  size = calcTextSize_ fontMgr style SingleLine KeepSpaces OnSpaces Nothing Nothing text


This one would also benefit from using styleTextLineBreak.

fjvallarino · 2022-02-18T08:22:11Z

Looks good!

I forgot to mention, but there is a ./watch-tests.sh file you can use for dynamically running unit tests; you can find the unit tests for text-related utility functions in Monomer.Widgets.Util.TextSpec. If you can add a few tests for the new line-breaking style, it would be great! Using the script will also allow you to check if the performance changes return the same results as the current version.

toku-sa-n · 2022-02-19T03:38:04Z

Now I understand what you said in the issue. I should've asked for the detail, sorry.

You said:

One option can be adding a new attribute to TextStyle that indicates how text should break on multiline. The benefit is that the functionality would be automatically available to all widgets using text (buttons, labeled items, etc.).

So are we going to add the lineBreak option to use it with styleBasic, not with configs of each widget like labelCfg?

Not to create instance of each widget.

toku-sa-n · 2022-02-19T04:17:33Z

I updated the code example to specify line break options with styleBasic.

fjvallarino · 2022-02-20T10:12:13Z

So are we going to add the lineBreak option to use it with styleBasic, not with configs of each widget like labelCfg?

Yes, I think it's easier to use that way. A single place needs updating, and every widget that relies on fitTextToSize and related get the functionality for free.

Let me know when you want me to review the PR again! I noticed you marked it as a draft, so I don't want to bug you until you are ready (looks great so far, by the way).

fjvallarino · 2022-02-21T14:26:03Z

Looks good! I added a comment regarding performance.

One thing about the example: styleBasic (or any of the similar functions) overwrites the node's style (it does not merge with the previous one). This means that when you set bgColor, it overwrites the node's previous style, so the lineBreak property is lost. While testing, I modified longTextLabel to receive the color and take care of setting both color and lineBreak in a single statement.

toku-sa-n · 2022-02-23T01:22:15Z

One thing about the example: styleBasic (or any of the similar functions) overwrites the node's style (it does not merge with the previous one). This means that when you set bgColor, it overwrites the node's previous style, so the lineBreak property is lost.

Yes, you're right. Thank you. I fixed the example code and the output.

fjvallarino

@toku-sa-n looks good! 👍

Thanks!

toku-sa-n commented Feb 18, 2022

View reviewed changes

fjvallarino reviewed Feb 18, 2022

View reviewed changes

fix: implement for TextStyle

dae3504

Not to create instance of each widget.

toku-sa-n marked this pull request as draft February 19, 2022 08:54

test: add for OnCharacters

4ba574d

toku-sa-n marked this pull request as ready for review February 20, 2022 12:38

toku-sa-n requested a review from fjvallarino February 20, 2022 12:38

perf: make fitLineToW faster

71f0d5a

fjvallarino approved these changes Feb 24, 2022

View reviewed changes

fjvallarino merged commit 7c6777d into fjvallarino:main Feb 24, 2022

toku-sa-n deleted the line_break branch February 24, 2022 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add the `lineBreak` option #86

feat: add the `lineBreak` option #86

toku-sa-n commented Feb 18, 2022 •

edited

toku-sa-n Feb 18, 2022 •

edited

fjvallarino Feb 18, 2022

toku-sa-n Feb 20, 2022

fjvallarino Feb 21, 2022

toku-sa-n Feb 23, 2022

fjvallarino Feb 18, 2022

fjvallarino Feb 18, 2022

fjvallarino Feb 18, 2022

fjvallarino Feb 18, 2022

fjvallarino Feb 18, 2022

fjvallarino commented Feb 18, 2022

toku-sa-n commented Feb 19, 2022

toku-sa-n commented Feb 19, 2022

fjvallarino commented Feb 20, 2022

fjvallarino commented Feb 21, 2022

toku-sa-n commented Feb 23, 2022

fjvallarino left a comment

feat: add the lineBreak option #86

feat: add the lineBreak option #86

Conversation

toku-sa-n commented Feb 18, 2022 • edited

toku-sa-n Feb 18, 2022 • 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

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

fjvallarino commented Feb 18, 2022

toku-sa-n commented Feb 19, 2022

toku-sa-n commented Feb 19, 2022

fjvallarino commented Feb 20, 2022

fjvallarino commented Feb 21, 2022

toku-sa-n commented Feb 23, 2022

fjvallarino left a comment

Choose a reason for hiding this comment

feat: add the `lineBreak` option #86

feat: add the `lineBreak` option #86

toku-sa-n commented Feb 18, 2022 •

edited

toku-sa-n Feb 18, 2022 •

edited