bug#14180: PATCH Better fullscreen frame support on Windows

[Eli Zaretskii]

> Date: Thu, 11 Apr 2013 00:59:54 -0400
> From: Erik Charlebois <erikcharlebois@gmail.com>
> 
> This change improves the 'fullscreen' frame support on Windows. When
> going to fullscreen, the frame gets fullscreened with no window
> decorations (which causes the Windows task bar to also disappear) on
> the nearest monitor.

Thank you for doing this.

Jan, does the fullscreen functionality on X also maximizes the frame
to one of the monitors, when several monitors are available?  I don't
want to introduce differences in behavior here.

> The old fullscreen functionality was actually 'maximize' behavior so
> I've moved that to 'maximize'.

Sorry, I don't understand.  The value of the 'fullscreen' frame
parameter can be either 'maximize' or 'fullboth' (the latter also has
a confusing alias 'fullscreen').  The current trunk code handles both,
and it does so in accordance with the documentation (the last sentence
below):

  `fullscreen'
       Specify that width, height or both shall be maximized.  The value
       `fullwidth' specifies that width shall be as wide as possible.
       The value `fullheight' specifies that height shall be as tall as
       possible.  The value `fullboth' specifies that both the width and
       the height shall be set to the size of the screen.  The value
       `maximized' specifies that the frame shall be maximized.  The
       difference between `maximized' and `fullboth' is that the former
       can still be resized by dragging window manager decorations with
       the mouse, while the latter really covers the whole screen and
       does not allow resizing by mouse dragging.

It looks like your patch swaps the meaning of 'fullboth' and
'maximize', so that Emacs on MS-Windows will behave differently from
its behavior on other platforms.  Is my understanding correct?  If so,
please don't make that swap, it is wrong.

> 'fullwidth' and 'fullheight' are now ignored because they were buggy
> (at least in Windows 8, they were not being sized correctly) and are
> not common Windows idioms.

Please don't ignore these two values.  They do work reasonably well on
XP and on Windows 7.  If they are buggy and/or don't work on Windows
8, let's fix them if we can, and leave them as they are if we can't
fix them.  Removing them is too drastic, IMO; I see no justification
for that.

> While they could be supported, I'd do it as follow on work and get
> it working right on multiple monitors as well.

Thanks, but in the meantime please don't remove the current code that
attempts to support these two values.

> I've modified the response to WM_WINDOWPOSCHANGED to do nothing when
> the frame is fullscreen. Otherwise, the size gets tuned to a
> multiple of the character cells and the fullscreen isn't perfectly
> matching the screen. When this happens, Windows' special behavior to
> hide the taskbar doesn't kick in.

During debugging the new code, I saw that w32_fullscreen_hook is
called with f->want_fullscreen set to the value FULLSCREEN_WAIT; I'm
not sure your code expects that value and/or handles it correctly.
Please double-check that.

> (I signed the FSF copyright assignment papers in January so I'm ok on that
> front.)

Unfortunately, the FSF records still don't reflect your assignment, as
of this writing.  Could you please write to the FSF clerk with whom
you were in touch, and ask him to expedite the update?  Thanks.

A few comments about your patch:

> 2013-04-11  Erik Charlebois  <erikcharlebois@gmail.com>
> 
> * src/w32fns.c (monitor_from_window_fn): New Windows API import.
> (w32_monitor_rect): New function to get nearest monitor rect.
> * src/w32term.c (w32_fullscreen_hook): Renamed from w32fullscreen_hook
> for consistency. Rewritten for improved fullscreen support.

Please leave 2 spaces after a period that ends a sentence (we use the
US conventions in ChangeLogs and in the docs.)

Also, please don't rename w32fullscreen_hook, I see no reason for
consistency in naming functions that are private to the w32 build.

> +void
> +w32_monitor_rect (struct frame *f, RECT *rect)
> +{
> +#ifdef HAVE_WINDOW_SYSTEM
> +  if (f)
> +    {
> +      /* If multiple monitor support is available, make the window
> +         fullscreen on the appropriate screen. */
                                                 ^
Two spaces here.

Also, is there really a reason for the ifdef?  I don't think this code
could possibly be called in a -nw session, could it?