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

Remember last-used string in the Find dialog in conhost #2845

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

asklar
Copy link
Member

@asklar asklar commented Sep 22, 2019

Remember the last searched-for string in the Find dialog

References

#2844

PR Checklist

  • Closes Console Find dialog should remember last-searched-for string #2844
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Manually verified the dialog retains the last used string for the lifetime of the process

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

This seems fine to me. It's in line with fFindSearchUp, and it's in an interactive component only.

Does SetDlgItemText copy the string (presumably zero-terminated) into [some internal dialog buffer]?

@DHowett-MSFT DHowett-MSFT added the Needs-Second It's a PR that needs another sign-off label Sep 22, 2019
@ghost ghost requested review from zadjii-msft, miniksa and carlos-zamora September 22, 2019 02:59
@asklar
Copy link
Member Author

asklar commented Sep 22, 2019

This seems fine to me. It's in line with fFindSearchUp, and it's in an interactive component only.

Does SetDlgItemText copy the string (presumably zero-terminated) into [some internal dialog buffer]?

Yes, that's correct. See MSDN link here: SetDlgItemText and WM_SETTEXT

@asklar asklar closed this Sep 22, 2019
@asklar asklar reopened this Sep 22, 2019
@zadjii-msft zadjii-msft merged commit bfb1484 into microsoft:master Sep 23, 2019
@asklar asklar deleted the user/asklar/2844 branch September 24, 2019 01:36
DHowett-MSFT pushed a commit that referenced this pull request Sep 26, 2019
DHowett-MSFT pushed a commit that referenced this pull request Sep 26, 2019
- Remember last-used string in the Find dialog in conhost (GH-2845)

  (cherry picked from commit bfb1484)

- Bugfix: TextBuffer Line Wrapping from VTRenderer (GH-2797)

  Change VT renderer to do erase line instead of a ton of erase chars

  (cherry picked from commit 8afc5b2)

- Add some retry support to Renderer::PaintFrame (GH-2830)

  If _PaintFrameForEngine returns E_PENDING, we'll give it another two
  tries to get itself straight. If it continues to fail, we'll take down
  the application.

  We observed that the DX renderer was failing to present the swap chain
  and failfast'ing when it did so; however, there are some errors from
  which DXGI guidance suggests we try to recover. We'll now return
  E_PENDING (and destroy our device resources) when we hit those errors.

  Fixes GH-2265.

  (cherry picked from commit 277acc3)

- Enable VT processing by default for ConPTY (GH-2824)

  This change enables VT processing by default for _all_ conpty clients. See
  GH-1965 for a discussion on why we believe this is a righteous change.

  Also mentioned in the issue was the idea of only checking the
  `VirtualTerminalLevel` reg key in the conpty startup. I don't think this would
  be a more difficult change, looks like all we'd need is a simple
  `reg.LoadGlobalsFromRegistry();` call instead of this change.

  **Validation Steps Performed**
  Manually launched a scratch app in both the terminal and the console. The
  console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the `
  ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag, which the client now had by default
  in the Terminal.

  Closes GH-1965

  (cherry picked from commit 1c412d4)

- Remove unwanted DECSTBM clipping (GH-2764)

  The `DECSTBM` margins are meant to define the range of lines within which
  certain vertical scrolling operations take place. However, we were applying
  these margin restrictions in the `ScrollRegion` function, which is also used in
  a number of places that shouldn't be affected by `DECSTBM`.

  This includes the `ICH` and `DCH` escape sequences (which are only affected by
  the horizontal margins, which we don't yet support), the
  `ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be
  affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback
  extension (which isn't really scrolling as such, but uses the `ScrollRegion`
  function to manipulate the scrollback buffer).

  This commit moves the margin clipping out of the `ScrollRegion` function, so it
  can be applied exclusively in the places that need it.

  While testing, I also noticed that one of the `ScrollRegion` calls in the
  `AdjustCursorPosition` function was not setting the horizontal range correctly
  - the scrolling should always affect the full buffer width rather than just the
    viewport width - so ...

Related work items: #23507749, #23563809, #23563819, #23563837, #23563841, #23563844
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Console Find dialog should remember last-searched-for string
3 participants