Skip to content

Background desktop is removed#4

Open
denis-fokin wants to merge 2 commits into
trofimander:masterfrom
denis-fokin:master
Open

Background desktop is removed#4
denis-fokin wants to merge 2 commits into
trofimander:masterfrom
denis-fokin:master

Conversation

@denis-fokin
Copy link
Copy Markdown

Background desktop is removed.
IDEA-143603 Operating systems clipboard is ignored under Windows.

@rprichard
Copy link
Copy Markdown

winpty uses a background window station because hiding the window wasn't sufficient. (At least on XP/Vista.)

If you only care about Win7 and up, you should replace CREATE_NO_WINDOW with STARTF_USESHOWWINDOW and SW_HIDE. I would suggest entering this mode only for Windows 7 and up, using IsWindows7OrGreater/GetVersion/GetVersionEx.

Details:

winpty needs to "freeze" the console during a poll so it can correctly read from it even when output is scrolling. To do that, it needs to enable/disable console selection. To do that, it needs an HWND.

On Windows XP, CREATE_NO_WINDOW produces a console with an HWND, but the window is hidden (e.g. IsWindowVisible() return false). However, the repeated selection/unselection commands prevent the user from interacting with the GUI in any other console window. (e.g. IIRC, opening console properties, moving windows, etc. are impossible.)

My recollection was that IntelliJ still worked on Windows XP, even though the newer JDK versions complain about a lack of support. I've been assuming that winpty needs to keep working on XP.

On Windows 7, CREATE_NO_WINDOW instead produces a console with a NULL HWND, so I'd expect the selection/unselection commands to do nothing. I'm guessing you'd see corruption in the winpty output if you run a program that spews output. To look for it, make sure you output at least 3000 lines first so that the console buffer itself is scrolling (as opposed to the console window descending through the buffer).

To hide the console properly on Win7 and up, use STARTF_USESHOWWINDOW and SW_HIDE. On Win7, the selection events in one console don't affect other consoles -- perhaps because consoles were split out from csrss.exe into conhost.exe.

I'm guessing this issue is related to (identical to?) rprichard#58. My plan for rprichard#58 was to disable background desktop creation for Windows 7 and up, then use a special winpty-agent.exe invocation for XP/Vista.

@rprichard
Copy link
Copy Markdown

Regarding rprichard#58, I don't think IntelliJ is hitting a race condition; my guess is that SetProcessWindowStation breaks the clipboard directly somehow, even if the original window station is restored.

@trofimander
Copy link
Copy Markdown
Owner

Ryan, thanks for the detailed explanation.

We don't want to brake winpty on Windows XP/Vista, but to avoid that we can just leave corresponding binaries unchanged, fixing the IDEA-143603(which seems to be quite a major problem) only for Windows 7+.

Later(probably for Intellij 2016.2) we'd like to switch completely(making this fork deprecated) to the brand new refactored winpty(which already has a bunch of useful improvements) and if it contains rprichard#58 fixed also for Win XP then we could rebuild it (and we'll need to do that anyway as an API will change).

@rprichard
Copy link
Copy Markdown

We don't want to brake winpty on Windows XP/Vista, but to avoid that we can just leave corresponding binaries unchanged, fixing the IDEA-143603(which seems to be quite a major problem) only for Windows 7+.

Makes sense. I think XP has its own binaries already. Does Vista use the same binaries as Windows 7? That can be changed, of course.

Later(probably for Intellij 2016.2) we'd like to switch completely(making this fork deprecated) to the brand new refactored winpty ...

At the moment, my version of winpty still doesn't have the ability to scrape a special stderr-only console buffer (i.e. "console mode"). I think that's the major thing that IntelliJ needs that's still missing.

winpty has a newer API on the https://github.com/rprichard/winpty/tree/libwinpty-rewrite branch, but that branch is a bit of a mess currently:

  • The first commit rewrites libwinpty, but it also switches to C++11, turns on exceptions, adds API flags that aren't implemented, breaks MinGW compilation, rewrites the {Read,Write}Buffer classes, etc.
  • I was a little unsure about the way the new APIs report errors. It felt a bit overengineered. Maybe it's good, though.
  • It needs the ability to assign a special console buffer to STDERR.
  • I thought it should have some way to control the STDIN/STDOUT/STDERR handles given to the agent's subprocess.

My current project is to extract the non-API-changing bits of that branch, clean them up, and then rebase libwinpty-rewrite onto the result. I'm almost done with the extraction-and-cleanup part. I'd have been done sooner, but I had some kind of forearm injury that has limited my typing. :-/

Probably worth mentioning: the new versions of winpty use C++11 and exception handling and require MSVC 2013 or newer. (The API itself is still strictly a C API--no exception is thrown across an API boundary.) AFAICT, this shouldn't be an issue for targeting XP. I'd actually prefer to require MSVC 2015, but I'm not sure what compiler versions winpty users have.

@denis-fokin
Copy link
Copy Markdown
Author

I would push the change only in traff/winpty trunk. We need to keep eye on changes we have made in our binaries.

@denis-fokin denis-fokin reopened this May 18, 2016
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.

3 participants