Add blinking

[mgazza]

Add the ability to process blinking text

[andydotxyz]

I don't see the purpose of BlinkEnabled - it is always checked at the same time as Blink.
Also the new parameter to the constructor seems to always be false...

[mgazza]

blink enabled tracks if the cell should blink, and blink tracks the state of the blink, i.e blinking/ not blinking.
It is set to the current blinking value in handleOutputChar

[andydotxyz]

Oh you mean like Blink and IsBlinking?
I guess the latter is just internal to the render state then - it doesn't seem like it should be on the widget/style API...

[mgazza]

Yeah I guess shouldBlink and isBlinking would be more descriptive. Where do you suggest we store that info since its per cell, and in the case isBlinking is true we should render the background color as to make it invisible?

…

On Mon, 23 Oct 2023, 22:13 Andy Williams, ***@***.***> wrote: Oh you mean like Blink and IsBlinking? I guess the latter is just internal to the render state then - it doesn't seem like it should be on the widget/style API... — Reply to this email directly, view it on GitHub <#45 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD3ZWQ5XR5HVHWMDX77ZBE3YA3MWJAVCNFSM6AAAAAA6D6RD66VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZWGAZTAMRTG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[andydotxyz]

I guess I'm not following why every cell needs to know if it is currently blinking... a cell that should blink will do so on a timer... a timer in (or signalling) the renderer will cause an inversion through some state, but surely this state is in the renderer not the cell?
If every blinking cell toggled at a different time / inversion it would be headache inducing.

[mgazza]

I mean it doesn't have to be for each cell, it could be for a cell block and tracked in the renderer.
However due to our previous conversations, the render currently doesn't have a mechanism to do this within the fyne term project, you would have to make the change(blink) available to the fyne text grid render.
Blinking is turned on via ESC[5m and turned off by ESC[0m in the same way colors are it felt like probably the least invasive way. There is a single timer for all of the blink's so they are all synchronized.

[andydotxyz]

I guess I was meaning renderer in the wider sense as in "not the widget api".

From an API design point of view there is 1 new feature here "blink". So it should be matched by 1 new api for the state. Do you not think it is possible to somehow encapsulate the blinking state internally in a way so we get that clean API?

[mgazza]

Can you suggest such and API? The spec for terminal blinking means it can be set or unset per each cell.

[andydotxyz]

Can you suggest such and API? The spec for terminal blinking means it can be set or unset per each cell.

Not really - what I mean is that the "IsBlinking"/"Blink" should not be needed at all. Yes a cell can be set to individually blink or not - but the escape sequence does not control when the blink occurs - so why would the public API expose that? Isn't it rendering metadata?
We control what is drawn because we embed the textgrid - and the single timer you have will be responsible for toggling all the cells that have blinking enabled won't it?

[mgazza]

Blink is the storage for should this cell be blinking.
IsBlinking is the current rendering state. if we remove this then we need to store it somewhere.
The render is a logical place, do you agree that we should track this here?

[andydotxyz]

Sounds good.
As above the public API should just be for things that are user configuration/features.
If we need the other field then yes it should be stored elsewhere.

[mgazza]

OK i'll take a look to see if theres any reason I can/or cannot add this. It will probably be easier to add this to the render in a new PR and then utilise it in this one

[andydotxyz]

I don't think we should add a new public API just to remove it later - once this is merged to the main branch the API exists for any public usage...

[mgazza]

Just looking at this now. since textGridRender is private I will have to copy/paste its content

[mgazza]

Is it possible for you to please review this now( i can fix conflicts etc later)?

[andydotxyz]

From a public API point of view this is better thanks - but the reverse-lookup of widgets to renderer is overhead that should not be needed (also holding onto a renderer is to be avoided).
I think the reason is because you're still running the actual blink in the model instead of renderer - can't this be put in the renderer itself? Started on widget creation and stopped on the destroy call...

[mgazza]

I did start with a global variable to test if it would work, but that would mean moving the control somewhere central which i don't like.
Let me try something and push that, if the approach is ok then I can clean all this up.

[andydotxyz]

I did start with a global variable to test if it would work, but that would mean moving the control somewhere central which i don't like.

I don't know why global variables are needed. A widget renderer can contain the blink refresh loop - and a renderer can reference the widget it is rendering so the state could be there.

Am I missing something?

[mgazza]

No I was just explaining my development process when i did this. Please see the last commit and see if this aligns with your thinking :D. If it does, then i will clean up this PR then we can do a review. I hope thats ok?

[mgazza]

we could be smarter and only refresh the cells that need to blink

[andydotxyz]

Please see the last commit and see if this aligns with your thinking

Thanks, this does seem to have the code in the right places.

we could be smarter and only refresh the cells that need to blink

That would be ideal yes - though as long as the ticker is not running when no cells are blinking the rest could be moved to an optimisation PR if easier.

[mgazza]

Cool, I'm away for 11 days, I'll clean this up asap. I'm not sure how I can detect if theres anything blinking as yeah right now this re-runs on the ticker which isn't great!

[mgazza]

I could probably trigger from the terminal processing, and detect nothing to "blink" when drawing. however I would have to be sure that I was single threaded between these two events. Also I would need some relationship between both the render and the terminal. Have you got any ideas?

[andydotxyz]

I could probably trigger from the terminal processing, and detect nothing to "blink" when drawing. however I would have to be sure that I was single threaded between these two events. Also I would need some relationship between both the render and the terminal. Have you got any ideas?

I'm not quite sure I follow here, sorry. The status of "BlinkEnabled" is communicated to the renderer through the model of all the grid cell styles. If one of those becomes true then the ticker should start and if they all become false it should stop (or if the renderer is destroyed). I don't see where the terminal relationship or thread complications comes in...

[mgazza]

I'm back! OK, I think i've sorted it!

[andydotxyz]

Overall this looks good.
Not done an in-depth review as there are conflicts and some dead code to clean up (lookup.go not needed)

[mgazza]

Thanks. I'll cleanup this PR so its ready for merging.

[mgazza]

Ready for merging :D

[andydotxyz]

Thanks so much, this is looking great