Add TextGridTextStyle #4244

mgazza · 2023-09-13T16:42:03Z

Description:

Enable the text style to be passed in as part of the TextGridStyle. this will allow font options to be passed down

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

Where applicable:

Public APIs match existing style and have Since: line.
Any breaking changes have a deprecation path or have been discussed.
Check for binary size increases when importing new modules.

enable the text style to be passed in as part of the TextGridStyle. this will allow font options to be passed down

coveralls · 2023-09-13T21:38:49Z

coverage: 65.292% (+0.003%) from 65.289% when pulling cf930d3 on codelaboratoryltd:feature/text-grid-highlighting into 307f928 on fyne-io:develop.

andydotxyz

Looks good. A few comments inline about since notes and naming.

Would it be possible to add a test?

Also I think you might find that although the APIs are added they do not take effect on output...
You'll need to extend the code in internal/painter/font.go I think (line 35 onwards). It will likely be necessary to pass the style in to the actual theme Font call instead of using the helper methods which are limited to monospace vs all other styles...

widget/textgrid.go

andydotxyz · 2023-09-13T21:40:09Z

widget/textgrid.go

 // CustomTextGridStyle is a utility type for those not wanting to define their own style types.
 type CustomTextGridStyle struct {
 FGColor, BGColor color.Color
+ Text *fyne.TextStyle


Wouldn't Style or TextStyle be more appropriate than Text for a style field?
Perhaps it should match naming elsewhere like on the canvas.Text or widget.Label?

p.s. It's missing the // Since: 2.5 comment

I've replaced this with two bool values

Taking this path means that adding another value later will need another interface... Using TextStyle (which is a concrete struct) would make it possible to add more I think

Text Style would need the underline property adding.

Indeed. But do you think underline is only useful for TextGrid? Or would we perhaps consider having it broadly available under a simple style value?

No, it could/would make sense. Just making you aware of the implications. I'm finding it very dificult to determine the scope of changes to fyne and associated projects as in some cases you suggest less invasive and some other cases more invasive. The implications of adding this to TextStyle would mean it would remain unimplemented by anything that uses it and other options like italics which will not be implemented within the TextGrid/Renderer.

widget/textgrid.go

Rework the bold feature to better support underline

mgazza · 2023-09-14T14:16:52Z

quick demo

widget/textgrid.go

andydotxyz · 2023-09-14T17:20:41Z

widget/textgrid.go

@@ -345,12 +369,15 @@ func (t *textGridRenderer) appendTextCell(str rune) {
 text.TextStyle.Monospace = true

 bg := canvas.NewRectangle(color.Transparent)
- t.objects = append(t.objects, bg, text)
+
+ ul := canvas.NewLine(color.Transparent)


Won't having 50% more objects always visible slow things down?
If adding them cannot be avoided at least avoid showing them... or maybe we need an optimisation in line drawing to ignore transparent lines? (I don't think that exists)...

It doesn't appear any slower to me. I do wonder if we should have some benchmarks around this, however.

even just running top should push the terminal and display CPU usage at the same time

andydotxyz · 2023-09-14T17:22:16Z

widget/textgrid.go

+type TextGridTextStyle interface {
+ TextGridStyle
+ Bold() bool
+ Underlined() bool


Isn't it Underline not Underlined in most places?

Underline only appears in the processing logic to refer to the actual line.

Sorry, I was meaning in most typographic APIs is it not referred to as underline?

I.e. it is not "bolded" or "italicised" either...

andydotxyz · 2023-09-14T17:26:21Z

You'll need to extend the code in internal/painter/font.go I think (line 35 onwards). It will likely be necessary to pass the style in to the actual theme Font call instead of using the helper methods which are limited to monospace vs all other styles...

I'm curious about how you got the demo working with that font cache not patched - did you find that it wasn't a problem for some reason? How was the bold monospace font looked up in that demo?

…t underline

mgazza · 2023-09-18T22:04:52Z

You'll need to extend the code in internal/painter/font.go I think (line 35 onwards). It will likely be necessary to pass the style in to the actual theme Font call instead of using the helper methods which are limited to monospace vs all other styles...

I'm curious about how you got the demo working with that font cache not patched - did you find that it wasn't a problem for some reason? How was the bold monospace font looked up in that demo?

overriding the theme was all that was needed

andydotxyz · 2023-09-18T22:13:08Z

Yup I appreciate that the API was designed to make this possible - but I'm pretty sure you'll get cache collisions inside the toolkit as it does not understand bold monospace.

Happy to be proved wrong on this of course, but it's a concern.

mgazza · 2023-09-19T09:42:07Z

Yup I appreciate that the API was designed to make this possible - but I'm pretty sure you'll get cache collisions inside the toolkit as it does not understand bold monospace.

Happy to be proved wrong on this of course, but it's a concern.

I haven't seen any, it sounds like an ideal unit test candidate, however. Is there a table test which I can add a test case to?

andydotxyz · 2023-09-19T10:14:22Z

I haven't seen any, it sounds like an ideal unit test candidate, however. Is there a table test which I can add a test case to?

The problem will be in CachedFontFace of internal/painter/font.go and there is a font_internal_test.go used to test caching and other non-public API.

mgazza · 2023-09-27T21:16:49Z

When running the tests locally, I get quite a few failures. I have checked to ensure no more/other tests are failing due to this work. I can't see how CachedFontFace is affected, it appears to call the theme font in every case from what I can see. I may be missing something however.

andydotxyz · 2023-09-29T10:19:51Z

When running the tests locally, I get quite a few failures. I have checked to ensure no more/other tests are failing due to this work. I can't see how CachedFontFace is affected, it appears to call the theme font in every case from what I can see. I may be missing something however.

I can't figure out how! The OpenGL driver text rendering uses the cached face to draw. it is created in internall/painter/gl/texture.go:155, and the text style passed in uses only the Monospace field in the case of mono, ignoring the others. I cannot find any other routes to get to DrawString which is the entry point for rendering text...

andydotxyz · 2023-09-29T10:25:14Z

I just realised that TextGridTextStyle does not handle Italics... so would that be another new interface in the future? It's looking more and more like a duplication of the TextStyle struct!

andydotxyz · 2023-09-29T10:26:39Z

When running the tests locally, I get quite a few failures.

There should not be - there is an intermittent issue due to window sizing and OS not always allowing it during the test IIRC. If the CI does not see failures then it's probably OK but worth opening a bug to report that it's not all working at your end.

andydotxyz · 2024-05-15T15:28:52Z

When running the tests locally, I get quite a few failures.

Latest develop should have all the test failures resolved.
Shall we try and get something based on this in to 2.5? Would be good to have the TextStyle used on more widgets.

mgazza force-pushed the feature/text-grid-highlighting branch from ad4bc9f to 9d2680a Compare September 13, 2023 16:48

Add TextGridTextStyle

cf930d3

enable the text style to be passed in as part of the TextGridStyle. this will allow font options to be passed down

mgazza force-pushed the feature/text-grid-highlighting branch from 9d2680a to cf930d3 Compare September 13, 2023 17:06

andydotxyz requested changes Sep 13, 2023

View reviewed changes

Add underline support

39093a2

Rework the bold feature to better support underline

andydotxyz requested changes Sep 14, 2023

View reviewed changes

fixup! Add underline support Rework the bold feature to better suppor…

030b2a5

…t underline

Fix textgrid tests

173196e

mgazza mentioned this pull request Oct 9, 2023

Enable strikethrough text #4297

Closed

2 tasks

GaetanBaert mentioned this pull request Oct 16, 2023

Add Underline to TextStyle options #4321

Open

2 tasks

mgazza mentioned this pull request Oct 18, 2023

Add bold and underline functionality fyne-io/terminal#46

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TextGridTextStyle #4244

Add TextGridTextStyle #4244

mgazza commented Sep 13, 2023

coveralls commented Sep 13, 2023

andydotxyz left a comment

andydotxyz Sep 13, 2023

mgazza Sep 14, 2023

andydotxyz Sep 14, 2023

mgazza Sep 18, 2023

andydotxyz Sep 18, 2023

mgazza Sep 19, 2023

mgazza commented Sep 14, 2023

andydotxyz Sep 14, 2023

mgazza Sep 18, 2023

andydotxyz Sep 19, 2023

andydotxyz Sep 14, 2023

mgazza Sep 18, 2023

andydotxyz Sep 19, 2023

andydotxyz commented Sep 14, 2023

mgazza commented Sep 18, 2023

andydotxyz commented Sep 18, 2023

mgazza commented Sep 19, 2023

andydotxyz commented Sep 19, 2023

mgazza commented Sep 27, 2023

andydotxyz commented Sep 29, 2023

andydotxyz commented Sep 29, 2023

andydotxyz commented Sep 29, 2023

andydotxyz commented May 15, 2024

Add TextGridTextStyle #4244

Are you sure you want to change the base?

Add TextGridTextStyle #4244

Conversation

mgazza commented Sep 13, 2023

Description:

Checklist:

Where applicable:

coveralls commented Sep 13, 2023

andydotxyz left a 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

mgazza commented Sep 14, 2023

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

andydotxyz commented Sep 14, 2023

mgazza commented Sep 18, 2023

andydotxyz commented Sep 18, 2023

mgazza commented Sep 19, 2023

andydotxyz commented Sep 19, 2023

mgazza commented Sep 27, 2023

andydotxyz commented Sep 29, 2023

andydotxyz commented Sep 29, 2023

andydotxyz commented Sep 29, 2023

andydotxyz commented May 15, 2024