Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for unification of monitor processing #212

Conversation

Nexarian
Copy link
Contributor

Related to neutrinolabs/xrdp#1895. Need to update to reflect the changes to xrdp_client_info.h

@Nexarian
Copy link
Contributor Author

@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 Nexarian force-pushed the unify_monitor_description_processing_resize_sec branch from ff96ec1 to b9de2ee Compare March 21, 2022 00:38
@Nexarian
Copy link
Contributor Author

Ah I figured it out. Also needed this: neutrinolabs/xrdp@341a325

@Nexarian
Copy link
Contributor Author

Decided to separate out the fix for the checks to keep the commits more atomic: #213

@Nexarian Nexarian force-pushed the unify_monitor_description_processing_resize_sec branch from b9de2ee to 27ef6aa Compare March 21, 2022 01:00
@matt335672
Copy link
Member

Hi @Nexarian

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

@Nexarian Nexarian force-pushed the unify_monitor_description_processing_resize_sec branch from 27ef6aa to 77c2d81 Compare March 21, 2022 14:33
@Nexarian
Copy link
Contributor Author

@matt335672 It now fails as expected!

@matt335672
Copy link
Member

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

module/rdpClientCon.c Show resolved Hide resolved
@Nexarian Nexarian force-pushed the unify_monitor_description_processing_resize_sec branch 2 times, most recently from 8d4fe02 to a4a9224 Compare March 27, 2022 03:57
@Nexarian Nexarian requested a review from metalefty March 27, 2022 03:59
@Nexarian
Copy link
Contributor Author

@matt335672 @metalefty Take another look, and hopefully this can be merged along with neutrinolabs/xrdp#1895

@matt335672
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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 Nexarian force-pushed the unify_monitor_description_processing_resize_sec branch from 51c0cb5 to 600095a Compare March 28, 2022 19:54
@Nexarian
Copy link
Contributor Author

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

@matt335672 matt335672 merged commit 0d5cbfe into neutrinolabs:devel Mar 29, 2022
@matt335672
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants