Add bold and underline functionality #46
Conversation
andydotxyz
changed the title
|
(converted to draft on GitHub rather than in the text) |
mgazza
force-pushed
the
add-bold-underline
branch
2 times, most recently
from
fa0ae3b to
be74aa6
Compare
|
Trying to come back to a backlog of PRs and I see this one - it seems to have blinking work in it as well, maybe the wrong base branch? |
|
Hey, sorry I've been really busy with other things recently. Certainly there's mention of blinking in this PR. That is because it depends on #45 |
mgazza
force-pushed
the
add-bold-underline
branch
from
be74aa6 to
43baa67
Compare
depends on |
mgazza
force-pushed
the
add-bold-underline
branch
from
43baa67 to
b4a2dc9
Compare
|
I had a glance through this, I'm not sure about the weight of the renderer on this - it seems like a lot of TextGrid is copied in, but I'm not sure why. |
|
I can't encapsulate it because the renderer isn't exported. This a huge issue IMO. I don't know the best way to resolve it. |
|
We could move the blinking down to the text grid render but I'm not sure what value that would give |
But why do you need to tweak the renderer implementation? Can't it be delegated to? |
|
For blink I need to swap the fg and bg colours and I can either do that in
the renderer or via the style. Previously I did this via the style and
created an interupt which did the blink. ( in the history of the pr ). This
was the second approach which you suggested would be better. In terms of
delegate, what are you suggesting?
…On Mon, 17 Jun 2024, 19:43 Andy Williams, ***@***.***> wrote:
I can't encapsulate it because the renderer isn't exported. This a huge
issue IMO. I don't know the best way to resolve it.
But why do you need to tweak the renderer implementation? Can't it be
delegated to?
—
Reply to this email directly, view it on GitHub
<#46 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD3ZWQ5KTCCHTMDPNDNJQSTZH4U6TAVCNFSM6AAAAAA6FC422OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZUGE4DMNZUGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Oh I see, this pattern was adopted earlier - I kinda missed how much was duplicated sorry. What I had thought was happening before was that the style information was being used as the source, internally it was toggling when blinking and so the renderer had the updated model to get the visuals right. The TextGridStyle is an interface after all so it can return different colour values as it blinks. By delegating I meant that we encapsulate a TextGrid (along with its renderer) and provide a layer on top that manages complex things like toggling blink or handling hyperlink taps etc... the core rendering is still in the UI library... But that leaves us with this PR. Is it right to pull in all the new code from Fyne TextGrid here to get the copy of the code working as it is upstream? I guess we kindof have to with what is in place now? Unless a simplification refactor could be done first? |
|
Can we make the renderer public is the question? That or provide something
that we can hook into to say which color we should use. However adding such
methods kind of break the open close principle
…On Wed, 19 Jun 2024, 14:18 Andy Williams, ***@***.***> wrote:
For blink I need to swap the fg and bg colours and I can either do that in
the renderer or via the style.
Oh I see, this pattern was adopted earlier - I kinda missed how much was
duplicated sorry.
Something between the two was in my head. The first was new public APIs
which were not needed, and putting it all in the renderer is fine but we've
got much more code to manage now.
What I had thought was happening before was that the style information was
being used as the source, internally it was toggling when blinking and so
the renderer had the updated model to get the visuals right. The
TextGridStyle is an interface after all so it can return different colour
values as it blinks. By delegating I meant that we encapsulate a TextGrid
(along with its renderer) and provide a layer on top that manages complex
things like toggling blink or handling hyperlink taps etc... the core
rendering is still in the UI library...
But that leaves us with this PR. Is it right to pull in all the new code
from Fyne TextGrid here to get the copy of the code working as it is
upstream? I guess we kindof have to with what is in place now? Unless a
simplification refactor could be done first?
—
Reply to this email directly, view it on GitHub
<#46 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD3ZWQZDQYCYUGUB4SDHNTDZIGAKHAVCNFSM6AAAAAA6FC422OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZYG4YDCOJYGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
No, renderers cannot be made public. However is the |
|
Right but the blinking state would need to be accessible for us to return
the correct color.
…On Wed, 19 Jun 2024, 15:08 Andy Williams, ***@***.***> wrote:
No, renderers cannot be made public. However is the BgColor and FgColor
being methods not exactly the thing you need to be able to influence the
colours on any particular render pass?
—
Reply to this email directly, view it on GitHub
<#46 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD3ZWQ77NWDRWMTTPOGSX6TZIGGFNAVCNFSM6AAAAAA6FC422OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZYHAYTIMRRG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
In my mind that is where our "wrapping" widget comes in - we know the original state and we track a blink flag, toggling the state that is passed into the embedded TextGrid |
|
Anything more I can do to help with the tidy-up here? |
mgazza
force-pushed
the
add-bold-underline
branch
from
b4a2dc9 to
707591b
Compare
|
If it's not public, we can't employ consumption over inheritance. Could you maybe take a stab at what your thinking here? |
I don't understand this question sorry. My point about blinking was that the colours are being asked for from an interface - and we can return different colours for the fg/bg as the blink occurs. Internally we manage blink state but the renderer would remain entirely the standard textgrid as we are just manipulating the widget state - no need to hook into a renderer. |
|
Blinking is already in the main branch. This PR is about bold and underlining. I think I'm confused as these couple of features have been left for awhile. |
|
The discussion, as far as I'm aware, is what was done with blinking is now resulting in constantly pulling renderer from upstream Fyne into this project to maintain a tweak for blinking. I don't know if it was missed at the time but this is really undesirable, causes a maintenance overhead and goes against the design of widget and renderer separation. This PR should be trivial but because of earlier decisions it is not - this is what I was hoping we could resolve. If you are keen to land this feature we could back out the blinking, add this in easily, and then review the renderer situation with fresh eyes to get blinking back? |
|
it was discussed here. |
I don't understand this - the widget is already in Fyne. What exists here seems to be a copy/paste of the renderer which you're now working to keep in sync as the upstream widget changes. |
|
The widget for a text grid exists in fyne, but it doesn't respect The Open-closed principal, the term grid was added in #45. This PR add's bold and underline to the termgrid. The confusion may come from the diff as the termgrid renderer was moved to its own file in this commit |
mgazza
force-pushed
the
add-bold-underline
branch
from
24e9952 to
b1ea76f
Compare
|
I'm not confused and have tried to outline before that I am aware this is not new code but has come to light it's tight connection to an existing widget. Without getting into the semantics of it all widgets are open to extension so I don't think it's correct to say that it's not open-closed. I have illustrated above ways that I think it is in fact possible to do what was achieved through copy and paste last time. Do you think it is untrue to say that without the copying out of the renderer this PR would have been far simpler? I think the issue is on me for having agreed to cloning the renderer in the first place, I did not fully understand how tightly this caused the two to be tied - which is what has become clear in this PR. I hope this explains at least my previous messages, I don't think that I am confused about the scope of this change. But with the required render changes it looks like this requires more custom rendering which is not the case. |
|
I'm trying to get on top of this and seeing what I can do to help. With the updates to Fyne on develop (adding Scroll to TextGrid) there have been a lot of refactoring internally which makes the duplicated TextGridRenderer in here more problematic (TextGrid now has renderers for the widget, it's content (which may or may not be in a scroller) and one per row as well for re-use during scrolling). I'm guessing that the best thing for me to do is work on the |
|
@andydotxyz Any progress with this? It's been around a while and Bold/Underline seems like a pretty base thing for term text-grid to support. Without it, many programs and outputs just render incorrectly. EDIT: Just to add, I was working around the lack of support by using the duplicate of the renderer and using these changes there. However, since you de-duplicated it and it's all now under "internal" I can't override it or duplicate it or anything. |
It's not my PR to progress so I think I am the wrong person to answer. Bold should now be in TextGrid upstream - all you need to hook in is a monospaced bold font in your custom theme.
It sounds like your code was going to run into the same problem as this PR - duplicating the renderer of an upstream widget is a very bad idea. The features have been added upstream but anyone with custom renderer implementations won't know that and/or will have very complex merges ahead as they try to pull our code in from upstream. |
|
@andydotxyz Then that's my mistake then, the existence of this PR still open made me assume the feature was not yet supported. Perhaps close this PR as it sounds like you're saying this is redundant? Or ask @mgazza to confirm it's redundant and close it?
Completely true, but until I had a working solution, this had to be the solution. So to confirm, you're saying that Bold and Underline are supported and that Terminal now correctly renders Bold and Underline text when the session outputs it? |
No. I am saying that both are /supported/ in the TextGrid that Terminal uses. Underlines will appear as they are coded in as lines. The bold font will not appear until the terminal (or your app's custom theme) introduce the added fonts necessary to have bold, monospaced (not bundled in Fyne to save space). This PR adds them, among other no-longer required changes. |
|
That clarifies it, thanks. I'll chat to the PR owner about what that means for this work. As you say, it seems some it certainly redundant. |
|
@mgazza I suggest this is closed, almost everything here is replaced with a couple of minor changes. I'll try and find the time to PR those changes. |
Punt/ Preview PR
Change the font to one that has a bold derivative - (I probably should use fyne bundle instead of go embed).
Add the handling code for supporting underlining and bold.
Make that info available for downstream consumption for fyne.
depends on :
#45
fyne-io/fyne#4244