Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RichTextSegment SizeName is not SizeNameText by default #5307

Open
2 tasks done
hkparker opened this issue Dec 10, 2024 · 9 comments
Open
2 tasks done

RichTextSegment SizeName is not SizeNameText by default #5307

hkparker opened this issue Dec 10, 2024 · 9 comments
Labels
unverified A bug that has been reported but not verified

Comments

@hkparker
Copy link
Contributor

Checklist

  • I have searched the issue tracker for open issues that relate to the same problem, before opening a new one.
  • This issue only relates to a single bug. I will open new issues for any other problems.

Describe the bug

When one creates a new widget.RichText, the value of widget.RichTextStyle.SizeName for the segments is not theme.SizeNameText by default, it's empty. I think it should default to theme.SizeNameText. At least, I would expect fyne.MeasureText to work out of the box without setting the size name, and it doesn't.

How to reproduce

package main

import (
	"fmt"

	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/theme"
	"fyne.io/fyne/v2/widget"
)

func main() {
	demo := app.New()
	window := demo.NewWindow("Demo")
	window.SetMaster()

	// Create a RichText
	rt := widget.NewRichText(&widget.TextSegment{
		Text: "demo time",
	})
	fmt.Println("Default SizeName: " + rt.Segments[0].(*widget.TextSegment).Style.SizeName)

	// Measure is inaccurate with empty size name
	width := fyne.MeasureText(
		rt.Segments[0].(*widget.TextSegment).Text,
		theme.Size(rt.Segments[0].(*widget.TextSegment).Style.SizeName),
		rt.Segments[0].(*widget.TextSegment).Style.TextStyle,
	) // this is wrong
	fmt.Printf("the wrong width: %f\n", width)

	width = fyne.MeasureText(
		rt.Segments[0].(*widget.TextSegment).Text,
		theme.TextSize(),
		rt.Segments[0].(*widget.TextSegment).Style.TextStyle,
	) // this is right
	fmt.Printf("the right width: %f\n", width)

	// Set the style
	rt.Segments[0].(*widget.TextSegment).Style = widget.RichTextStyle{
		SizeName: theme.SizeNameText,
	}

	fmt.Println("New SizeName: " + rt.Segments[0].(*widget.TextSegment).Style.SizeName)
	width = fyne.MeasureText(
		rt.Segments[0].(*widget.TextSegment).Text,
		theme.Size(rt.Segments[0].(*widget.TextSegment).Style.SizeName),
		rt.Segments[0].(*widget.TextSegment).Style.TextStyle,
	) // this is right
	fmt.Printf("the new right width: %f\n", width)

	window.Resize(fyne.Size{Height: 400, Width: 400})
	window.Show()
	demo.Run()
}

Produces:

Default SizeName: 
the wrong width: {0.000000 0.000000}
the right width: {71.343750 19.078125}
New SizeName: text
the new right width: {71.343750 19.078125}

Screenshots

No response

Example code

see "how to reproduce"

Fyne version

2.5.0

Go compiler version

go1.23.2 linux/amd64

Operating system and version

Arch Linux

Additional Information

No response

@hkparker hkparker added the unverified A bug that has been reported but not verified label Dec 10, 2024
@coder-in-go
Copy link

coder-in-go commented Dec 11, 2024

What should be expected output?
@hkparker
Is it as below?

Default SizeName: text
the wrong width: {71.343750 19.078125}
the right width: {71.343750 19.078125}
New SizeName: text
the new right width: {71.343750 19.078125}

@andydotxyz
Copy link
Member

I'm not sure what we can do, your code is creating a new TextSegment with a blank SizeNameText...
At what point would our code be able to intercept this and set a sensible value?

@hkparker
Copy link
Contributor Author

That is a good point... I guess I was thinking that in NewRichText there'd be something to the effect of "for each segment, if the size name is empty, set it to text". But, more generally, the thing I'm bringing up here is that fyne.MeasureText didn't work as expected without setting it, so maybe there's another place to address that. Or maybe I have the wrong expectations here and everything is actually fine!

@dweymouth
Copy link
Contributor

I think it could make sense for the builtin theme to return the size corresponding to fyne.SizeNameText when th.Size(name) is called with the empty string. That would make your code work as expected.

@andydotxyz
Copy link
Member

I think it could make sense for the builtin theme to return the size corresponding to fyne.SizeNameText when th.Size(name) is called with the empty string.

That's assuming that an empty size name is always referring to text, which seems dangerous.

That would make your code work as expected.

Yes it would, but at a significant possible impact to other areas. Thinking about it would it not make more sense to put the workaround in the measure text, where it defaults to something sane if it's 0.0?

@hkparker
Copy link
Contributor Author

If there's never a reason to measure text with a size of 0 this mostly makes sense to me, but maybe that's useful to someone somewhere? If none of these feel like good solutions I'd be inclined to suggest that there's maybe nothing to change here and people doing this will figure it out like I did. Like part of me thinks that measure text shouldn't "guess" what you mean and should just return 0 if you pass it 0, do what it says it's doing. I opened this because it was a rare "that didn't just work" moment in Fyne for me, but upon inspection I could see no change being warranted here.

@andydotxyz
Copy link
Member

If there's never a reason to measure text with a size of 0 this mostly makes sense to me

That's an interesting point - is there ever a reason to measure 0 size text? It feels quite meaningless to me, and a NewText will be created using the theme default size so it is an exception to the "canvas primitives don't use theme" rule... Perhaps this is a needed extension of the Text handling special case?

@dweymouth
Copy link
Contributor

Like part of me thinks that measure text shouldn't "guess" what you mean and should just return 0 if you pass it 0, do what it says it's doing.

I kind of agree with this. Especially since this is the direction we have gone in other areas (e.g. rejecting the idea that passing a duration of 0 to fyne.NewAnimation could start an endless animation instead of a zero-length animation). Maybe if there is a code change to be made here, it's just a documentation one? I.e. clearly documenting on the RichTextSegment that if the SizeName value is left empty/default, it will use theme.SizeNameText

@hkparker
Copy link
Contributor Author

I agree with this, calling out that an empty SizeName defaults to theme.SizeNameText in the rest of Fyne, however that it will indeed be an empty string and therefore can't be passed around to refer to the size of the text elsewhere (like MeasureText).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
unverified A bug that has been reported but not verified
Projects
None yet
Development

No branches or pull requests

4 participants