GNOME Bugzilla – Bug 579964
vte, hence Gnome-terminal, does not support blinking text
Last modified: 2019-11-05 11:52:24 UTC
The command: tput blink; echo "blinking text"; tput sgr0 produces a blinking text in the terminal window. It works fine in "xterm" and "urxvt". I would like this to work to be able to use gnome-terminal to connect to to legacy vt220 applications.
The lack of support for blinking text in gnome-terminal can be verified using the vttest program available here: http://invisible-island.net/vttest/vttest.html
Escape sequence is still not honored for blinking. For example the following works in xterm: echo -e "Normal \e[5mBlink" in latest vte 2.90 doesn't work
It's unlikely to be implemented, ever.
I was thinking on how to implement this one. Maybe I'll do it at some point, even though it's very low priority and of questionable usefulness. Technical bits: Obviously there needs to be a timer, and obviously (to save CPU wakeups) the timer needs to be stopped if there's no blinking text visible. For the sake of simplicity, I'd probably always perform a fullscreen repaint even if there's a single blinking cell, so we don't have to bother with bounding box. Such an update cycle can tell us if the timer can be stopped. For design parity and simplicity, I guess it would also be the drawing part of the code (vte_terminal_determine_colors_internal() perhaps) that starts the timer if necessary. (Starting the timer where the ring contents change would lead to tons of missed or tricky cases, e.g. when dragging the scrollbar, so it's not the right place.) UX bits (this is the more interesting): Do we want blink in all the windows, or only in the focused one? urxvt blinks in unfocused window, xterm and konsole don't (although xterm seems to be buggy). My take on this: the human eye sees sharp where it looks at, but notices changes much better in the periferic field. Blinking is useful to call your attention to something very important. I think if blinking is ever implemented, blinking the unfocused window is probably at least as important as blinking the focused one. Do we care if blinking multiple windows are in sync? I think it looks nicer if they are, but it's not necessary. Should blinking the text be synchronous to blinking the mouse cursor? In konsole it is, in xterm it's either in sync or totally opposite, in urxvt they're not synchronized (although are of the same frequency). Separate blink cycles look totally silly, konsole's behavior looks the best. But if we go for this, do we misuse cursor-blink-time for blinking actual content too? And do we give up stopping the blinking while typing (as in konsole, where the cursor keeps blinking all the time)? Or do we still stop blinking it while typing, and syncronize back later to the text's blinking period? Then it might be a bit weird that after you stop typing, there's a varying amount of time before cursor blinking starts again (maybe just because there's blinking content in another window).
What comment 3 meant was that there doesn't seem to be an actual reason to implement the most annoying thing ever, aka blinking text.
I don't think GNOME should decide what's annoying a what's not (even though I hate it personally as well ;) )... blink is there for nearly ever and people should expect that it works. What seems even worse is, that gnome-terminal sets TERM=xterm (or is this just some Debian speciality??) thus things like tput blink report the wrong exit code (0 instead of 1) because xterm supports blink while gnome does not. Cheers, Chris.
> I don't think GNOME should decide what's annoying a what's not I kinda agree with this... although what should be the reference? Do we need to reimplement every feature of xterm, including e.g. the Tek window (which I have absolutely no clue what it's doing)? xterm contains many-many features and escape sequences that we don't (yet) implement, and probably don't even plan to implement anytime soon. Consider blink just one of these dozens of such features. > blink is there for nearly ever and people should expect that it works. Just for reference: Firefox killed the <blink> tag in v23 (Aug 2013). I don't think many people shed a tear or complained that it should be kept. > TERM=xterm (or is this just some Debian speciality??) It's mainstream vte (actually xterm-256colors nowadays, but it's irrelevant here). There were times when it used to set TERM=vte or TERM=gnome-terminal or something like this; deploying and maintaining these terminal definitions across all Unix flavors probably caused more problem than good. We try to emulate xterm reasonably close – but there quite a few intentional (and sure some accidental) deviations. Vte does recognize the escape codes that en/disable blinking, so no harm is done other than the text not actually blinking. Does the "tput blink" exit code cause actual problem somewhere?
Part of why blink isn't implemented is that it's too much work with little benefit.
(In reply to Egmont Koblinger from comment #7) > I kinda agree with this... although what should be the reference? Do we need > to reimplement every feature of xterm, including e.g. the Tek window (which > I have absolutely no clue what it's doing)? No absolutely not,... As I've said, I don't like blink myself either (its ugly ^^)... the reason why I've suggested to implement it isn't because XTerm has it,... but rather because it's "standard" on many terminals. Since I like gnome-terminal quite a lot (at least as long as you don't switch to GTK+ CSDs o.O)... I feel that it should support that attribute. But apart from that, the main problem here is again that you announce a TERM which you aren't (see the other bug). So if e.g. someone want's to make his super important message like "Intruder alert" very visible and chooses it to blink, it would be just normal text in GNOME. Even when that someone was so smart to check for the blink cap (and e.g. use red background if it's not implemented) he would fail, since you announce the wrong terminal. > Just for reference: Firefox killed the <blink> tag in v23 (Aug 2013). I > don't think many people shed a tear or complained that it should be kept. Uhm?! Terminal blink and HTML <blink> element are two completely different pairs of shoes. <blink> was *always* non-standard HTML, but there is a replacement in CSS (not sure though how well this is supported). > There were times when it used to set TERM=vte or TERM=gnome-terminal or > something like this; deploying and maintaining these terminal definitions > across all Unix flavors probably caused more problem than good. We try to > emulate xterm reasonably close – but there quite a few intentional (and sure > some accidental) deviations. Well but it adds new problems on the other hand... and it kinda contradicts breaks the design ideas if terminfo: if the idea would have been to make a "base" set of caps which should be supported by all terminals, than this wouldn't be "xterm" but "base" and we'd probably still have only 8 colours ;-) > Vte does recognize the escape codes that en/disable blinking, so no harm is > done other than the text not actually blinking. Does the "tput blink" exit > code cause actual problem somewhere? Well if someone somehow depends on that (see my example above) he'd be screwed. If someone else changes in xterm's definition and you don't comply anymore, he'd be screwed either. Several other "standard" things (IIRC e.g. rmul) actually produce different sequences for xterm* and gnome*. Plus the problem that you basically make it impossible for anyone to detect the running terminal, if he wants to for whichever reason :-( As said in the other bug, the best solution would be to do as many other Terminals (e.g. rxvt) already do and announce the right name, and delegate problem solving with remote connections to the SSH level. Or at least, allow people to configure their TERM=, so that those who don't want breakage can chose :) Best wishes, Chris.
(In reply to Christoph Anton Mitterer from comment #9) > So if e.g. someone want's to make his super important message like "Intruder > alert" very visible and chooses it to blink, it would be just normal text in > GNOME. > Even when that someone was so smart to check for the blink cap (and e.g. use > red background if it's not implemented) he would fail, since you announce > the wrong terminal. Are we talking about an actual real-life scenario or a hypothetical one? > If someone else changes in xterm's definition and you don't comply anymore, > he'd be screwed either. If someone changes xterm's definition then even xterm is immediately screwed, due to the complete lack of versioning. > Plus the problem that you basically make it impossible for anyone to detect > the running terminal, if he wants to for whichever reason :-( There's $VTE_VERSION for this purpose. The response to \e[>c might help too. > As said in the other bug, the best solution would be to do as many other I'm really not convinced it would be the "best" solution. It would be another solution with other pros and other cons. Gnome-terminal has been there, and I assume it changed for a good reason.
By the way: Blink is utterly broken on xterm too. It doesn't blink when the window is not focused, and it might stop at the "off" phase. Mouseover toggles the state. Producing output toggles the state too, so printing some blinking text might result in that text not appearing at all, and other text also disappearing at the same time. I'd argue that what we do is already better :P
Hi, I'm the original reporter of this bug. For what it's worth, this feature is no longer important for me. Since I reported the issue I have switched jobs and blinking text on a terminal emulator is no longer in my life (thank $DEITY for that!!) Cheers, Lucas Vieites
For what is worth I made the comment nr2 and the reason for looking at having the text blinking was to have monitoring scripts alerting with blinking text for important alerts. The reason is to attract attention on important alerts over a list of other alerts. This is still a reason today if gets implemented. Regards
If chpe gives green light, I'm willing to implement blink.
I'm fine with adding this, provided it doesn't complicate the code too much. (In reply to Egmont Koblinger from comment #4) > Do we want blink in all the windows, or only in the focused one? urxvt > blinks in unfocused window, xterm and konsole don't (although xterm seems to > be buggy). My take on this: the human eye sees sharp where it looks at, but > notices changes much better in the periferic field. Blinking is useful to > call your attention to something very important. I think if blinking is > ever implemented, blinking the unfocused window is probably at least as > important as blinking the focused one. Yes. E.g. think two terminal windows side-by-side, they're both 'active' but only one has 'focus'. So should definitely blink all 'visible' windows, and with a compositing WM, that's all windows. > Do we care if blinking multiple windows are in sync? I think it looks nicer > if they are, but it's not necessary. IMHO the only thing worse than blinking text would be blinking text with the blinking not being in sync different windows :-) > Should blinking the text be synchronous to blinking the mouse cursor? In > konsole it is, in xterm it's either in sync or totally opposite, in urxvt > they're not synchronized (although are of the same frequency). Separate > blink cycles look totally silly, konsole's behavior looks the best. But if > we go for this, do we misuse cursor-blink-time for blinking actual content > too? And do we give up stopping the blinking while typing (as in konsole, > where the cursor keeps blinking all the time)? Or do we still stop blinking > it while typing, and syncronize back later to the text's blinking period? > Then it might be a bit weird that after you stop typing, there's a varying > amount of time before cursor blinking starts again (maybe just because > there's blinking content in another window). I do think blinking should be sync with cursor blinking; and the cursor blinking code could be just a special case of 'something blinking'. Also, maybe blinking text (and cursor) shouldn't just blink on/off, but fade in/out. (Only if gtk-enable-animations=true, of course.)
(In reply to Christian Persch from comment #15) > I'm fine with adding this, provided it doesn't complicate the code too much. Cool! > Yes. E.g. think two terminal windows side-by-side, they're both 'active' but > only one has 'focus'. So should definitely blink all 'visible' windows, and > with a compositing WM, that's all windows. Especially with the missing-focus-events bug, I couldn't agree more. (Not sure if I'll be able not to update the inactive tabs, but I don't care too much about those few CPU cycles.) > IMHO the only thing worse than blinking text would be blinking text with the > blinking not being in sync different windows :-) If someone wants to be annoyed, I don't want to stop them :) I'll try to go for this, but code simplicity will probably be at least as strong as this preference. > I do think blinking should be sync with cursor blinking; and the cursor > blinking code could be just a special case of 'something blinking'. Ditto. I'll see when writing the code what seems to be the best compromise between simplicity and nice look. > Also, maybe blinking text (and cursor) shouldn't just blink on/off, but fade > in/out. (Only if gtk-enable-animations=true, of course.) I'm not planning to go in this direction; it'd probably annoy me too much :D But if you do it separately for the cursor only, I'll merge to have it for the text too.
Created attachment 333940 [details] [review] Blinking proof of concept v0 Could it be that it's _this_ simple to implement blinking? :D I mean, it's damn simple, and it works! There's just no way to get it perfect what to synchronize blinking to (e.g. whether to sync across terminals, or sync to the cursor, and in the mean time also make the cursor start blinking as soon as you press a key...). So I went for the simplest possible implementation, which is to use the system clock. Hence it's not synchronized with the cursor at all, but different vte widgets (even across different apps) are synchronized with each other. It's important that we do blink if there's any visible content, however, we save on CPU and wakeups if there's none. The latter one includes the case of partially obscured window (the blinking text being obscured) with non-compositing WMs. The design is this simple: Each VTE handles blinking separately. If there's ever a blinking cell encountered during a paint, it schedules an invalidate_all for the next integer second, unless it's already scheduled. Then this invalidate_all will trigger a repaint for the visible parts, which again will automagically set a new timer for 1s later if there's any blinking part painted (but will not if no blinking text was painted), and so on... TODO: Respect gnome's blinking frequency. TODO: Maybe the code would be a bit nicer if determine_colors() and friends just bubbled up the info that blinking needs to be started, and remained const. E.g. dumping an html output wouldn't start a blink cycle. On the other hand, figuring out which phase we're in and calculating the remaining time should ideally be kept at the same place, and the former needs to stay in determine_colors(), so the delay until the next blink should be passed up to the caller who'd be responsible for setting the g_timer. Opinions welcome. TODO: How to handle if the terminal ceases to exist and there's a pending timer? How does the cursor blink stuff handle it? I couldn't find it in a few secs. I hope there's some generic magic so I don't need to do anything.
Created attachment 333946 [details] [review] Blinking proof of concept v1 v1: (Mis)use cursor-blink-time rather than a hardcoded value.
Review of attachment 333946 [details] [review]: ::: src/vte.cc @@ +8525,3 @@ + int blink_time = 1000; + struct timeval tv; + long now; Gotta be unsigned, 'cause it can overflow on 32 bits, and doing modulo with negative numbers sucks.
(In reply to Christian Persch from comment #15) > Also, maybe blinking text (and cursor) shouldn't just blink on/off, but fade > in/out. (Only if gtk-enable-animations=true, of course.) I would have loved to give it a try now. Alas, determine_colors() returns palette indices, converting to RGB happens in a later step. I really don't think it's worth the refactoring it requires.
(In reply to Egmont Koblinger from comment #20) > I would have loved to give it a try now. Alas, determine_colors() returns > palette indices, converting to RGB happens in a later step. I really don't > think it's worth the refactoring it requires. That's a refactoring I want to make for other reasons. I have a start in a branch which I'll dust off when I get time. It makes it so that the palette is resolved when the palette is set (from API or sequence), and painting will have the resolved palette available.
(In reply to Christian Persch′ from comment #21) > That's a refactoring I want to make for other reasons. You'll need to have at least 16M+1 colors (for "default" background). It's fine if you use some RGBA object, and use its alpha channel to denote the default, which will probably end up as a solid color anyways. I guess you figured it out already. Once you're done, I'm happy to give fading-blinking a try. What do you think of the current blinking patch until then?
(In reply to Egmont Koblinger from comment #22) > [...] and use its alpha channel to denote the default [...] ... or, me stupid, just place the resolved color here as well, which may or may not be transparent/translucent :)
(In reply to Egmont Koblinger from comment #22) > What do you think of the current blinking patch until then? Hmm... not sold on doing all this inside determine_colors() :-) I'd have thought we could just keep record in the row data whether the row has any blinking cells, and do blinking iff any visible rows do have one...
(In reply to Christian Persch′ from comment #24) > Hmm... not sold on doing all this inside determine_colors() :-) I don't like it either. determine_colors() should remain const and time-agnostic. I've been thinking on how to do this properly. Not entirely sure yet. I'll give it another try, hopefully something better will come out of that :) > I'd have thought we could just keep record in the row data whether the row > has any blinking cells, and do blinking iff any visible rows do have one... I don't like this approach. Blinking should belong to painting, not to the underlying data representation. Maintaining this flag in row_data is quite error prone I'm afraid, there are too many ways the data there can be updated and we could miss something. It's also expensive, since it needs to be done on each modification. Also, having it per-row is a bit arbitrary (why not per-cell or just one for the whole screen?). Let's not forget that if you scroll back, it's the onscreen contents that matters (the data we've encrypted and written to the streams), not the ring. Reading it again solely for the purpose of blinking is expensive, so we should keep an eye on blinking when we process that data for displaying anyways. Making blinking the responsibility of the underlying ring/stream independently from painting is expensive (needs to do stuff on every content modification rather than on every paint), error-prone and troublesome (e.g. how would it know when to start and when to stop the timer?), also just something that really shouldn't be there (the data shouldn't push its visual representation, it's the other way around, painting should query the data). I see no advantage in splitting responsibility between the two areas either, rather than having the entire responsibility at painting. And if we do it in painting, having a per-row "blinks" flag does not help us at all. Also, if there's blinking content in an inactive tab of g-t, I really do not want g-t to wake up every second just to figure out there's nothing to do. There's no direct way the ring/stream could know if the contents are visible. The only indirect way of knowing it is to see if Gtk+ actually asks us to paint the contents. (We can't rely on focus either, since I definitely do want unfocused vtes to blink as well, or at least keep this possibility (*)) Long story short, I definitely intend to keep the entire story at the painting logic, and not to the slightest bit in the rowdata/ring/stream storage. (*) By the way, how about an API to enable/disable blinking? Blinking is indeed annoying in many cases and I can imagine that some users want to turn this off. Also, having such an API would allow applications to decide to stop blinking for unfocused windows/panes.
Created attachment 334183 [details] [review] Blinking v2 Moved blinking from determine_colors() to draw_cells(). Much better, isn't it?
Created attachment 334185 [details] [review] Fading demo (for v2) And here's a quick demo for fade in/out effect (goes on top of the previous patch).
- I find fade in/out at least as annoying (probably even more annoying) than simple blinking. - All patches so far handle transparency incorrectly (the text in "off" state is rendered with the opaque background color, rather than transparently). There's a special case for invisible text, I guess I need to watch for blink's state there too. Probably reasonably doable for on/off blinking but quite troublesome with fading.
Created attachment 334187 [details] [review] Blinking v3 v2 handled incorrectly a continuous run of blinking cells with different backgrounds. Fixed now. Transparent background is also fixed. What I do not like now is that the "off" state of blinking is handled in draw_cells(), whereas the invisible property is handled in draw_rows() (and checked here twice, not sure why). It would be nice to clean it up. (Thoughts?)
Created attachment 334190 [details] [review] Blinking v3 + API The same as v3, plus API introduced. It's a piece of cake to add a boolean per-profile (hidden) setting to g-t whether to enable blinking. I'll take a look how hard it is to add a tri-state (only blink the focused one).
Created attachment 334211 [details] [review] Blinking v3 + API v2 I changed my mind wrt the API, made it a tri-state instead of a boolean. Strictly speaking, the "focused" state is not necessary, it can be emulated by the container app my toggling the on/off state back-n-forth as the focus changes; however, it would be really cumbersome (and replicated across quite a few apps). Instead, it's a piece of cake to implement it iin vte, since it already knows if its focused for a very similar reason (the blinking cursor). So I believe it's of a huge convenience for apps to have this third option. The naming of methods/constants is inconsistent, I couldn't make up my mind whether to use the word "allow" or not, "contents" or "text" or neither, "blink" or "blinking", "mode" or not. Needs to be cleaned up, guidance welcome. The actual blinking code is still the same, hence I still call this patch "v3". g-t patch is coming too, you can set the per-profile "allow-blink" to "always", "never" or "focused" in gsettings.
Created attachment 334212 [details] [review] Blinking v3 + API v2 (gnome-terminal bits)
Created attachment 334225 [details] [review] Blinking v3.1 + API v2 Version 3.1 (almost the same as version 3). I've seen a couple of segfaults. As I've mentioned earlier, I did not remove the pending timer when the terminal ceases to exit. It's fixed now. I _hope_ this was the reason for the segfaults (although I wonder why there was no Gtk+ warning at all, nor did it segfault way more frequently).
Created attachment 334226 [details] [review] Blinking v3.2 + API v2 Version 3.2: Add cmdline flag to the test apps; minor doc fix.
- Segfaults are clearly gone, they were sure caused by not removing the stale timer. - As for the naming: I'd definitely like to have the word "allow" in it, just like allow_bold, and unlike the blinking cursor property. The cursor is almost always onscreen, so it makes sense to simply talk about its blinking. Talking about blinking the text without the word "allow" would sound like we wanted to blink all the normal text for no particular reason. I'm still uncertain about the rest of the naming (see comment 31). - Pending: Is the code at the right place? Do you like the overall design? - Pending: Would it be a good idea to move the handling of the invisible attribute here as well? (I think it would be.)
- I'm also thinking to add an "unfocused" version, to blink only unfocused widgets. It may sound unusual, but motion is best noticed in the peripheric vision. For windows outside of your eye's current focus it's a good way to call your attention at something important which you probably notice anyways even without blinking if you're looking at that particular window. - Christian, any chance of getting this into 0.46 (freeze in a week)? :)
I hadn't realised you wanted to get this into 0.46... let's review :-) Naming this 'allow' with a non-boolean is a bit odd to me... maybe call it VteBlinkMode/blink-mode instead? The patch looks mostly fine (apart from the placement of installing the timeout), a few nits: <SUBSECTION Standard> +VTE_TYPE_ALLOW_BLINK That should be VTE_TYPE_BLINK if we don't rename it as per above. + /* Remove the contents blink timeout function. */ + remove_blink_timeout(); + Nit: move this up a few lines so it's just with removing the cursor blink timeout. +static gboolean +invalidate_all_cb(VteTerminalPrivate *that) Name this so it's clear it's the blinking timeout, maybe blink_timeout_cb or so? @@ -8579,6 +8610,28 @@ VteTerminalPrivate::draw_cells(struct _vte_draw_text_request *items, rgb_from_index(fore, fg); rgb_from_index(back, bg); + if (blink && [...] I don't really like the place. I'd like drawing to not change anything in VteTerminalPrivate (since I want to refactor drawing), so I'd propose to make this code return (inout value?) whether any cells had blinking, and only install the blink timeout after all drawing has been done. + unsigned int blink_time = VTE_BLINK_TIME_DEFAULT; + guint64 now; + + g_object_get(gtk_widget_get_settings(m_widget), + "gtk-cursor-blink-time", &blink_time, + nullptr); Usually we use glib types with glib API, so |guint blink_time| here. + if (m_contents_blink_tag == 0) + m_contents_blink_tag = g_timeout_add_full(G_PRIORITY_LOW, + blink_time / 2 - now % (blink_time / 2), + (GSourceFunc)invalidate_all_cb, + this, + NULL); nullptr. +typedef enum { + VTE_BLINK_NEVER, + VTE_BLINK_ALWAYS "ALWAYS" is a bit odd, since it only blinks if there's actually blinking attr used. Maybe DISABLED/ENABLED ? OFF/ON ?
The g-t bits patch (attachment 334212 [details] [review]) is fine; we can file a new bug to add UI for the pref (for 3.23.x).
(In reply to Christian Persch from comment #37) > I hadn't realised you wanted to get this into 0.46... let's review :-) Me neither, until I checked the deadlines :) Well, obviously it's not that super important, but if both of us have enough time and come to an agreement then why not... > + if (blink && > [...] > > I don't really like the place. I'd like drawing to not change anything in > VteTerminalPrivate (since I want to refactor drawing), so I'd propose to > make this code return (inout value?) whether any cells had blinking, and > only install the blink timeout after all drawing has been done. The problem with this is: - either you need to inject to this method whether blinking is in on or off state, resulting in tons of g_get_(real|monotonic)_time() calls even when there's no blinking contents at all (not sure how expensive this is), - or the timing logic is split to two places: drawing is responsible for figuring out the blinking phase, yet its caller is responsible for figuring out the remaining time (ugly and error prone at further refactorings), - or the return value should be the remaining time, which will be ignored by the caller if the timer is already set (that is, the caller assumes that there's at most 1 kind of blinking; everything that "expires" expires at the same time), or the caller reinstalls the timer to the MIN of the expire times seen so far - seems already overengineered and overcomplicated to me. What do you think? I'm really uncertain which one to go for. I have a feeling that sure the current solution is a tiny bit hackish, but trying to avoid this hack would lead to a way too complex overengineered solution. I don't particularly see a problem with drawing being responsible for initiating a redraw when whatever it's just drawing will expire. Anyways, if we try to hit 0.46 it's the API that we should finalize, the internal implementation we can refactor later if needed. (I'll get back to the rest of your comments soon.)
Sorry, I got busy and I won't have time to make it until Monday. And we still have quite a few pending details (including the API naming). So I can: - either commit the simplest implementation (v3.1 or so) with no API, and we'll refactor and add the API later; - or defer completely until 0.48. Which one would you prefer? (The advantage of the first approach is that user feedback (or lack thereof) might suggest to us that we won't need an API at all :))
Let's do this for 0.48 then.
(In reply to Egmont Koblinger from comment #39) > - either you need to inject to this method whether blinking is in on or off > state, resulting in tons of g_get_(real|monotonic)_time() calls even when > there's no blinking contents at all (not sure how expensive this is), So I was afraid of doing this in the false belief that getting the time was perhaps a bit expensive operation. It's totally not. My Core i5 @ 2.3 GHz can do about 35 million g_get_(real|monotonic)_time() calls per second, or a bit more of raw gettimeofday(). "time" shows all this as user time and not sys, and strace/ltrace shows no activity. Seems there's some magic involved that there's no kernel context switch. No idea how and where this magic comes from (kernel/glibc/gcc). So I'll go for figuring out the blink phase before every paint operation, even though it usually won't be used. (If we wanted to avoid this, and wouldn't want to go with the current approach [which I agree is not nice] then we'd have to go full proper OO, something like passing an object implementing an interface with methods like getting the blink phase... it'd be a total overkill here.)
Created attachment 360999 [details] [review] vte, v4 Ported to latest git, added "unfocused" option.
Created attachment 361000 [details] [review] g-t, v4 Ported to latest git, added "unfocused" option.
Created attachment 365630 [details] [review] vte, v4.1 Ported to latest git. Now that we have tons of new decorations for 0.52, I'm planning to finish this one to complete the picture :)
Created attachment 365728 [details] [review] vte, v5 Finally a version that I quite like; hope Christian that you'll also like it :) Moved the blinking (timing etc.) logic to the outmost possible place, right where GTK+ calls us to paint, i.e. the ::widget_draw() method. draw_cells() and friends only care about two member fields. One tells them the current on/off state, another one they flip if they encounter cells with the "blink" attribute. This latter is a tiny bit ugly, goes against the "const"ness of the structure which was removed anyway at some point. The other possibility, bubbling up in an outbound parameter adds some overhead to carry via multiple hops of methods, but I'll do that if you really insist. Hopefully much cleaner and more consistent naming of the API and internal stuff too.
Created attachment 365729 [details] [review] g-t, v5 Updated to the new API names. Added a UI entry. Looks ugly. If we figure out how to make it not ugly then we should probably also address bug 559990 the same way.
Review of attachment 365728 [details] [review]: ::: doc/reference/vte-sections.txt @@ +54,3 @@ vte_terminal_set_cursor_blink_mode +vte_terminal_get_text_blink_mode +vte_terminal_set_text_blink_mode Need to add VteTextBlinkMode too.
Review of attachment 365729 [details] [review]: ::: src/profile-preferences.ui @@ +72,3 @@ + </row> + <row> + <col id="0" translatable="yes" comments="Text blink mode">Only in focused terminals</col> Should be singular.
This is much better than v4. Having the extra bool in the struct to signal from ::draw_cells upwards instead of having a param on the function is fine. + m_text_blink_mode = VTE_TEXT_BLINK_ALWAYS; [...] + pspecs[PROP_TEXT_BLINK_MODE] = + g_param_spec_enum ("text-blink-mode", NULL, NULL, + VTE_TYPE_TEXT_BLINK_MODE, + VTE_TEXT_BLINK_ALWAYS, Not sure that really should be the default... :-) I'd prefer opt-in (default to NEVER). That way, users of frontends that haven't updated to disable/add a user pref won't get blinked at. What do you think? +typedef enum { + VTE_TEXT_BLINK_NEVER, + VTE_TEXT_BLINK_ALWAYS, + VTE_TEXT_BLINK_FOCUSED, + VTE_TEXT_BLINK_UNFOCUSED Move ALWAYS to the end, so it's value (3) is FOCUSED + UNFOCUSED. Or maybe this should be a flag, not an enum? + g_object_get(gtk_widget_get_settings(m_widget), + "gtk-cursor-blink-time", &blink_time, + nullptr); Could cache that in ::widget_settings_notify() instead of getting it on each draw. In ::draw_rows() there's some special handling of invisible cells in the /* Walk the line. */ loop; shouldn't this be update to also check if the cell is blinking but the blink cycle is 'off'? Otherwise looks good :-)
+ <col id="0" translatable="yes" comments="Text blink mode">Nope</col> + <col id="0" translatable="yes" comments="Text blink mode">Yup</col> + <col id="0" translatable="yes" comments="Text blink mode">Only in focused terminals</col> + <col id="0" translatable="yes" comments="Text blink mode">Only in unfocused terminals</col> The wording could use a bit work ;-) Otherwise the g-t patch is fine, modulo the potential reordering of the enum values (comment 50).
(In reply to Christian Persch from comment #50) > Having the extra bool in the struct to signal from ::draw_cells upwards > instead of having a param on the function is fine. However, since we already walk the line in ::draw_rows(), could just do that work there instead in ::draw_cells() (which then wouldn't need to be called for cells that blink but are currently invisible dues to currently being in 'off' in the blinking cycle.)
(In reply to Christian Persch from comment #50) > This is much better than v4. Huhh :) I have to admit I had a giant mental obstacle to overcome. I returned to this bug a couple of times during the last bit more than a year, but until now never got the inspiration. I'm glad to see that we're finally on the same track. > Not sure that really should be the default... :-) I'd prefer opt-in > (default to NEVER). That way, users of frontends that haven't updated to > disable/add a user pref won't get blinked at. What do you think? I'd prefer the standards-complying behavior to be the default. It's not that stuff will suddenly blink for everyone, you'll also need an app that asks for blinking text which is rare to begin with, and probably can be disabled on those apps too. I imagine user going like "this should be blinking, why isn't it blinking?" at least as frequently as "blinking? wtf? it's annoying, let's stop it". I might be wrong of course. With other VTE-based apps in mind, yet another possibility to consider: g-t's default doesn't have to match VTE's. We could enable blinking by default in g-t, yet leave it disabled for other apps until they catch up. Or the other way around. That being said, my vote still goes for totally enabling it. > +typedef enum { > + VTE_TEXT_BLINK_NEVER, > + VTE_TEXT_BLINK_ALWAYS, > + VTE_TEXT_BLINK_FOCUSED, > + VTE_TEXT_BLINK_UNFOCUSED > > Move ALWAYS to the end, so it's value (3) is FOCUSED + UNFOCUSED. Sure, will do. (As far as I remember, the prefs UI order will follow this too, unless I introduce some reordering magic that doesn't yet exist elsewhere.) > Or maybe this should be a flag, not an enum? I'm not sure what you exactly have in your mind code-wise. Would it be like VtePtyFlags, with the generated vtetypebuiltins.cc somehow containing some g_flags_register_static() stuff; g_param_spec_flags instead of g_param_spec_enum; in gnome-terminal implement string_to_flags and flags_to_string similarly to their enum counterparts? What would the dconf value be? Etc... I actually thought about having two checkboxes on the UI :) Would be stupid I guess. But I still don't like the current look of the prefs dialog. What do you think? Is it okay? Do you have an idea to improve? Should we loop in Allan? > + g_object_get(gtk_widget_get_settings(m_widget), > + "gtk-cursor-blink-time", &blink_time, > + nullptr); > > Could cache that in ::widget_settings_notify() instead of getting it on each > draw. Okay.
(In reply to Christian Persch from comment #50) > In ::draw_rows() there's some special handling of invisible cells in the /* > Walk the line. */ loop; shouldn't this be update to also check if the cell > is blinking but the blink cycle is 'off'? (In reply to Christian Persch from comment #52) > However, since we already walk the line in ::draw_rows(), could just do that > work there instead in ::draw_cells() (which then wouldn't need to be called > for cells that blink but are currently invisible dues to currently being in > 'off' in the blinking cycle.) A while ago in comment 29 I mentioned that I don't like invisible and blinking handled elsewhere. I'm no longer sure about that. Invisible, and blinking that happens to be off right now are different in one very important aspect: The latter one has to trigger a forthcoming redrawing. There are 2 reasons I don't want to do this change: 1. The call path of method is something like this: The common entry point is widget_draw(). Then it goes in three or different branches: paint_area(), paint_cursor(), paint_im_preedit_string(), where (it's a bit complicated, I don't see the exact story, the rough picture follows) they somehow call draw_cells() directly or draw_rows() which in turn call draw_cells(). Sometimes there's also draw_cells_with_attributes() in between somewhere. Anyway, in the end, every branch somehow arrives at draw_cells() for painting text. So if I do something at the very beginning in widget_draw(), or in the very end in draw_cells() then I have to do it once only. If I do anything in some intermediate step, e.g. in draw_rows() which isn't always there in the call chain, then I have to repeat that logic at multiple places (and this is basically where a couple of my earlier attempts in continuing this stalled work failed again, it was just mentally unfollowable for me). 2. If we ever add fade in/out animation for blinking (I don't want to add this now, but keep its possibility in mind), this design could no longer work. Then blinking text needs to (almost) always make into actually being drawn, and then there's no point in special handling that "almost". --- Speaking about fading animation: See comment 27 & 28 for a previous demo; something like that can easily be done on top of the current patches too. With that zig-zag fading it looks quite bad, a sine wave is much better, but still a bit more annoying to me than simple blinking. There's one thing in the way that kinda blocks fade in/out, and that's invalidation. Currently we lazily invalidate the entire canvas, it's okay at 1 FPS, probably not okay at 30-ish FPS. We'd need to invalidate the blinking cells, or at least a bounding rectangle then. --- Speaking about bounding box: I almost implemented it, there was one tiny issue requiring some refactoring that made me omit from this patch. We'd might address in a followup refactoring + fix. The invalidate() calls require cell coordinates. In draw_cells() (where we flip that certain boolean to denote blinking text; instead we'd need to update the bounding box) we have pixel coordinates. There are no methods to convert backwards and that'd be plain ugly anyways. So either (less favorable to me) there should be an invalidate() taking pixels, or (more favorable to me) draw_cells() should know the cell coordinates.
(In reply to Egmont Koblinger from comment #53) > With other VTE-based apps in mind, yet another possibility to consider: > g-t's default doesn't have to match VTE's. We could enable blinking by > default in g-t, yet leave it disabled for other apps until they catch up. Or > the other way around. That being said, my vote still goes for totally > enabling it. I'd even be happy to file a heads-up bugreport against the ten or so well-known VTE-based terminal emulators. That would give them about 2.5 months to catch up, i.e. explicitly disable if that's what they prefer, or add a preference. It's more than enough time for actively developed frontends, while even years may not be enough for not-so-actively developed ones. Yet yet yet another idea: There could be some means of disabling it from gtk.css. Actually there's a CSS "animation-duration: 1s;" with the default value of 0 meaning no animation. How about it? I quite like it! Currently we misuse GTK's cursor-blink-time for the frequency. It would be a nice addition (independently from the API's default) to honor this CSS setting for the frequency, and only fallback to cursor-blink-time if it's not set. And as a by-product it would also allow some advanced, non-API means of disabling it.
I've found something interesting, totally new to me (probably not to you): If I specify the values in the enum declaration, like typedef enum { ... VTE_TEXT_BLINK_FOCUSED = 1, ... then nothing changes yet. However if I do some bit shifting: VTE_TEXT_BLINK_FOCUSED = 1 << 0, then the generated vtetypebuiltins.cc becomes different (goes with GFlags instead of GEnum), which causes everything to break (well, at least without plenty of other code changes). For the time being I wanted to stay with enums, but move ALWAYS to the end of the list as you suggested (which also allows a much simpler way to determine text_blink_enabled_now's value). I thought it'd be cool to assign values "0", "1 << 0", "1 << 1" and "(1 << 1) | (1 << 0)" to them. Well, looks like I won't. I really dislike that two formats that are equivalent in C and don't differ in code annotation either result in different generated code. Whatever. --- What we have here is not an ideal fit for "enum", but probably not an ideal fit for the "flags" pattern either. I think wherever I saw flags somewhere, it was for denoting the presence/absence of each of several individual properties. Here we'd like to denote the desired boolean behavior in two mutually exclusive situations, and hence by definition there couldn't be any more "flags" than these two ever added to the set. I think the typical pattern for handling this kind of situation is to have two separate booleans, although here that would make the API (and in turn perhaps vteapp's parameters or g-t's UI too) look stupid. If not going for the theoretically correct approach but for the one that seems most practical (in terms of simple API, as well as vteapp's parameters and g-t's UI just working out of the box without writing tons of new boilerplate), I think enum is the simplest-cleanest.
(In reply to Egmont Koblinger from comment #53) > With other VTE-based apps in mind, yet another possibility to consider: > g-t's default doesn't have to match VTE's. We could enable blinking by > default in g-t, yet leave it disabled for other apps until they catch up. Or > the other way around. That being said, my vote still goes for totally > enabling it. Ok. (Can always reconsider later :-) > I'm not sure what you exactly have in your mind code-wise. Would it be like > VtePtyFlags, with the generated vtetypebuiltins.cc somehow containing some > g_flags_register_static() stuff; g_param_spec_flags instead of > g_param_spec_enum; in gnome-terminal implement string_to_flags and > flags_to_string similarly to their enum counterparts? What would the dconf > value be? Etc... This is all automatic, but probably not worth changing now; let's keep it as enum. > I actually thought about having two checkboxes on the UI :) Would be stupid > I guess. But I still don't like the current look of the prefs dialog. What > do you think? Is it okay? Do you have an idea to improve? Should we loop in > Allan? UI-wise, the one 4-choice combo seems ok; 2 independent checkboxes would be weird, I think. (In reply to Egmont Koblinger from comment #54) > There are 2 reasons I don't want to do this change: > > 1. The call path of method is something like this: The common entry point is > widget_draw(). Then it goes in three or different branches: paint_area(), > paint_cursor(), paint_im_preedit_string(), where (it's a bit complicated, I > don't see the exact story, the rough picture follows) they somehow call > draw_cells() directly or draw_rows() which in turn call draw_cells(). > Sometimes there's also draw_cells_with_attributes() in between somewhere. > Anyway, in the end, every branch somehow arrives at draw_cells() for > painting text. > > So if I do something at the very beginning in widget_draw(), or in the very > end in draw_cells() then I have to do it once only. If I do anything in some > intermediate step, e.g. in draw_rows() which isn't always there in the call > chain, then I have to repeat that logic at multiple places (and this is > basically where a couple of my earlier attempts in continuing this stalled > work failed again, it was just mentally unfollowable for me). Ok. > Speaking about fading animation: See comment 27 & 28 for a previous demo; > something like that can easily be done on top of the current patches too. > > With that zig-zag fading it looks quite bad, a sine wave is much better, but > still a bit more annoying to me than simple blinking. > > There's one thing in the way that kinda blocks fade in/out, and that's > invalidation. Currently we lazily invalidate the entire canvas, it's okay at > 1 FPS, probably not okay at 30-ish FPS. We'd need to invalidate the blinking > cells, or at least a bounding rectangle then. Right, before we could do that full-redraw would need to be optimised heavily. > Speaking about bounding box: I almost implemented it, there was one tiny > issue requiring some refactoring that made me omit from this patch. We'd > might address in a followup refactoring + fix. Ok. > The invalidate() calls require cell coordinates. In draw_cells() (where we > flip that certain boolean to denote blinking text; instead we'd need to > update the bounding box) we have pixel coordinates. There are no methods to > convert backwards and that'd be plain ugly anyways. So either (less > favorable to me) there should be an invalidate() taking pixels, or (more > favorable to me) draw_cells() should know the cell coordinates. The latter sounds better to me too. (In reply to Egmont Koblinger from comment #55) > I'd even be happy to file a heads-up bugreport against the ten or so > well-known VTE-based terminal emulators. That would give them about 2.5 > months to catch up, i.e. explicitly disable if that's what they prefer, or > add a preference. It's more than enough time for actively developed > frontends, while even years may not be enough for not-so-actively developed > ones. Ok. > Yet yet yet another idea: There could be some means of disabling it from > gtk.css. Actually there's a CSS "animation-duration: 1s;" with the default > value of 0 meaning no animation. How about it? I quite like it! > > Currently we misuse GTK's cursor-blink-time for the frequency. It would be a > nice addition (independently from the API's default) to honor this CSS > setting for the frequency, and only fallback to cursor-blink-time if it's > not set. And as a by-product it would also allow some advanced, non-API > means of disabling it. I don't know the gtk CSS APIs well, but AFAIK we can't hook at the required level. When the animation is set in CSS, gtk itself runs it, which will cause massive full redraws. I don't think this one is worth it; also blinking text and cursor having different frequencies would be annoying, IMHO.
(In reply to Egmont Koblinger from comment #56) > I've found something interesting, totally new to me (probably not to you): [...] > then the generated vtetypebuiltins.cc becomes different (goes with GFlags > instead of GEnum), which causes everything to break (well, at least without > plenty of other code changes). You can also just use typedef enum /*< flags >*/ { ... } Foo; (And then you need to use g_param_spec_flags and g_value_[sg]et_flags etc, to fix the 'breakage'.) > For the time being I wanted to stay with enums, but move ALWAYS to the end > of the list as you suggested (which also allows a much simpler way to > determine text_blink_enabled_now's value). I thought it'd be cool to assign > values "0", "1 << 0", "1 << 1" and "(1 << 1) | (1 << 0)" to them. Well, > looks like I won't. It seems that an analogous /*< enum >*/ to override the autodetection doesn't exist, so yes, you can't do that :/ Let's keep it as enum then.
If I enable blinking when "focused" and open g-t's right-click menu, blinking stops. Then close that menu, blinking doesn't resume. I thought we did a full redraw upon focus in/out. (Was that the case earlier?) We don't. Interestingly, when I actually switch focus, the window does receive a full rewdraw. Do we do that in some other branch? Or does GTK+ do that? Why?? Anyway, will fix this.
(In reply to Egmont Koblinger from comment #59) > If I enable blinking when "focused" and open g-t's right-click menu, > blinking stops. Then close that menu, blinking doesn't resume. > > I thought we did a full redraw upon focus in/out. (Was that the case > earlier?) We don't. I think we still do (it's forced onto us by gtk's 'backdrop' CSS thingy). > Interestingly, when I actually switch focus, the window does receive a full > rewdraw. Do we do that in some other branch? Or does GTK+ do that? Why?? > > Anyway, will fix this. I missed in the review that you need to update blinking in ::widget_focus_in/out according to the pref. Now whether that should re-show the text if the focus stops blinking at the time the blinking text is hidden, or keep it hidden... I'll leave that up to you :-)
(In reply to Christian Persch from comment #60) > Now whether that should re-show the text if the focus stops blinking at the > time the blinking text is hidden, or keep it hidden... I'll leave that up to > you :-) See comment 11 :-) I was actually thinking how cool it would be if it still completed the current "off" cycle. But then so should newly appearing / newly repainted blinking text, which complicates our bookkeeping by at least another boolean or two. Probably not worth it. (Read: I'm not doing it now.)
(In reply to Christian Persch from comment #57) > I don't know the gtk CSS APIs well, but AFAIK we can't hook at the required > level. When the animation is set in CSS, gtk itself runs it, which will > cause massive full redraws. Just for curiosity: If there's an "animation-duration" property, maybe in combination with gtk-enable-animations=true, would GTK+ automatically begin to ask us to fully repaint frequently, without even having a clue what and how we're animating?? I meant to access and parse that CSS field manually, without interfering with anything in GTK. Just as if this field had a totally different random name. Which is still one possibility if we can't otherwise avoid GTK+ interfering with us. But this is all irrelevant for now because... > I don't think this one is worth it; ... you convinced me :)
(In reply to Christian Persch from comment #60) > > I thought we did a full redraw upon focus in/out. (Was that the case > > earlier?) We don't. > > I think we still do (it's forced onto us by gtk's 'backdrop' CSS thingy). To clarify, I meant to say: "I though we _explicitly_ did a full redraw...", i.e. we had an invalidate_all() in widget_focus_{in,out}. I was wrong here. So apparently most of the time, but not always, GTK+ forces a full redraw on us on focus in/out.
Created attachment 365825 [details] [review] vte, v5.1 (incremental) > + g_object_get(gtk_widget_get_settings(m_widget), > + "gtk-cursor-blink-time", &blink_time, > + nullptr); > > Could cache that in ::widget_settings_notify() I think this is the only remaining missing piece now.
(In reply to Egmont Koblinger from comment #49) > + <col id="0" translatable="yes" comments="Text blink mode">Only in > focused terminals</col> > > Should be singular. (In reply to Christian Persch from comment #51) > + <col id="0" translatable="yes" comments="Text blink mode">Nope</col> > + <col id="0" translatable="yes" comments="Text blink mode">Yup</col> > > The wording could use a bit work ;-) :-) This was intentional for the non-final version :-) I don't like the singular/plural difference in "Only in unfocused terminals" vs. "Only in (the?) focused terminal", so I'm thinking about changing them to "When (the?) terminal is focused" / "When (the?) terminal is unfocused". Are "Always" and "Never" good for the UI? I'm also thinking about a simple "Yes" and "No" but I'm afraid it looks a bit unprofessional. What do you think of these?
... or maybe simply "When focused" / "When unfocused".
Created attachment 365856 [details] [review] vte, v6 Caching done. Actually we do already cache gtk-cursor-blink-time for the cursor. I now store it in another variable as well. I didn't want to use the incorrect naming "m_cursor_blink_cycle" for text's blink, and didn't want to rename this variable all across the source. Another variable containing the same value is damn cheap. And if we'll ever have fade in/out blinking, I'm not sure it'll be essential to have it on the same frequency as cursor blinking. I just think it's cleaner this way (just as we have tons of m_[underlinetype]_thinkness now all containing the same value). Reverted the introduction of VTE_BLINK_TIME_DEFAULT as it's only used at one place, as before, the current patch doesn't change it. Turned the "Note" about dead code into a "FIXME". Christian, please take a (hopefully final) look, and please also advise on g-t's UI wording (I tend to vote for the concise "Always"/"Never"/"When focused"/"When unfocused"), thanks!
The code looks fine now, apart from one additional problem I just thought of now: suppose blinking is ALWAYS or UNFOCUSED, and in g-t you switch away from the tab with the blinking text. Does the timeout get removed immediately, or does it run one more time? ::widget_visibility_notify() would be the right place to remove it. The proposed wording in g-t Always/Never/When Focused/When Unfocused is ok too. Thanks!
(In reply to Christian Persch from comment #68) > suppose blinking is ALWAYS or UNFOCUSED, or FOCUSED. The tab you're switching away from (that is, its window) may or may not have been focused. > and in g-t you switch away > from the tab with the blinking text. Does the timeout get removed > immediately, or does it run one more time? ::widget_visibility_notify() > would be the right place to remove it. It's an absolutely harmless place where I indeed could've removed the timer and indeed it runs once more. Note that by design, blinking already does spurious invalidations / full redraws sometimes. The code doesn't know upfront when there's no longer blinking text onscreen (the blinking cells got overwritten by steady ones), it only realizes after a full redraw that didn't encounter any blinking stuff and hence doesn't reinstall the timer. I can of course add that one line here to remove the timer... but I was also wondering about going the opposite way. In invalidate_all() I remove the timer with a comment /* this sometimes saves us a full repaint after blinking has stopped, otherwise not important to have here */, but I'm tempted to delete this "optimization". Both there, as well as here as spotted by you, I'm wondering that probably, given how rarely blinking text will be encountered, it's better for the overall performance across all world-wide gnome-terminal installations not to do these checks. That is, perhaps spurious full redraws (or in the newly discovered case just a timer kicking in, taking a note that everything's already invalidated so there's nothing left to do) around blinking text is probably still cheaper than an additional check all the time when there's no blinking at all. :-) The only place where we must (obviously) remove the timer is when the widget is destroyed. Everywhere else it should be harmless to just leave it in place. It's a really insignificant detail and I'm fine with either approach, pick one :)
As long as it's just one extraneous timer call, that's fine either way; I'd only be concerned if we'd continuously timeout in a non-shown (hidden tab) terminal. So I'm ok with simplifying this not to remove the timeout in invalidate_all().
(In reply to Christian Persch from comment #70) > As long as it's just one extraneous timer call, that's fine either way; I'd > only be concerned if we'd continuously timeout in a non-shown (hidden tab) > terminal. So I'm ok with simplifying this not to remove the timeout in > invalidate_all(). There's no periodic timer in this game, just a sequence of one-shot timers. It's drawing that installs a new one if necessary. There's no explicit "stop" moment when there's no longer text to be blinked, it's just that implicitly the timer is not reinstalled. (This design idea occurred to me in the first demo patch in comment 17.) When a tab is obscured, GTK+ doesn't call widget_draw so there's no chance of installing a new timer. But please do double check that there's indeed no activity (e.g. strace'ing g-t-s).
(In reply to Egmont Koblinger from comment #54) > (In reply to Christian Persch from comment #52) > > > However, since we already walk the line in ::draw_rows(), could just do that > > work there instead in ::draw_cells() (which then wouldn't need to be called > > for cells that blink but are currently invisible dues to currently being in > > 'off' in the blinking cycle.) > > A while ago in comment 29 I mentioned that I don't like invisible and > blinking handled elsewhere. I'm no longer sure about that. Invisible, and > blinking that happens to be off right now are different in one very > important aspect: The latter one has to trigger a forthcoming redrawing. Thinking about it a bit more: You mentioned something along the lines that you wanted to rework drawing so that there's no tons of separate booleans for all attributes, but rather a bitmap or struct containing all of them. That would of course be a nice change. Once done (and as a result, the invisible attribute is also passed to draw_cells), I'd remove all special handling of invisible from draw_rows and friends (it's a way too rare use case to deserve any extra code doing some optimization), and just simply handle it in draw_cells exactly where blinking's off phase is handled, using an early return.
Created attachment 365899 [details] [review] vte, v6.1 (incremental) (In reply to Egmont Koblinger from comment #69) > In invalidate_all() I remove the > timer with a comment /* this sometimes saves us a full repaint after > blinking has stopped, otherwise not important to have here */, but I'm > tempted to delete this "optimization". [...] > > The only place where we must (obviously) remove the timer is when the widget > is destroyed. Everywhere else it should be harmless to just leave it in > place. This wasn't exactly true. When blinking frequency is modified, we need to remove a previously installed timer because it might fire too late, to leave room for installing a new one that fires at the newly desired time. See the proposed patch for this (and some additional minor cleanup / comments).
Comment on attachment 365729 [details] [review] g-t, v5 (With the changes as discussed.)
Submitted. Closing.
Thank you! :-)
You're welcome! I'm glad that finally we could find the design that we both like, and is simple, clean, maintainable. Even though I got really stuck for a long while at some point, after all it was a really fun experience! :-)
Finally I found a cool use for blinking text :-) curl wttr.in/Some_Stormy_Location The thunder sign is blinking.