The seconds update in the tooltip may noticeably lag the seconds update in the panel. Reproduce as follows: Configure a vertical panel (this is not essential but makes the problem easier to see). Add two instances of the datetime plugin, one above the other. Configure the upper instance to display "Date, then time", with a time format that displays seconds. Configure the lower instance to display "Date only", with a time format that displays seconds. Place the cursor over the lower instance. A tooltip appears showing the current time and it updates noticeably later than the time in the upper instance. (If there is not a lag, open the properties dialog for the lower instance, wait for the upper instance to update, further wait slightly less than one second, click close.) Version is r4921.
Created attachment 1677 schedule tooltip update for next second or minute This patch fixes this bug. It schedules the tooltip update for the next second or minute using same algorithm as the update of the date/time displayed in the panel. It works with both one second and one minute update intervals. Patch is against trunk r4921.
Created attachment 1678 update tooltip along with plugin With patch 1677, if the update interval was changed from one minute to one second, the tooltip did not update until the one-minute timer expired. My first idea was to cancel the tooltip timer when the date/time format changed. After looking closer, ISTM that the existing timer can update *both* the tooltip and the plugin. So this patch fixes this bug and removes the separate tooltip timer. (The function datetime_wake_interval_from_current_time() isn't really needed now, since it is called only once.)
I think I overlooked something when I was implementing tooltips that you're now taking advantage of. Suppose the tooltip has a seconds display and the panel just shows the date. I wanted the panel to update every minute, and then only have a timer every second while the tooltip is active. That would keep wakeups down to a reasonable quantity.
(In reply to comment #3) > Suppose the tooltip has a seconds display and the panel just shows the date. I > wanted the panel to update every minute, and then only have a timer every > second while the tooltip is active. Good point. Even though the seconds are not displayed, the plugin would be getting updated every second. Would it be sufficient to pre-compute *two* update intervals (when the properties change) and dynamically select one depending on whether the tooltip is currently being displayed or not?
> Would it be sufficient to pre-compute *two* update intervals (when the > properties change) and dynamically select one depending on whether the tooltip > is currently being displayed or not? Yeah, I think that'd be best.
Created attachment 1681 bug fix; implement two update intervals This patch fixes this bug and implements separate update intervals for the button labels and the tooltip. I tried and failed to implement a single-timer design, so I now fully appreciate the merits of Diego's double-timer design and have retained it in this patch. :-) I added two DBG() lines to datetime_query_tooltip() so I could better understand when the "query-tooltip" event was triggered. They could probably be removed. This patch also moves the installation of the "query-tooltip" handler to the plugin initialization because it does not change during the lifetime of the plugin. I was getting a GLib assertion failure when restarting the panel. It seemed to occur in datetime_free() if the tooltip timer had never been started. The tooltip_timeout_id would be 0 in that case. $ xfce4-panel -r (xfce4-panel:8979): GLib-CRITICAL **: g_source_remove: assertion `tag > 0' failed ** Message: Restarting xfce4-panel... DBG[datetime.c:726] datetime_new(): Starting datetime panel plugin
(In reply to comment #6) > This patch also moves the installation of the "query-tooltip" handler to the > plugin initialization because it does not change during the lifetime of the > plugin. This is not such a good idea after all, because g_signal_connect() ultimately allocates a small amount of memory with "g_slice_new (Handler)". (I was assuming it simply installed a function pointer.) http://svn.gnome.org/viewvc/glib/trunk/gobject/gsignal.c?view=markup
Finally looked at the handy macros in /usr/include/glib-2.0/glib/gmessages.h. g_return_val_if_reached() writes a message to stderr. --- trunk/panel-plugin/datetime.c 2008-06-08 19:17:04.000000000 -0700 +++ exp4/panel-plugin/datetime.c 2008-06-13 15:08:07.000000000 -0700 @@ -195,12 +195,11 @@ format = datetime->time_format; break; default: + /* Tooltips are disabled in this case, so it should not occur. */ + g_return_val_if_reached(FALSE); break; } - if (format == NULL) - return FALSE; - g_get_current_time(&timeval); current = localtime((time_t *)&timeval.tv_sec);
Created attachment 1697 restore timer cancellation so panel does not crash after plugin is removed During testing of the previous patch, the panel was crashing after the plugin was removed from the panel. The core file showed that datetime_update1() was being called with a corrupt datetime structure. The problem was an orphan timer calling its timeout handler after the datetime structure had been freed (with a one minute update interval, the crash could occur nearly a minute after the removal). The fix is to restore the code to cancel the timer each time the handler is called (in particular, after the properties change). This patch also adds another DBG() call to the "query-tooltip" handler to show the current system time each time the handler is called. This makes it possible to distinguish successive events in the debug output. (unlike, the DBG("enter") messages, which all look alike.) It also makes previously suggested changes.
I am still alive - I was just out of town for the weekend (extended), and now I'm trying to get caught up with everything. I'll review the patch in the next few days.
I fixed the "seconds update in tooltip lags seconds update in panel" using part of this patch in svn r4995. I also applied other parts of the patch in r4993 and r4994. I kept the tooltip timeout at 1000ms regardless of what it's displaying. I think it's perfectly reasonable to wake at that rate while the mouse is hovering there. So, I think I've mined the meatier bits of this patch. I'll leave the bug open until I get some feedback from you, Steve.
(In reply to comment #11) > I fixed the "seconds update in tooltip lags seconds update in panel" ... Thanks! The tooltip and button updates are crisply synced with each other and with my radio-controlled clock. (Verified with the test case above and an additional one with the times displaying minutes only.) Your fix looks good. I believe this bug is resolved. > I kept the tooltip timeout at 1000ms regardless of what it's displaying. I > think it's perfectly reasonable to wake at that rate while the mouse is > hovering there. OK. The only rationale I can think of for applying the one-minute update interval optimization to the tooltip display is that a laptop user could unintentionally leave the cursor over the plugin and unknowingly consume more battery power than would be suggested by the displayed tooltip. > So, I think I've mined the meatier bits of this patch. I'll leave the bug open > until I get some feedback from you, Steve. Glad you were able to use some of my patch.
> OK. The only rationale I can think of for applying the one-minute update > interval optimization to the tooltip display is that a laptop user could > unintentionally leave the cursor over the plugin and unknowingly consume more > battery power than would be suggested by the displayed tooltip. Yeah, I considered that, but I'm willing to make the compromise.