Re: [PATCH 2/9] winemac.drv: EnumDisplayDevices placeholder implementation

[Ken Thomases]

Hi,

Thanks for taking this up again…

On Apr 24, 2017, at 3:48 PM, Donat Enikeev  wrote:
> Based on Ken Thomases  staging patches
> Signed-off-by: Donat Enikeev
>
> dlls/winemac.drv/display.c        | 51 +++++++++++++++++++++++++++++++++++++++
> dlls/winemac.drv/winemac.drv.spec |  1 +
> 2 files changed, 52 insertions(+)

This doesn't build:

display.c:1007:13: error: use of undeclared identifier 'monitors'
        if (monitors[iDevNum].dwFlags & MONITORINFOF_PRIMARY)
            ^
display.c:1010:80: error: use of undeclared identifier 'monitors'
        memcpy(lpDisplayDevice->DeviceName, monitors[iDevNum].szDevice, sizeof(monitors[iDevNum].szDevice));
                                                                               ^
display.c:1010:45: error: use of undeclared identifier 'monitors'
        memcpy(lpDisplayDevice->DeviceName, monitors[iDevNum].szDevice, sizeof(monitors[iDevNum].szDevice));
                                            ^

Is this patch written on the assumption that some other staging patches have already been applied?

Beyond that, can you explain the rationale of moving the implementation of EnumDisplayDevices to the drivers?  My patches implemented it in user32 based on EnumDisplayMonitors() and GetMonitorInfoW().  It may be somewhat clunky, but has the advantage that it ensures all three functions return mutually consistent results.  Since you appear to have dropped my patches which caused GetMonitorInfoW() to return unique names for the displays, they are no longer consistent.  GetMonitorInfoW() always uses "\.\DISPLAY1" for all monitors while this implementation of EnumDisplayDevices() attempts to use "\.\DISPLAY2", etc., for secondary and subsequent monitors.

For what it's worth, my original patches were rejected because Alexandre wanted the tracking and management of displays and display modes centralized in the explorer process[1].  I have my reservations about that approach, mind you. ;)
[1] https://www.winehq.org/pipermail/wine-devel/2014-March/103680.html

Issues with other patches in the series:

Patch 1: \ No newline at end of file

Patches 1 and 2 ought to come after 3.  In the current order, they introduce dead code.

-Ken

[PATCH 2/9] winemac.drv: EnumDisplayDevices placeholder implementation

[Donat Enikeev]

Hi Ken,
> This doesn't build:

Thanks, will have a deeper look. Though since I don't have mac (even VM), it was aimed to bring just backward compatibility with the migration, while actual devices/displays enumeration, flags, etc -- are kept for someone having experience with Cocoa, with an ability to check results live on multi-display setup.
> Beyond that, can you explain the rationale of moving the implementation of EnumDisplayDevices to the drivers?

Primarily - centralization and integrity: user32 knows nothing about device names, ids, etc and fully relies on driver implementation. All others function in the row are there and sharing same data that is as close to actual device information as possible. Whenever it is decided to refactor the whole approach -- it is supposed to be much easier from the one place. So it looks reasonable to me
> It may be somewhat clunky, but has the advantage that it ensures all three functions return mutually consistent results. Since you appear to have dropped my patches which caused GetMonitorInfoW() to return unique names for the displays, they are no longer consistent.

I am probably missing something, but they supposed to be consistent (at least for winex11.drv)

Within initialization we fill structure with proper name for each device

-            /* FIXME: using the same device name for all monitors for now */
-            lstrcpyW( monitors[i].szDevice, default_monitor.szDevice );
+            sprintfW( monitors[i].szDevice, adapter_name, i + 1);

While GetMonitorInfo() still does nothing other than returning whatever is there (another advantage of the moving)

if (info->cbSize >= sizeof(MONITORINFOEXW))
        lstrcpyW( ((MONITORINFOEXW *)info)->szDevice, monitors[i].szDevice );
> because Alexandre wanted the tracking and management of displays and display modes centralized in the explorer process[1].

Not sure what was meant exactly here, but the patch making things work with certain respect to surrounding approach - looks to me as a small but good first step.
> I have my reservations about that approach, mind you. ;)

Could you please clarify this point, do you mean you are going to work all that out in the nearest future?
> Patch 1: \ No newline at end of file

Thanks, will fix
> Patches 1 and 2 ought to come after 3. In the current order, they introduce dead code.

Yes, the patches 1 and 2 don't make sense without 3, so the order is not that important probably,  but no problem to change it

Best,
Donnie

25.04.2017, 07:07, "Ken Thomases" :
> Hi,
>
> Thanks for taking this up again…
>
> On Apr 24, 2017, at 3:48 PM, Donat Enikeev  wrote:
> > Based on Ken Thomases  staging patches
> >
> > Signed-off-by: Donat Enikeev 
> >  ---
> >  dlls/winemac.drv/display.c | 51 +++++++++++++++++++++++++++++++++++++++
> >  dlls/winemac.drv/winemac.drv.spec | 1 +
> >  2 files changed, 52 insertions(+)
>
> This doesn't build:
>
> display.c:1007:13: error: use of undeclared identifier 'monitors'
>         if (monitors[iDevNum].dwFlags & MONITORINFOF_PRIMARY)
>             ^
> display.c:1010:80: error: use of undeclared identifier 'monitors'
>         memcpy(lpDisplayDevice->DeviceName, monitors[iDevNum].szDevice, sizeof(monitors[iDevNum].szDevice));
>                                                                                ^
> display.c:1010:45: error: use of undeclared identifier 'monitors'
>         memcpy(lpDisplayDevice->DeviceName, monitors[iDevNum].szDevice, sizeof(monitors[iDevNum].szDevice));
>                                             ^
>
> Is this patch written on the assumption that some other staging patches have already been applied?
>
> Beyond that, can you explain the rationale of moving the implementation of EnumDisplayDevices to the drivers? My patches implemented it in user32 based on EnumDisplayMonitors() and GetMonitorInfoW(). It may be somewhat clunky, but has the advantage that it ensures all three functions return mutually consistent results. Since you appear to have dropped my patches which caused GetMonitorInfoW() to return unique names for the displays, they are no longer consistent. GetMonitorInfoW() always uses "\.\DISPLAY1" for all monitors while this implementation of EnumDisplayDevices() attempts to use "\.\DISPLAY2", etc., for secondary and subsequent monitors.
>
> For what it's worth, my original patches were rejected because Alexandre wanted the tracking and management of displays and display modes centralized in the explorer process[1]. I have my reservations about that approach, mind you. ;)
> [1] https://www.winehq.org/pipermail/wine-devel/2014-March/103680.html
>
> Issues with other patches in the series:
>
> Patch 1: \ No newline at end of file
>
> Patches 1 and 2 ought to come after 3. In the current order, they introduce dead code.
>
> -Ken

