neutrinolabs / xorgxrdp

Xorg drivers for xrdp
Other
428 stars 108 forks source link

Update for unification of monitor processing #212

Closed Nexarian closed 2 years ago

Nexarian commented 2 years ago

Related to https://github.com/neutrinolabs/xrdp/pull/1895. Need to update to reflect the changes to xrdp_client_info.h

Nexarian commented 2 years ago

@matt335672 @metalefty I'm not sure why the build checks fail. The probably will because of the dependency on xrdp, but additionally, it looks like something is wrong with the build environment. Embarrassingly, I also can't find the "retry/re-run" option.

Nexarian commented 2 years ago

Ah I figured it out. Also needed this: https://github.com/neutrinolabs/xrdp/commit/341a32581cb902c49cb557c956d754c912a31d6e

Nexarian commented 2 years ago

Decided to separate out the fix for the checks to keep the commits more atomic: https://github.com/neutrinolabs/xorgxrdp/pull/213

matt335672 commented 2 years ago

Hi @Nexarian

I've pulled your other fix in, so if you re-base this it should get through the CI now.

Nexarian commented 2 years ago

@matt335672 It now fails as expected!

matt335672 commented 2 years ago

That's fine - we can merge this at the same time as the other changes in neutrinolabs/xrdp#1895.

Nexarian commented 2 years ago

@matt335672 @metalefty Take another look, and hopefully this can be merged along with https://github.com/neutrinolabs/xrdp/pull/1895

matt335672 commented 2 years ago

That'll work, and I'm sort of Okay with it, but rather than having three explicit checks against CLIENT_INFO_CURRENT_VERSION, it might be better to add a level of indirection:-

/* See <add suitable reference> */
#if CLIENT_INFO_CURRENT_VERSION > 20210723
#   define CLIENT_INFO_HAS_DISPLAY_SIZES
#endif

then :-

#if CLIENT_INFO_HAS_DISPLAY_SIZES
    if (clientCon->client_info.display_sizes.monitorCount > 0)
#else
    if (clientCon->client_info.monitorCount > 0)
#endif

etc.

What do people think? Is that overthinking it?

Nexarian commented 2 years ago

I believe it is overthinking it, largely because:

  1. We do not generally intend to have xorgxrdp be backwards compatible. This is a specific, bespoke, one-off request from @metalefty that I believe will be short term.
  2. If we DO intend for xorgxrdp to be backwards compatible with varying versions of XRDP, that is a much bigger conversation, and neither what I've written here nor what you propose would be sufficient to deal with that.

My change is a surgical fix to directly fix this issue, and only that. Doing more risks moving the surgery from "minimally invasive" to "actually invasive."

I also expect/suspect we'd want to remove this fix when the next release happens, and this is here only for the interim.

matt335672 commented 2 years ago

OK - that's fine. You've convinced me.

Do you want to add a comment somewhere along those lines? The comment will obviously also be deleted when we remove this fix!

Nexarian commented 2 years ago

@matt335672 Alright, updated. See the issue I've linked to!

matt335672 commented 2 years ago

@Nexarian, @metalefty - I've added an additional Wiki page for xorgxrdp here:-

https://github.com/neutrinolabs/xorgxrdp/wiki/NEWS

This serves the same purpose as the NEWS page for xrdp. We can collect the merged changes as we go along so it makes the actual release process easier.