Re: [2/2 v3] winex11.drv: Implement FlashWindow with _NET_WM_STATE_DEMANDS_ATTENTION.

[Ken Thomases]

On Sep 30, 2015, at 2:53 AM, Jactry Zeng <jzeng(a)codeweavers.com> wrote:
> dlls/user32/driver.c              | 16 ++++++++++++++--
> dlls/user32/user_private.h        |  1 +
> dlls/user32/win.c                 |  1 +
> dlls/winex11.drv/window.c         | 29 +++++++++++++++++++++++++++++
> dlls/winex11.drv/winex11.drv.spec |  1 +
> dlls/winex11.drv/x11drv.h         |  1 +
> dlls/winex11.drv/x11drv_main.c    |  1 +
> 7 files changed, 48 insertions(+), 2 deletions(-)

Since FlashWindowEx() is the more general function — that is, FlashWindow() could be fully implemented in terms of FlashWindowEx(), but not the other way around — shouldn't the driver entry point be FlashWindowEx()?  Otherwise, when FlashWindowEx() is eventually implemented more completely, the driver entry will have to be changed.

For now, since user32 implements FlashWindow() but FlashWindowEx() is a stub (or semi-stub after your next patch), FlashWindow() would have to create a FLASHWINFO structure, fill it in, and pass that along.  I think that dwFlags would be either FLASHW_ALL or FLASHW_STOP, depending on bInvert.  uCount would be 1, I guess.

Shouldn't pFlashWindow (or pFlashWindowEx) be added to the USER_DRIVER structure among the "windowing functions" (see the comments), in alphabetical order (after pDestroyWindow)?  Of course, that affects the changes to driver.c in terms of where things are added.  The order in winex11.drv.spec is less clear, but it seems to be mostly alphabetical after a certain point.  I'd put it after EnumClipboardFormats() in that list.

Why does FlashWindow() only call the driver entry point if the window is minimized?  Shouldn't the driver get the opportunity to act for a non-minimized window, too?

For the X11 implementation, shouldn't something potentially remove the _NET_WM_STATE_DEMANDS_ATTENTION state, presumably when bInvert is false (or when dwFlags doesn't include FLASHW_CAPTION or FLASHW_TRAY)?

-Ken

[2/2 v3] winex11.drv: Implement FlashWindow with _NET_WM_STATE_DEMANDS_ATTENTION.

[Jactry Zeng]

Hi Ken,
2015-09-30 17:05 GMT+08:00 Ken Thomases <ken(a)codeweavers.com>:
> Since FlashWindowEx() is the more general function — that is,

FlashWindow() could be fully implemented in terms of FlashWindowEx(), but
not the other way around — shouldn't the driver entry point be
FlashWindowEx()?  Otherwise, when FlashWindowEx() is eventually implemented
more completely, the driver entry will have to be changed.
> For now, since user32 implements FlashWindow() but FlashWindowEx() is a

stub (or semi-stub after your next patch), FlashWindow() would have to
create a FLASHWINFO structure, fill it in, and pass that along.  I think
that dwFlags would be either FLASHW_ALL or FLASHW_STOP, depending on
bInvert.  uCount would be 1, I guess.
>

Thanks for your review!

Yes, function() will be implemented by functionEx() commonly in Wine.
But I didn't find a perfect way to implement FlashWindowEx() and then
implement FlashWindow() with FlashWindowEx().
Because in X11, how many times will the window be flashed is depend on
design of window manager/desktop environment.(For example, in XFCE, taskbar
will always flashes when _NET_WM_STATE_DEMANDS_ATTENTION is sent until the
user active the window again; In Ubuntu Unity, it'll only be flashed one
time and then highlight taskbar until the user active the window again.)
And I don't think it's necessary (and any possible?) to follow Window's
design.
In this way, implementing uCount and other parameters may  be useless,
because all we need to do is sending _NET_WM_STATE_DEMANDS_ATTENTION to
WM/DE one time when FlashWindow()/FlashWindowEx() is called.

Similarly, Windows's FlashWindowEx() can decide which part it want to flash
- only flash taskbar or only flash
caption or flash both. But it'll depend on WM/DE's design in X11.

So I implemented FlashWindowEx() by calling FlashWindow() simply.
> Shouldn't pFlashWindow (or pFlashWindowEx) be added to the USER_DRIVER
> structure among the "windowing functions" (see the comments), in
> alphabetical order (after pDestroyWindow)?  Of course, that affects the
> changes to driver.c in terms of where things are added.  The order in
> winex11.drv.spec is less clear, but it seems to be mostly alphabetical
> after a certain point.  I'd put it after EnumClipboardFormats() in that
> list.
> Agree.
> Why does FlashWindow() only call the driver entry point if the window is

