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

Convert TermControl to .xaml #4729

Merged
merged 3 commits into from
Feb 27, 2020
Merged

Conversation

DHowett-MSFT
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT commented Feb 26, 2020

This commit removes all of the custom UI initialization code in
TermControl and replaces it with a xaml file. Some dead or reundant code
was removed as part of this refactoring.

It also fixes two (quasi-related) issues:

  • The search box, on first launch, was offset by the scrollbar even if
    the scrollbar was invisible.
  • The scrollbar state wasn't hot-reloadable.

PR Checklist

  • Closes [a thing that's been hounding me since the beginning, but no real issue]

Validation Steps Performed

Launched, opened, closed, searched and TSF-inputted-into controls.

Fixes #4920

This commit removes all of the custom UI initialization code in
TermControl and replaces it with a xaml file. Some dead or reundant code
was removed as part of this refactoring.

It also fixes two (quasi-related) issues:

* The search box, on first launch, was offset by the scrollbar even if
  the scrollbar was invisible.
* The scrollbar state wasn't hot-reloadable.
@DHowett-MSFT
Copy link
Contributor Author

To be considered: All of the getters (ControlName()) really do just boil down to accesses on member variables, so while they look expensive they aren't. I don't know if using them in this way allows for addref/release elision. That part bothers me a bit.

@DHowett-MSFT
Copy link
Contributor Author

This broke Background color (!)

@DHowett-MSFT
Copy link
Contributor Author

The xaml compiler automatically generates weak event handlers for every event we subscribe to using xaml. It automatically connects the local members of the TermControl to the Names specified in the xaml file. It even emitted Unload handlers for the search box control because I made it dynamically loadable.

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.

Nothing worth blocking over. Excited for this to go in!!!

src/cascadia/TerminalControl/TermControl.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.xaml Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.xaml Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Show resolved Hide resolved
Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

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

Thanks for translating the code into XAML 😄, and everything seems to work just as expected, so 🚢 it

@DHowett-MSFT DHowett-MSFT merged commit 1de07aa into master Feb 27, 2020
@DHowett-MSFT DHowett-MSFT deleted the dev/duhowett/xaml_all_the_thangs branch February 27, 2020 00:35
ghost pushed a commit that referenced this pull request Feb 28, 2020
This pull request is like #4729 but for TSFInputControl.
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.

Bug and feature request about scrollbar
4 participants