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

Prevent the v1 propsheet from zeroing colors, causing black text on black background. #2651

Merged
merged 6 commits into from
Oct 2, 2019

Conversation

zadjii-msft
Copy link
Member

Summary of the Pull Request

The v1 console doesn't know anything about "default foreground" or "default background" colors. So when the v1 console starts up, it leaves those members of the CONSOLE_STATE_INFO zero'd. However, both the v1 and v2 console's share the same console.dll, the propsheet. When the user unchecks the "use legacy settings" checkbox and saves the settings, the propsheet tries to save the v2 settings that are totally inaccessible to the "v1" propsheet, and were zero'd to match the state of the console.

This change makes is so that the propsheet only saves the "Terminal" settings when the propsheet was launched from a v2 console.

PR Checklist

Detailed Description of the Pull Request / Additional comments

This PR fixes the bug for both loading the settings from the registry and loading from the shortcut.

Validation Steps Performed

  1. Copied my modified console.dll into system32.
  2. Opened a v2 conhost from win+r, cmd (using the registry settings)
  3. opened the propsheet, confirmed it had a default FG/BG.
  4. Used the propsheet to switch to legacy mode
  5. Opened a v1 conhost with win+r
  6. Opened the propsheet, confirmed there was no "Terminal" propsheet
  7. unchecked the "legacy" checkbox
  8. Opened a new win+r conhost
  9. It re-opened with the original default colors

and then repeat steps 2-9 using the Win+X, C conhost, which uses a shortcut instead of the registry.

  What's happening is the console is writing the Forcev2 setting, then the v1
  console is ignoring those settings, then when you check the checkbox to save
  the v2 settings, we'll write the zeros out. That's obviously bad. So we'll
  only write the v2 settings back to the registry if the propsheet was launched
  from a v2 console.

  This does not fix the shortcut path. That'll be the next commit.
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase labels Sep 3, 2019
@DHowett-MSFT
Copy link
Contributor

One last validation step: right-click a shortcut from explorer, launch propsheet that way. The "no console host" path.

zadjii-msft added a commit that referenced this pull request Sep 5, 2019
@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Sep 6, 2019
@ghost ghost requested review from miniksa and carlos-zamora September 6, 2019 16:29
@DHowett-MSFT
Copy link
Contributor

@zadjii-msft did it end up working in the shortcut-only case?

ghost pushed a commit that referenced this pull request Sep 9, 2019
* this actually fixes #1219

* the terminal page should check the checkbox on the options page

* Discard these changes from #2651

* Add comments, pull function out to helper
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Just want an answer to that one comment before signing off.

src/propslib/ShortcutSerialization.cpp Show resolved Hide resolved
miniksa
miniksa previously requested changes Sep 9, 2019
src/propsheet/globals.h Outdated Show resolved Hide resolved
tools/bx.ps1 Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 9, 2019
@ghost ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Sep 18, 2019
@ghost
Copy link

ghost commented Sep 18, 2019

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

DHowett-MSFT pushed a commit that referenced this pull request Sep 23, 2019
* this actually fixes #1219

* the terminal page should check the checkbox on the options page

* Discard these changes from #2651

* Add comments, pull function out to helper

(cherry picked from commit ce34c73)
@ghost ghost closed this Sep 25, 2019
@zadjii-msft zadjii-msft reopened this Sep 25, 2019
@ghost ghost removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. labels Sep 25, 2019
@zadjii-msft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlos-zamora carlos-zamora merged commit b97db63 into master Oct 2, 2019
@carlos-zamora carlos-zamora deleted the dev/migrie/b/2319-v1-propsheet-zeros-colors branch October 2, 2019 23:05
DHowett-MSFT pushed a commit that referenced this pull request Oct 17, 2019
…lack background. (#2651)

* This fixes the registry path

  What's happening is the console is writing the Forcev2 setting, then the v1
  console is ignoring those settings, then when you check the checkbox to save
  the v2 settings, we'll write the zeros out. That's obviously bad. So we'll
  only write the v2 settings back to the registry if the propsheet was launched
  from a v2 console.

  This does not fix the shortcut path. That'll be the next commit.

* Fix the shortcut loading too

  fixes #2319

* remove the redundant property I added

* add some notes to the bx.ps1 change

(cherry picked from commit b97db63)
DHowett-MSFT pushed a commit that referenced this pull request Oct 17, 2019
- Fix snapping to the cursor in "Terminal Scrolling" mode (GH-2705)

fixes GH-1222

  PSReadline calls SetConsoleCursorPosition on each character. We try to snap.

  However, we'd only ever do this with the visible viewport, the viewport
  defined by `SCREEN_INFORMATION::_viewport`. When there's a virtual viewport in
  Terminal Scrolling mode, we actually need to snap the virtual viewport, so
  that this behavior looks more regular.

(cherry picked from commit 6f4b98a)
- Passthrough BEL in conpty (GH-2990)

fixes GH-2952.

(cherry picked from commit 6831120)
- make filling chars (and, thus, erase line/char) unset wrap (GH-2831)

EraseInLine calls `FillConsoleOutputCharacterW()`. In filling the row with
chars, we were setting the wrap flag. We need to specifically not do this on
ANY _FILL_ operation. Now a fill operation UNSETS the wrap flag if we fill to
the end of the line.

Originally, we had a boolean `setWrap` that would mean...
- **true**: if writing to the end of the row, SET the wrap value to true
- **false**: if writing to the end of the row, DON'T CHANGE the wrap value

,- current wrap value
| ,- fill last cell?
| | ,- new wrap value
| | | ,- comments
|-|-|-|
|1|0|0| THIS CASE WAS UNHANDLED

To handle that special case (1-0-0), we need to UNSET the wrap. So now, we have
~setWrap~ `wrap` mean the following:
- **true**: if writing to the end of the row, SET the wrap value to TRUE
- **false**: if writing to the end of the row, SET the wrap value to FALSE
- **nullopt**: leave the wrap value as it is

Closes GH-1126

(cherry picked from commit 4dd9f9c)
- When reverse indexing, preserve RGB/256 attributes (GH-2987)

(cherry picked from commit 847d6b5)
- do not allow CUU and CUD to move "across" margins. (GH-2996)

If we're moving the cursor up, its vertical movement should be stopped
at the top margin.
Similarly, this applies to moving down and the bottom margin.

Fixes GH-2929.

(cherry picked from commit 0ce08af)
- Prevent the v1 propsheet from zeroing colors, causing black text on black background. (GH-2651)

  fixes GH-2319

(cherry picked from commit b97db63)
- Render the cursor state to VT (GH-2829)

(cherry picked from commit a9f3849)
- Don't put NUL in the buffer in VT mode (GH-3015)

(cherry picked from commit a82d6b8)
- Return to ground when we flush the last char (GH-2823)

The InputStateMachineEngine was incorrectly not returning to the ground
state after flushing the last sequence. That means that something like
alt+backspace would leave us in the Escape state, not the ground state.
This makes sure we return to ground.

Additionally removes the "Parser.UnitTests-common.vcxproj" file, which
was originally used for a theoretical time when we only open-sourced the
parser. It's unnecessary now, and we can get rid of it.

Also includes a small patch to bcz.cmd, to make sure bx works with
projects with a space in their name.

(cherry picked from commit 53c81a0)
- Add support for passing through extended text attributes, like… (GH-2917)
 ...

Related work items: #23678919, #23678920, #23731910, #23731911, #23731912, #23731913, #23731914, #23731915, #23731916, #23731917, #23731918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggling Legacy console on and off enables Terminal colors but sets them to all black
4 participants