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.

Sign up for GitHub

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

Jump to bottom

Fix Environment.UserInteractive on Windows #1234

Merged
merged 17 commits into from
Jan 4, 2020
Merged

Fix Environment.UserInteractive on Windows #1234

merged 17 commits into from
Jan 4, 2020

Conversation

danmoseley
Copy link
Member

Fix #770

@danmoseley danmoseley force-pushed the Environment.UserInteractive branch from 67406aa to e6ea75b Compare January 2, 2020 20:36
@jkotas
Copy link
Member

jkotas commented Jan 3, 2020 •
edited
Loading

Build break ... LGTM otherwise.

@gmatzen gmatzen mentioned this pull request Jan 3, 2020
Closed
@danmoseley
Copy link
Member Author

All failures are from the bad ARM legs.

danmoseley and others added 5 commits January 3, 2020 14:26
@danmoseley @jkotas
Update src/libraries/System.Private.CoreLib/src/System.Private.CoreLi…
f52a075 
…b.Shared.projitems

Co-Authored-By: Jan Kotas <jkotas@microsoft.com>
@danmoseley @jkotas
Update src/libraries/System.Private.CoreLib/src/System.Private.CoreLi…
65f5813 
…b.Shared.projitems

Co-Authored-By: Jan Kotas <jkotas@microsoft.com>
@danmoseley @jkotas
Update src/libraries/System.Private.CoreLib/src/System.Private.CoreLi…
960cbc7 
…b.Shared.projitems

Co-Authored-By: Jan Kotas <jkotas@microsoft.com>
@danmoseley
CR feedback
444554b
@danmoseley
Merge branch 'Environment.UserInteractive' of https://github.com/danm…
ef583c0 
…osemsft/runtime into Environment.UserInteractive
@danmoseley danmoseley requested a review from jkotas January 4, 2020 00:43
@danmoseley
Copy link
Member Author

Any more feedback? I can't merge without dismissing your review -- not sure of the protocol

CI is fine, except for an unrelated bug I logged.

@danmoseley danmoseley merged commit 2c6ec17 into dotnet:master Jan 4, 2020
@danmoseley danmoseley deleted the Environment.UserInteractive branch January 4, 2020 07:11
@ericstj ericstj mentioned this pull request Jan 15, 2020
Open
@janvorli janvorli mentioned this pull request Jan 30, 2020
Closed
@danmoseley danmoseley mentioned this pull request Feb 27, 2020
Closed
ericstj
ericstj reviewed Feb 27, 2020
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Environment.Windows.cs
uint dummy = 0;
if (Interop.User32.GetUserObjectInformationW(handle, Interop.User32.UOI_FLAGS, &flags, (uint)sizeof(Interop.User32.USEROBJECTFLAGS), ref dummy))
{
return ((flags.dwFlags & Interop.User32.WSF_VISIBLE) == 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you forgot to invert this logic. Also, should we add back the static property like we had in desktop?
https://referencesource.microsoft.com/#mscorlib/system/environment.cs,1435

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack - thank you @ericstj !

Re the static, @jkotas pointed out that there may be a way to change a process to/from interactive - although I didn't do it successfully myself. See mention here #770 (comment) I saw no reason to cache the value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll make the simple fix for now. FWIW I'm now able to reproduce the test failure, so I can confirm fail/then-fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would explain #1724 also. I was not albe to run the service controller tests locally because of #1262. I tested locally by hand instead, and apparently did so before I made this mistake.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments crossed. Great, I was about to do it, but apparently you've already got those tests working so you can verify it as well. thanks

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ericstj ericstj ericstj left review comments

@jkotas jkotas jkotas approved these changes

Assignees
No one assigned
Labels
area-System.Runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should we revise implementation of Environment.UserInteractive?
4 participants
@danmoseley @jkotas @ericstj @Dotnet-GitSync-Bot