minimized?  Shouldn't the driver get the opportunity to act for a
non-minimized window, too?
>

Same as above, different WM/DE may have different UI effect, that makes it
hard to follow Windows's design.
In Windows, different UI effects will be showed when window is minimized
and non-minimized.
And in my opinion, it is a little noisy if USER_Driver->pFlashWindow( hWnd,
bInvert ) is called all the time.
So I'll prefer to only let the user notice new message when the window is
minimized (it may hard to be noticed when
it's minimized).
> For the X11 implementation, shouldn't something potentially remove the
> _NET_WM_STATE_DEMANDS_ATTENTION state, presumably when bInvert is false (or
> when dwFlags doesn't include FLASHW_CAPTION or FLASHW_TRAY)?
> You are right. It seems flashing will be stopped when bInvert is false.
> I'll improve it for this case.

Thanks again! :)

-- 
Regards,
Jactry Zeng

[2/2 v3] winex11.drv: Implement FlashWindow with _NET_WM_STATE_DEMANDS_ATTENTION.

[Ken Thomases]

Hi,

On Sep 30, 2015, at 9:04 AM, Jactry Zeng <jactry92(a)gmail.com> wrote:
> 2015-09-30 17:05 GMT+08:00 Ken Thomases <ken(a)codeweavers.com>:
> > Since FlashWindowEx() is the more general function — that is, FlashWindow() could be fully implemented in terms of FlashWindowEx(), but not the other way around — shouldn't the driver entry point be FlashWindowEx()?  Otherwise, when FlashWindowEx() is eventually implemented more completely, the driver entry will have to be changed.
> >
> > For now, since user32 implements FlashWindow() but FlashWindowEx() is a stub (or semi-stub after your next patch), FlashWindow() would have to create a FLASHWINFO structure, fill it in, and pass that along.  I think that dwFlags would be either FLASHW_ALL or FLASHW_STOP, depending on bInvert.  uCount would be 1, I guess.
> > Thanks for your review!

You're welcome!
> Yes, function() will be implemented by functionEx() commonly in Wine.
> But I didn't find a perfect way to implement FlashWindowEx() and then implement FlashWindow() with FlashWindowEx().

I wasn't suggesting that you necessarily implement FlashWindow() in terms of FlashWindowEx().  Just that you model the driver entry function after FlashWindowEx() instead of FlashWindow().  If the driver doesn't implement the full semantics of FlashWindowEx(), that's OK.  It's no worse than the semi-stub that FlashWindowEx() will be.
> Because in X11, how many times will the window be flashed is depend on design of window manager/desktop environment.(For example, in XFCE, taskbar will always flashes when _NET_WM_STATE_DEMANDS_ATTENTION is sent until the user active the window again; In Ubuntu Unity, it'll only be flashed one time and then highlight taskbar until the user active the window again.) And I don't think it's necessary (and any possible?) to follow Window's design.
> In this way, implementing uCount and other parameters may  be useless, because all we need to do is sending _NET_WM_STATE_DEMANDS_ATTENTION to WM/DE one time when FlashWindow()/FlashWindowEx() is called.
>
> Similarly, Windows's FlashWindowEx() can decide which part it want to flash - only flash taskbar or only flash 
> caption or flash both. But it'll depend on WM/DE's design in X11.

It's OK for the driver to implement just that subset of the functionality it's able to.  It's not OK for the interface of the driver to prevent the implementation of the full functionality.  Don't forget, there are other drivers than X11.  The Mac driver may have greater flexibility to more fully implement the functionality (or it may have less).  But if the entry point doesn't provide the information, there's no chance for it to do as much as it can.
> So I implemented FlashWindowEx() by calling FlashWindow() simply.

Again, the way the two functions are implemented at the user32 level is a different question than what the driver interface is.
> > Why does FlashWindow() only call the driver entry point if the window is minimized?  Shouldn't the driver get the opportunity to act for a non-minimized window, too?
>
> Same as above, different WM/DE may have different UI effect, that makes it hard to follow Windows's design.
> In Windows, different UI effects will be showed when window is minimized and non-minimized.
> And in my opinion, it is a little noisy if USER_Driver->pFlashWindow( hWnd, bInvert ) is called all the time.
> So I'll prefer to only let the user notice new message when the window is minimized (it may hard to be noticed when
> it's minimized).

I have the same objection as above.  The driver interface should not be designed based on the capabilities of one specific driver (or things like WM/DE behavior, which are specific to that driver).  It has to be general enough for any driver.  The choices about what the driver actually does should be left to the driver, not user32.  You're making choices which unnecessarily limit what the driver can choose.

-Ken