Re: [patch v3] showing menu bar temporarily when f10 is pressed

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

>>> Why do you need the menu-bar?  Why not pop up the menu usually bound to
>>> C-mouse-3?
>> To be honest, I didn't know that.
>> I've just tried C-mouse-3; and found some advantages of C-mouse-3
>> comparing with f10 for KEYBOARD USER.

While I agree we should also support the f10 behavior (so your current
patch is acceptable), I think the problem you point out for the
C-mouse-3 behavior could/should be fixed.

>> 1. With C-mouse-3, the menu is pop up where mouse pointer is.

That should be easy to fix so that the menu is popped up at point.

>> 2. With C-mouse-3, the contents of menu is chosen based
>> on the window where the mouse pointer is.

Same here.

Some comments on your code:

> +(defcustom x-menu-bar-temorary-visibility t
> +  "Control whether use menu bar or `tmm-menubar' when `x-menu-bar-open' is invoked.
> +Non-nil means showing menu bar temporarily even if `menu-bar-mode' is turned off.
> +If nil, the behavior is changed whether `menu-bar-mode' is on or off. See `x-menu-bar-open'
> +for more detail."
> +  :type 'boolean
> +  :group 'x
> +  :version "24.2")

Please stay within 80 columns and use two spaces to separate sentences.

> +(defun x-accelerate-menu-with-temporary-visible-menu-bar (frame)

Please only use "x-" for functions that are specific to X11.

> +		       (when (or (frame-live-p old-frame)
> +				 ;; Handle the case that if menu-bar-mode is
> +				 ;; turned on/off with choosing a menu item for controling the mode.
> +				 (or (eq old-menu-bar-mode menu-bar-mode)
> +				     (not menu-bar-mode)))

I don't understand this condition:
- shouldn't the outer `or' be an `and'?
- why check (not menu-bar-mode)?

> +	(prog1
> +	    (accelerate-menu frame)
> +	  (add-hook 'deactivate-menubar-hook hide))))))

Why put the add-hook after rather than before calling accelerate-menu?
If it can't come before, then the prog1 needs to be replaced by an
unwind-protect in case accelerate-menu signals an error.

>  (defun x-menu-bar-open (&optional frame)
> -  "Open the menu bar if `menu-bar-mode' is on, otherwise call `tmm-menubar'."
> +  "Open the menu bar if `menu-bar-mode' is on or `x-menu-bar-temorary-visibility' is non-nil, otherwise call `tmm-menubar'."

This docstring's first line is too long.  It has to be less than 80
columns and it has to "stand on its own" (you can't just line-wrap it)
since modes like apropos will only display the first line.

>    (interactive "i")
> -  (if (and menu-bar-mode
> -	   (fboundp 'accelerate-menu))
> -      (accelerate-menu frame)
> +  (if (fboundp 'accelerate-menu)
> +      (cond
> +       (x-menu-bar-temorary-visibility
> +	(x-accelerate-menu-with-temporary-visible-menu-bar frame))
> +       (menu-bar-mode
> +	(accelerate-menu frame))

To be on the safer side, I recommend you only use
x-accelerate-menu-with-temporary-visible-menu-bar when accelerate-menu
won't work (e.g. when menu-bar-mode is nil).

>  popup_deactivate_callback (GtkWidget *widget, gpointer client_data)
>  {
> +  safe_run_hooks (Qdeactivate_menubar_hook);
>    popup_activated_flag = 0;
>  }

Could someone more knowledgeable tell me if this is safe?
I.e. is popup_deactivate_callback called asynchronously (e.g. from
a separate thread or from a signal handler)?
If not, then we'll need to delay running deactivate-menubar-hook to
a safe moment (maybe export popup_activated_flag to Lisp and then use
a pre/post-command-hook that checks this var before running
deactivate-menubar-hook).


        Stefan