bug#22000: Patch addressing the menu-bar frame-resize interaction

[Eli Zaretskii]

> Date: Thu, 18 Oct 2018 13:23:26 +0100 (BST)
> From: Vivek Dasmohapatra <vivek@etla.org>
> cc: 22000@debbugs.gnu.org, David Engster <deng@randomsample.de>, 
>     "eliz@gnu.org" <eliz@gnu.org>
> 
> +/* Sets up the menubar style for scrolling/non-scrolling modes.
> +   Reparents the menubar directly into the vbox for non-scrolling
> +   mode and adds a scrolledwindow+viewport for scrolling modes.  */

This commentary is n ot in the style we (and you elsewhere in the
patch) use: you should say "Set up", not "Sets up".

> +#if GTK_CHECK_VERSION (3, 8, 0)
> +      gtk_container_add (GTK_CONTAINER (x->menubar_viewport), 
> x->menubar_widget);
> +#else
> +      gtk_scrolled_window_add_with_viewport (GTK_SCROLLED_WINDOW 
> (x->menubar_viewport), x->menubar_widget);

Please break long lines into several shorter ones.

> +#if GTK_CHECK_VERSION(3, 16, 0)
> +  /* Always want the scrollable container for capable-enough GTK versions */
> +  menubar_scrollbox (f, 1);

Comments should be complete sentences, start with a capital letter,
and end in a period and 2 spaces.

> +@itemize
> +@item @code{always} - Scrollbar is present, menu bar scrolls when too wide.
> +@item @code{automatic} - Scrollbar appears when menubar grows too wide.
> +@item @code{forced-resize} - No scrollbar.  Growing menubar forces a frame 
> resize.

This is not the correct way of using @itemize.  You should do this
instead:

  @itemize
  @item
  @code{always} - Scrollbar is present, menu bar scrolls when too wide.
  @item
  @code{automatic} - Scrollbar appears when menubar grows too wide.

etc.  (Actually, I'd suggest using "@table @code" here, not @itemize.)

Also, please use "--" for an en-dash, not a single "-".

> +(defcustom menu-bar-scrollbar-mode nil
> +      "Specify how GTK menu bars deal with the frame being too narrow to 
> hold them.\n
> +Valid values are:
> +  `always'        - the menu bar always has a scrollbar
> +  `automatic'     - a scrollbar is added when required
> +  `forced-resize' - no scrollbar, the frame is forced to resize to 
> accommodate
> +                    the menu bar.
> +   nil (or any other value) - the menu bar is truncated\n
> +Note that prior to GTK 3.16 truncation is not possible and the default
> +is equivalent to 'forced-resize.\n

Do you really need these explicit \n newlines?

> +(defun menu-bar-scrollbar-mode (&optional mode)
> +  "Cycle through scroll/truncate/resize modes for GTK menu bars.\n
> +If the optional parameter MODE is specified then apply that instead.
> +The new mode is stored in the variable `menu-bar-scrollbar-mode' via
> +the custom interface (but not automatically saved).\n
> +Returns the new MODE.\n
> +NOTE: pass 'default if you want to set the mode explicitly to nil.\n

Likewise here.

More generally, shouldn't this mode have "gtk" somewhere in the name?
Or, if we hope to implement it for other toolkits at some future date,
the doc string should not say "GTK menu bars", it should say "menu
bars" and then have a note that this currently has effect only with
GTK menus.

> +(defun menu-bar-showhide-menu-bar-scrollbar-mode-customize-forced-resize ()
> +  "Resize the frame to accommodate the menu bar."
> +  (interactive)
> +  (customize-set-variable 'menu-bar-scrollbar-mode 'forced-resize))
> +(defun menu-bar-showhide-menu-bar-scrollbar-mode-customize-always ()
> +  "Add a permanent scrollbar to the menu bar."
> +  (interactive)
> +  (customize-set-variable 'menu-bar-scrollbar-mode 'always))
> +(defun menu-bar-showhide-menu-bar-scrollbar-mode-customize-automatic ()
> +  "Add a scrollbar to the menu bar when it tries to grow past the frame 
> edge.."
> +  (interactive)
> +  (customize-set-variable 'menu-bar-scrollbar-mode 'automatic))
> +(defun menu-bar-showhide-menu-bar-scrollbar-mode-customize-nil ()
> +  "Truncate the menu bar to fit the frame."
> +  (interactive)
> +  (customize-set-variable 'menu-bar-scrollbar-mode 'default))

I think doc strings of these functions are too laconic for interactive
functions.

Thanks.