[PATCH 2/9] winemac.drv: EnumDisplayDevices placeholder implementation

[Ken Thomases]

On Apr 25, 2017, at 12:58 AM, Donat Enikeev  wrote:
> > This doesn't build:
>
> Thanks, will have a deeper look. Though since I don't have mac (even VM), it was aimed to bring just backward compatibility with the migration, while actual devices/displays enumeration, flags, etc -- are kept for someone having experience with Cocoa, with an ability to check results live on multi-display setup.

Then just leave the Mac driver patch out of the series.  The null driver in user32 should probably contain the old implementation to prevent regressions for drivers which don't implement the entry point.  If your patches are accepted, I'll implement it for the Mac driver.
> > It may be somewhat clunky, but has the advantage that it ensures all three functions return mutually consistent results. Since you appear to have dropped my patches which caused GetMonitorInfoW() to return unique names for the displays, they are no longer consistent.
>
> I am probably missing something, but they supposed to be consistent (at least for winex11.drv)

Right, but it doesn't work that way in the Mac driver.  See my original patch https://github.com/wine-compholio/wine-staging/blob/master/patches/gdi32-MultiMonitor/0004-winemac-Make-GetMonitorInfo-give-a-different-device-.patch.
> > because Alexandre wanted the tracking and management of displays and display modes centralized in the explorer process[1].
>
> Not sure what was meant exactly here, but the patch making things work with certain respect to surrounding approach - looks to me as a small but good first step.

That's what I thought about my original patches.  We'll have to see if Alexandre agrees. :)
> > I have my reservations about that approach, mind you. ;)
>
> Could you please clarify this point, do you mean you are going to work all that out in the nearest future?

No.  I meant I have doubts that Alexandre's preferred approach will be feasible on the Mac.  I was referring to my replies to Alexandre's email that I linked to:
https://www.winehq.org/pipermail/wine-devel/2014-March/103685.html
https://www.winehq.org/pipermail/wine-devel/2014-March/103688.html

-Ken

[PATCH 2/9] winemac.drv: EnumDisplayDevices placeholder implementation

[Donat Enikeev]

Hi Ken,
> Then just leave the Mac driver patch out of the series.

Yes, it was a bad idea, I've just moved old implementation to `nulldrv` 
and submitted other fixes mentioned here, hope it's accepted and you 
are able to add necessary implementation for mac.

Best,
Donnie

On Вт, апр 25, 2017 at 9:18 , Ken Thomases  
wrote:
> On Apr 25, 2017, at 12:58 AM, Donat Enikeev  wrote:
> > > This doesn't build:
> >
> > Thanks, will have a deeper look. Though since I don't have mac 
> > (even VM), it was aimed to bring just backward compatibility with 
> > the migration, while actual devices/displays enumeration, flags, etc 
> > -- are kept for someone having experience with Cocoa, with an 
> > ability to check results live on multi-display setup.
>
> Then just leave the Mac driver patch out of the series.  The null 
> driver in user32 should probably contain the old implementation to 
> prevent regressions for drivers which don't implement the entry 
> point.  If your patches are accepted, I'll implement it for the Mac 
> driver.
> > > It may be somewhat clunky, but has the advantage that it ensures 
> > > all three functions return mutually consistent results. Since you 
> > > appear to have dropped my patches which caused GetMonitorInfoW() to 
> > > return unique names for the displays, they are no longer consistent.
> >
> > I am probably missing something, but they supposed to be consistent 
> > (at least for winex11.drv)
>
> Right, but it doesn't work that way in the Mac driver.  See my 
> original patch 
> https://github.com/wine-compholio/wine-staging/blob/master/patches/gdi32-MultiMonitor/0004-winemac-Make-GetMonitorInfo-give-a-different-device-.patch.
> > > because Alexandre wanted the tracking and management of displays 
> > > and display modes centralized in the explorer process[1].
> >
> > Not sure what was meant exactly here, but the patch making things 
> > work with certain respect to surrounding approach - looks to me as a 
> > small but good first step.
>
> That's what I thought about my original patches.  We'll have to see 
> if Alexandre agrees. :)
> > > I have my reservations about that approach, mind you. ;)
> >
> > Could you please clarify this point, do you mean you are going to 
> > work all that out in the nearest future?
>
> No.  I meant I have doubts that Alexandre's preferred approach will 
> be feasible on the Mac.  I was referring to my replies to Alexandre's 
> email that I linked to:
> https://www.winehq.org/pipermail/wine-devel/2014-March/103685.html
> https://www.winehq.org/pipermail/wine-devel/2014-March/103688.html
>
> -Ken