GNOME Bugzilla – Bug 160450
Make the window menu use a menu bar
Last modified: 2005-01-04 14:31:25 UTC
Here is a patch that make the window menu applet use a GtkMenuBar instead of its own thing. Among other things this will make it play nicer with themes. The patch also contains a fix to remove the separator if it would be at the end of the menu. The patch is actually a net win in terms of code size.
Created attachment 34485 [details] [review] the patch
How funny! I worked on this yesterday and was blocked. It seems your filter_button_press is what I was missing. I'll look at the patch right now.
Comment on attachment 34485 [details] [review] the patch There are some changes I'll made before committing, but there's one problem left. I had the same one and couldn't find how to solve it: there's no focus line around the applet when you're navigating in the panel. We need to have one without breaking Fitt's law.
*** Bug 111026 has been marked as a duplicate of this bug. ***
It should be possible to connect_after to the menu bar expose signal and draw the focus line there. This combined with the GtkMenuItem::horizontal_padding style property should make this possible.
I've just tried the patch, and it works wonders. Thanks guys. Minor nitpick : don't forget to follow the panel background in the same way as all the other applets do now (eg like the recent change to the menu bar applet). Otherwise, it's perfect.
Any news on this ?
I'd like to have the focus line before committing the patch. I wanted to work on it, but this was not top priority.
By the way, there doesn't seem to be a focus line on the menubar applet either... Even if I guess it's not as important because you can access it with Ctrl-F1.
It's on my TODO list too :-)
Created attachment 35088 [details] [review] same patch with focus line Here is an updated patch - it does the same thing but adds the focus line, following the technique explained in comment 5.
Created attachment 35092 [details] [review] Same patch with focus line and translucency And finally... The same patch, with all of the above + translucency and pixmap background support restored.
Actually the patch seems to have issues : the applet is not properly updated anymore when you switch windows, etc. This is weird. Investigating.
Created attachment 35093 [details] [review] another patch The previous patch was calling gtk_widget_queue_draw_area to get rid of the focus line when the focus was moved to another widget. This triggered an infinite loop of expose-events. This patch looks and works as intended. Sorry for the noise :)
Some random comments (not a review by any standard): - It looks like the style that is copied in PANEL_PIXMAP_BG is leaked (I could be wrong though). - The "need_redraw" thing going on looks highly dubious. It is illegal to queue a redraw out of an expose handler - that will trigger infinite loops. The right approach would be calling gtk_widget_queue_draw() when the focus changes (both in and out), then in the expose handler just draw the focus line if the widget HAS_FOCUS. Generally, the gtk+ drawing model is (a) Call gtk_widget_queue_draw() to schedule an area for redraw. (b) This will cause gtk+ to generate a call to the expose handler(s) for the area. So queueing a draw out of an expose handler will cause infinite loops.
Thanks for the comments, and for the approach tip - I knew the need_redraw thing was smelling like bad code, but I couldn't see any other approach at the time. For the style leak, the patch did not actually change anything in that regard - I just applied the style to the menu bar instead of applying it to the applet itself. This technique is the same one used in the clock applet, weather applet, trash applet, etc - if there indeed is a leak, we should fix it everywhere then. The docs do not help much - there is no info about gtk_style_copy... Who would know about this ?
Created attachment 35132 [details] [review] another patch Another patch, following the approach outlined in comment #15 (connecting to the "focus-in" and "focus-out" events)
Actually you can just connect the "focus-in" and "focus-out" event to the gtk_widget_queue_draw function - there's no need for an intermediate function.
* Connecting focus-in/out directly to queue_draw() should be fine. It's illegal according to the C standard, but gtk+ itself relies on it working. * It still looks to me like the created styles need to be g_object_unref()ed. Maybe file a separate bug report. * In principle you can't draw on a *different* widget than the one the expose was emitted on. The code currently connects to the expose of the applet, then goes on to draw on the menu item. This means you might end up drawing on a window that isn't exposed, which can have various undesirable side-effects, such as flicker or artefacts. Instead, I'd connect to the menubar's expose handler, and then do the drawing on the menubar's window. Like this: if (GTK_WIDGET_HAS_FOCUS (widget)) gtk_paint_focus (window_menu->menu_bar->style, window_menu->menu_bar->window, GTK_WIDGET_STATE (window_menu->menu_item), NULL, window_menu->menu_bar, "menu-applet", 0, 0, -1, -1); Passing 0, 0, -1, -1 means basically "use the whole window". I'd also call the expose function something like "on_menu_bar_expose()" to make it clear when it will run. It's fine to keep invalidating the applet (as opposed to the menubar) in the focus signal handlers, because gtk_widget_queue_draw() invalidates all subwidgets (including in this case the menubar). Other than those things, I don't see any problems with the patch. (But the panel maintainers will need to review it).
Thanks Søren for reviewing this part of the patch :-) Small comments: * do not do this: +static gboolean +window_menu_focus_event (GtkWidget *widget, GdkEventFocus *event, gpointer data) but this (with tabs and not space): +static gboolean +window_menu_focus_event (GtkWidget *widget, + GdkEventFocus *event, + gpointer data) * remove "GtkWidget *frame;" since it's been useless for a while * does color background work? * "gnome-panel-window-menu-menu-bar-style" seems a bit long, but I don't have any better proposition ;-) (btw, stricly speaking, it should be window-selector and not window-menu...)
Sorry for the spam. Confirming some bugs. Filter on "VINCENT CONFIRMS" to ignore.
Created attachment 35166 [details] [review] and one more patch I'm bound to get it right eventually... Thanks guys for all the pointers. In this new patch, all your comments should be included. Søren : I have opened bug #162093 for the gtk_style leak. Vincent : All types of background are working (plain, color and pixmap).
Comment on attachment 35166 [details] [review] and one more patch Thanks Vincent. Please commit to HEAD.
Vincent: if you have time to cook a patch for the menu bar, you'll be my first hero of the new year :-)
I will commit the patch sometime tomorrow if nobody beats me to it (I don't have access to my usual box right now). In the meantime, here is a patch to add a focus line to the menu bar. AFAICS, it works :)
Created attachment 35405 [details] [review] patch to add a focus line to the menu bar applet This patch adds a focus line around the menu bar applet when it is focused using the keyboard. I didn't try to focus the menu bar items seperately because when these are focused they are selected anyway.
Comment on attachment 35405 [details] [review] patch to add a focus line to the menu bar applet Ok to commit this one too if you tested it.
Well I tested it and it works well. The only problem I found is that if you navigate the applets using the arrow keys, once the menu bar is focused hitting any arrow key selects one of the menu items in the menu bar, and you cannot leave the menu bar with the arrows. The tab key works though, and lets you leave the menu bar and cycle through the applets...
Both patches are now committed. Thanks to all. I'm closing the bug, please feel free to re-open if something is wrong with the fix.