GNOME Bugzilla – Bug 99886
GNOME needs a visual bell
Last modified: 2004-12-22 21:47:04 UTC
For accessibility by people with hearing impairment, and to avoid annoying other users when keyboard accessibility "bell/beep" notifications are in use, GNOME needs a 'visual bell'. A visual bell feature causes visual notification alongside, or in place of, the system "beep" and XkbBell/XBell features. HP and I discussed this and it seems logical to put this feature in Metacity, since Metacity already listens to a number of system-wide X events and handles some desktop auxiliary visuals and frame visuals. In particular, if the visual bell implementations we offer include window-frame flashing or similar changes in the window visuals, Metacity is the logical place to put the code. I attach a patch which provides the following features: "visual-bell" on/off "audible-bell" on/off (regardless of the state of the 'visual-bell') "visual-bell-type", which indicates the way in which visual bell feedback is provided. The patch provides currently only provides one visual bell feedback type, "fullscreen", which flashes the X display. Note that the patch is under development, it currently flashes the default screen, whereas it should probably flash one or all of the Metacity-managed screens specifically, rather than reverting to the default screen. The patch also provides a stub for "frame_flash" feedback, the implementation of which awaits advice from Havoc.
Created attachment 12635 [details] [review] proposed patch to provide a simple visual-bell feature
Thanks for the patch, some comments: - the flash window should be per-screen and stored in the MetaScreen struct instead of global IMO - metacity doesn't normally read /desktop/gnome/interface settings because in principle any WM can work correctly with GNOME. If the setting is /desktop/gnome/interface we would generally propagate it to an X property in gnome-settings-daemon then metacity would pick it up. If the setting is in /apps/metacity then the control center has a virtualized window manager object that it would modify that setting via. - to avoid DefaultScreen(), if we have a window field or are guessing at the window field using the focused window, I would suggest we use the screen for that window. If we don't have a window field, I would suggest we flash all screens. - I think right now the flash is "as fast as the X server can do it" - I wonder if we shouldn't have some timeout in there to be sure it's at least N milliseconds long or something.
For flashing the window frame, here is one approach: - change meta_frame_get_flags() to return a new flag META_FRAME_IS_FLASHING or something when the flame is in the "flashed" state - change theme.c:theme_get_style() to return a different style while flashing - enter the flash state, draw the frame, wait a timeout, leave flash state, redraw the frame. Something along those lines.
thanks Havoc for your quick comments. Since the visual-bell (generally) is not metacity-specific I would prefer the approach you suggest for reading the setting in g-s-d and propagating an XSetting to moving those two gconf keys. I am not sure we need a timeout for the flash since I am calling XSync. I suppose there might be some broken servers out there that might collapse these calls into a vertical retrace or something but it seems broken not to wait a retrace interval between XSync() calls at least, and as long as the repaint actually makes it to the frame buffer I think it's fine. If we make a significant timeout it could be a bother for multiple bells in a row (i.e. cat of a non-ascii file, etc.) But I will think about it. Thanks for your suggestions about frame flash, I will try it; and I agree with you regarding strategy as to which screens to flash and where to store the flash_window. Will revise the patch soon.
just letting you know I haven't forgotten this bug... Havoc, I hope to have time to implement your suggestions this weekend.
Created attachment 12859 [details] [review] new version of patch, provides 2 vis-bell types, incorporates hp's suggestions.
I just attached a new version of the patch. It fixes the DefaultScreen stuff, provides a screen-based fullscreen flash, and flashes all metacity-managed screens on fullscreen-flash. It also implements the titlebar flashing in the way hp recommends; it flashes the titlebar of the bell's originating window if known, otherwise the currently focussed window (according to display->focus_window, that is), if it has a frame; otherwise it, too, falls back to fullscreen mode. At the moment the patch still listens to gconf directly for the boolean properties visual_bell and audible_bell, though there's partial implementation for the xsettings approach; I want to discuss the latter a little more before patching g-s-d and possibly gdk, to make sure I am doing it right. The current patch might also be acceptable as an interim solution if it's not desirable to add these xsettings right away.
Created attachment 12860 [details] [review] cleaned version of previous patch (removed partial non-gconf settings code)
havoc, I need feedback on this since we really think this is needed for 2.2.
Comments, if you clean this stuff up I'll do a final pass and check it in: - there are CVS conflict markers in configure.in - I'd like to move most of the code for this to a bell.[hc] instead of display.c/screen.c just to avoid giant files from hell. For the bell.h interface I would envision something like: meta_display_init_bell (MetaDisplay*); meta_display_shutdown_bell (MetaDisplay*); meta_display_process_bell_event (MetaDisplay*, XEvent*); - xkb_base_event_type needs to be in the display object, not a global - some indentation looks wrong, e.g.: +#ifdef HAVE_XKB + if (!XkbQueryExtension (xdisplay, &xkb_opcode, &xkb_base_event_type, + &xkb_base_error_type, NULL, NULL)) + { + xkb_base_event_type = -1; + g_message ("could not find XKB extension."); + } + else In emacs, "M-x c-set-style RET gnu RET" then select region and "M-x indent-region" - Where does XKB_SET_AUTO_RESET_IS_WORKING get defined? Is that an official part of the Xkb API? Just checking, it seems like such a lame API ;-) - I'd take out the "use PseudoColor visual" code until we're actually doing it for a reason (actually doing the colormap games, etc. for speed) - In visual_bell_flash_fullscreen() should handle screen_for_xwindow returning NULL, not sure it ever does right now but I think I usually assume it can - I don't like the /desktop/gnome gconf keys, I think they should be /apps/metacity, to preserve modularity. If we do a global setting it should be sorted on wm-spec-list. - visual_bell_type_from_string() should not use strncmp, right now it will accept a value with trailing junk as a valid value - can't include display.h in prefs.c, that violates the model-view architecture. prefs.[hc] is the model, display is the view. Instead, in display.c:prefs_changed_callback() you would update the bell settings. - meta_prefs_get_visual_bell_type() should never return the INVALID bell type, always something usable. - window.h has a random whitespace change
Havoc: I think the pseudocolor code is actually working; the issue is that at the moment we are assuming that the first 8 bit pseudocolor visual is an overlay. That's true on some architectures but not all. I can comment it out but I'd prefer not to lose the code altogether. otherwise I agree with the changes, except for the visual-bell gconf key location. GNOME is already using a number of non-standardized /desktop/gnome/interface keys, so I don't see why we can't add another while standards are being discussed. However I will go ahead and put in in /apps/metacity for the moment (under protest ;-).
BTW, the #ifdef XKB_SET_AUTO_RESET_IS_WORKING bracketing is a bug workaround, XFree's current XKB implementation of XkbSetAutoResetControls seems hopelessly broken. We'd really like to call this API, but the side effects are too nasty at the moment, so I've bracketed it out with what I took to be ab obvious reference to why it was #ifdeffed away.
If the pseudocolor thing is actually doing something useful, it's fine to leave it. Maybe change the comment to explain that we're assuming it's an overlay. The #ifdef XKB_SET_AUTO_RESET_IS_WORKING is hard; hmm. Is it the client library or the X server that's broken? Any way we can detect it, in configure.in for library or at runtime for the server? Otherwise it'll be hard to ever enable this option.
I took the Pseudocolor code but left the comment explaining what we want to do (i.e. figure out if there's an overlay visual available). I assume the AutoReset bug is version-dependent, so eventually we can do a runtime check at the same time we get the XKB base event type. I can easily use a state variable (in the display) instead of using #ifdef.
hp, I need clarification on one comment. I have changed to patch to move all the implementation stuff from display.c to bell.c, and some of the prefs stuff. Did you want to move all the bell stuff out of prefs.c as well? (That would different from the way we handle all other prefs). At the moment I have three bell-related statics in prefs.c, and also: visual_bell_type_from_string() update_visual_bell() I could easily move the first one to bell.c/h (at the cost of making them public), but moving the update_ func would break with the prefs.c pattern. Which way do you want it? -Bill
Created attachment 13045 [details] [review] latest patch - complete AFAICT
I think this version of the patch covers the bases. I don't see any more spurious whitespace, though the patch adds a couple of single lines of buffering whitespace which I think are appropriate. OK to commit as-is, or with specific revisions?
The only problem I see is that a window may be destroyed while the flame unflash idle handler is still pending, resulting in unflashing a destroyed frame. The idle handler should probably be stored in MetaDisplay (not per-window, as increasing size of MetaWindow is a size increase * N windows), and removed when the flash-pending window goes away. Or alternatively, have a global variable for a list of windows to unflash, have the idle run over the list, and when a window goes away be sure it's not in the list. I'll fix this if you want to go ahead and commit.
I don't follow the point about per-window, since there's no added per-window data and only one frame-flash idle handler (per screen) is pending per bell event. Why not just export an api meta_bell_notify_frame_destroy (), and call it when a frame is being destroyed? Then we can remove the idle func cleanly on notify (I notice there seems to be no destroy notification for frames ATM).
I ended up using meta_bell_notify_destroy_frame() [called in frame.c, in meta_window_destroy_frame ] and checking frame->is_flashing to see if I need to call g_idle_remove_by_data. That's simple and clean, and solves the problem of dangling timeouts. Feel free to bin that if you want to do it differently. I also turn the audible bell back on at metacity exit, which is better than nothing until the AutoReset stuff gets fixed. Thanks Havoc!
reopening since release team has asked for reversion. I attach also a patch (_not_ a -R patch) that reverts the feature, which I will apply if as I expect the RT does not change its opinion today.
Created attachment 13093 [details] [review] patch to REVERT visual-bell (as committed). Not a -R patch.
When we re-apply this patch, update_visual_bell_type needs to be fixed to accept NULL in case the key is unset. Reverted in CVS now.
Havoc: Is it time to reapply this patch to HEAD now? BTW I changed visual_bell_type_from_string to accept a NULL string (it reverts to the default visual bell type, i.e. fullscreen flash). I have recreated the patch which I will attach.
Created attachment 13883 [details] [review] new version of patch, against CVS HEAD of jan 28. Virtually same as old patch.
Yes, it's OK to put this back on HEAD, thanks.
applied to HEAD. Should we consider backporting to gnome-2.2.x ?
Features aren't allowed on the stable branch, though if vendors want to backport it in their packages I would not be upset. There's a good reason features aren't allowed, we want people to track HEAD and contribute to HEAD. Past experience demonstrates the importance of this - if you don't, stable just becomes de facto HEAD, and isn't stable.
havoc: I have to say this is a rock/hard-place argument. I tried to be a good citizen w.r.t gnome-2.2.0, but we really DO need this feature before GNOME 2.4. Considering the number of features that are still slipping into gnome-2-2 (i.e. new themes, etc!) I think it's unreasonable to wait with this feature until gnome 2.4 before there's a stable release of it. I think you have every right to maintain your own stability/feature policies, and release numbers (as you do!) in Metacity, but I would ask you to take this into consideration before rejecting a backport to a gnome-2-2-1 or 2-2-2 tagged Metacity branch. Selected features should, IMO, go into stable branches, especially if they are self-contained and the next stable branch is far off.
I'm not talking about metacity policy here, it's a GNOME-wide policy. The next stable release is *not* far off; it's 6 months off. The reason it's 6 months off is that we're going to work on it, and not work on the current stable branch. Trust me, we have tried your way in the past. GNOME 1.4 lived for ages, with unstable feature creep in ever point release, and GNOME 2 never got worked on. Never again should that happen. It screwed the stable branch users because stable wasn't, and it screwed the unstable branch because it was never released. Vendors are welcome to do backports, which should be easy since the difference between major releases is only 6 months worth of stuff, but we should not take those backports in CVS, because we need to keep up the motivation for people to work on HEAD. There have been *rare* exceptions such as menu editing, but even adding menu editing turned out to be a mistake - it was completely unstable and went in too soon. Anyhow, at some point this discussion needs to land on gnome-hackers, but I'm fairly sure the current policy is no features in stable branch. "we have to add features because unstable branch will never be released" is a self-fulfilling prophecy. Finish stable, leave it stable, move on.
Don't get me wrong: this isn't "my way", I think in general, having features in HEAD rather than stable is the right way to go. But I think that the current feature cycle is not working that well for features that impact multiple projects; and this particular feature fared badly last time. Realistically I think we're talking about closer to 9 mos. from the satisfactory completion of the feature (early Dec) to release of STABLE containing the feature. Meanwhile look at all the new stuff that rolled into 2.2 after "feature freeze" ! I realize that the latter is not your fault hp, but I am concerned about the loss of an important feature in the 2.2 stream. This doesn't reflect a general point of view of mine in regards to features.