-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add TextGridTextStyle #4244
base: develop
Are you sure you want to change the base?
Add TextGridTextStyle #4244
Conversation
ad4bc9f
to
9d2680a
Compare
enable the text style to be passed in as part of the TextGridStyle. this will allow font options to be passed down
9d2680a
to
cf930d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced this with two bool values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text Style would need the underline property adding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Rework the bold feature to better support underline
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't appear any slower to me. I do wonder if we should have some benchmarks around this, however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even just running top should push the terminal and display CPU usage at the same time
widget/textgrid.go
Outdated
type TextGridTextStyle interface { | ||
TextGridStyle | ||
Bold() bool | ||
Underlined() bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it Underline
not Underlined
in most places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Underline only appears in the processing logic to refer to the actual line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was meaning in most typographic APIs is it not referred to as underline?
I.e. it is not "bolded" or "italicised" either...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods act as queries, determining whether a specific style is currently applied. "IsUnderline" wouldn't be appropriate because it suggests an action to check for underlining, rather than reflecting the current state of the text.
if Bold() accurately reflects the presence of bold formatting, then "Underlined()" follows the same logic. It's not "IsBolded" because the text either is bolded or not.
While "underline" might be more common in typography, "Underlined()" aligns with the existing naming convention and emphasizes the query nature of these methods. This prioritizes readability and consistency within the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this is consistent, "Bold" and "Italic" used elsewhere refer to state. Their "is set" equivalent in getters would be "IsItalic" or "Italicised".
Following this "Underline" is the state version and underlined refers to an action that occurred.
As said in other places though, I'm still not sure why it isn't a better plan to use existing "TextStyle" API rather than a specific style API for this widget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said in other places though, I'm still not sure why it isn't a better plan to use existing "TextStyle" API rather than a specific style API for this widget.
This would make sense, having different versions of effectively the same thing isn't great, assuming there's no gotchas lurking under the code for why this wouldn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To weigh in a bit more, I think the confusion is coming from the fact this is a method on a struct and not an enum like more traditional API's.
Since this is a method it feels more subject to OO programming naming, like we have suggested. If this was an enum then I suspect this whole thing would go away.
some other GUI API's for reference.
https://learn.microsoft.com/en-us/windows/win32/api/wingdi/nf-wingdi-createfonta?redirectedfrom=MSDN
https://docs.oracle.com/javase%2F8%2Fdocs%2Fapi%2F%2F/javax/swing/text/StyleConstants.CharacterConstants.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said in other places though, I'm still not sure why it isn't a better plan to use existing "TextStyle" API rather than a specific style API for this widget.
This would make sense, having different versions of effectively the same thing isn't great, assuming there's no gotchas lurking under the code for why this wouldn't work.
The only gotchas would be that I'm only implementing this for this single widget, all other widgets can be updated subsequently by the parties wanting that functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would make sense, having different versions of effectively the same thing isn't great, assuming there's no gotchas lurking under the code for why this wouldn't work.
The only gotchas would be that I'm only implementing this for this single widget, all other widgets can be updated subsequently by the parties wanting that functionality.
I think it's reasonable to use the existing TextStyle then, add the new features, support them in the widget you need it for and anyone who wants to add support to the other widgets can if/when it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, difference between field and method yes this is causing confusion.
The only gotchas would be that I'm only implementing this for this single widget, all other widgets can be updated subsequently by the parties wanting that functionality.
We should not add temporary APIs or items that are widget specific but will be broadened. Any public API will remain part of the toolkit indefinitely. Given there are feature requests open for underline it seems pretty reasonable to think it will be added in the meantime we could add some documentation about it not being supported in all locations until that is no longer true.
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? |
|
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? |
The problem will be in |
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 |
I can't figure out how! The OpenGL driver text rendering uses the cached face to draw. it is created in |
I just realised that |
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. |
Latest develop should have all the test failures resolved. |
Sorry for the delay, yeah it would be good to have this into 2.5 |
That might be tough, yesterday was feature freeze. I remain concerned that adding "TextGridTextStyle" in addition to "TextGridStyle" and "TextStyle" is confusing and the discussion about bold/bolded underline/underlined would be avoided by using the existing APIs |
Change TextGridStyle to use TextStyle
Description:
Enable the text style to be passed in as part of the TextGridStyle. this will allow font options to be passed down
Checklist:
Where applicable: