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

Introduce abstraction layer between TermControl and TerminalCore (aka Interactivity) #6842

Closed
carlos-zamora opened this issue Jul 8, 2020 · 3 comments · Fixed by #9820
Closed

Introduce abstraction layer between TermControl and TerminalCore (aka Interactivity) #6842

carlos-zamora opened this issue Jul 8, 2020 · 3 comments · Fixed by #9820
Assignees
@zadjii-msft
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-WPFControl Things related to the WPF version of the TermControl Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.9

Comments

@carlos-zamora
Copy link
Member

carlos-zamora commented Jul 8, 2020 •
edited
Loading

Description of the new feature/enhancement

We have some redundant code between the WPF Control and the UWP TermControl. This indicates a missing layer of abstraction between the TerminalCore and the TermControl. Some benefits that we could/would get from introducing an "interactivity" layer includes...

  • better parity between both controls
  • less duplicated code
  • better separating scrolling in both layers

Proposed technical implementation details (optional)

Specific interactions that could be abstracted better include (but are not limited to)...

  • Selection
  • Scrolling
  • VT Mouse Mode (sorta)

\cc @DHowett
@carlos-zamora carlos-zamora added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Jul 8, 2020
@carlos-zamora carlos-zamora added this to the Terminal v2.0 milestone Jul 8, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 8, 2020
This was referenced Jul 8, 2020
Merged
Open
@DHowett DHowett added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-WPFControl Things related to the WPF version of the TermControl and removed Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 9, 2020
@zadjii-msft zadjii-msft mentioned this issue Feb 8, 2021
Open
3 tasks
@zadjii-msft zadjii-msft self-assigned this Mar 25, 2021
@zadjii-msft
Copy link
Member

zadjii-msft commented Mar 31, 2021 •
edited
Loading

Alright, as of 2ac8318, this is fundamentally pulled into 3 files, but there's still a lot of work to do. I'm using this comment to track that.

  • Move ControlCore and ControlInteractivity to Microsoft.Terminal.Core so I can unittest them
    • didn't exactly do that, because that was WAY crazier. But we got tests!
  • Mousewheel transparency is busted
  • Acrylic actually doesn't work at all
  • Manually print hyperlink to console
  • auto-detecting hyperlinks doesn't work
    • oh good this actually has a TODO!
  • Trying to make a second selection while one is active is weird
  • Actually making a second selection while one is active is impossible? (OH BUT IT'S BUSTED IN 1.8.892.0 AND 1.8.921.0 - fixed in Fix marking of new selection start #9727)
  • multi-click selection does seem to work
  • The scrollbar never moves!
  • Scrolling with the mouse is fine. With the trackpad though: Scrolling up seems to work, but scrolling down is very unsmooth
  • OH GOD just typing dir (presumably causing a scroll) HANGS THE TERMINAL
  • wheel scrolling of the font size seems to work
  • keybinding changing of the font size seems to work
  • changing DPI seems fine
  • Find dialog opens, works
  • Closing a tab with right-click close tab reliably crashes the Terminal
    • some crash in ITerminalConnection)->remove_TerminalOutput(impl::bind_in(token))); when exiting with VS debugger attached
    • The _connection is already null in ControlCore::Close
  • Remove Windows::UI::Input::PointerPoint
    • PointerPressed
    • PointerMoved
    • PointerReleased
    • MouseWheel
    • Touched
    • TouchMoved
    • TouchReleased
  • single click = no selection
  • single click and drag = selection starting from first point
  • single click in unfocused pane and drag = focus pane, selection starting from first point
  • double-click = selects a whole word
  • triple-click = selects a whole line
  • double-click and drag = selects a whole word, drag selects whole words
  • triple-click and drag = selects a whole line, drag selects whole lines
  • Shift single-click = defines start point
  • second Shift single-click = defines end point
  • Shift double-click = selects entire word
  • Shift triple-click = selects entire line
  • Shift double-click and drag = selects entire word, drag selects whole words
  • With existing selection: single-click outside the selection and drag = establishes a new selection starting from the click point
  • toggling renderer settings at runtime (without a shader set unset<->retro)
  • toggling renderer settings at runtime (with a shader set unset<->shader)
  • Hot reloading settings
    • Font
    • Antialiasing ❌ OH NO - seemingly a DPI change fixes this though. So something in the stack there is wrong. Not triggering a redraw, but triggering a font change?
      • Actually, this NEVER hot reloaded! We need to force the _d2dDeviceContext to the new setting
    • Background color
    • BG image
    • retro effect
    • Acrylic opacity
    • Scheme
    • CopyOnSelect
  • Hot reloading the settings seemed to make text with a background color disappear. But it didn't happen in both windows. ?
    • this never reprod again. Must've been something else.
    • I saw this come back after merging Add support for a profile to specify an "unfocused" appearance #8392. Might have actually been caused by that PR, need to investigate.
    • The two repros I saw were:
      • Change the BG to have an image - some of the text was temporarily invisible. not all of it, and not only the text with a BG color.
      • Mouse wheel the opacity - again once triggered the invisibility, but might have been a settings reload that hadn't yet propagated.
  • Right-click copy w/o CopyOnSelect
  • CopyOnSelect
  • read-only mode doesn't work at all currently.
    • keyboard input
    • mouse input in VT mode
  • The taskbar is fucky Progress state doesn't update correctly between panes #9743
    • Setting the taskbar progress to indeterminate changes the taskbar, but not the tab icon
    • Set the taskbar progress to indeterminate in one tab, and switch to another tab. The taskbar still shows indeterminate. Neither tab has the spinner icon.
    • Set the taskbar progress to indeterminate in one pane, and switch to another pane. The taskbar still shows indeterminate. Neither pane gets the spinner icon.

Post- 5dc1632

  • The font weight doesn't load right initially

Other scenarios to check:

  • Click+drag out of the window to autoscroll the buffer
  • snap to grid - does that still work?
    • Seems good on 96 DPI
    • Might not work on high DPI
      • nevermind, this is behaving the same as it did before
    • Is the window supposed to resize when mouse scrolling the font size with snap to grid? because it doesn't currently.
      • It is not supposed to. So that's a good sign.
  • Mouse mode: Shift single-click = defines start point
  • Mouse mode: second Shift single-click = defines end point
  • Mouse mode: Shift double-click = selects entire word
  • Mouse mode: Shift triple-click = selects entire line
  • Mouse mode: Shift double-click and drag = selects entire word, drag selects whole words
  • Does actual mouse mode still work?
  • Prevent mouse dragging from dismissing existing selection Prevent mouse dragging from dismissing existing selection #9790
  • more...

Tests to write:

  • Changing opacity with mouse wheel
  • Closing the ControlCore, then dtoring it
  • Scrolling with the mouse
  • Scrolling by panning with touch
  • scroll initiated by the terminal
  • scrollbar initiated scrolling
  • single click = no selection
  • single click and drag = selection starting from first point
  • With existing selection: single-click outside the selection and drag = establishes a new selection starting from the click point
  • Prevent mouse dragging from dismissing existing selection Prevent mouse dragging from dismissing existing selection #9790

@zadjii-msft zadjii-msft mentioned this issue Mar 31, 2021
Merged
2 tasks
ghost pushed a commit that referenced this issue Apr 5, 2021
@zadjii-msft
Create a control unittesting project (#9677)
24b9a7a 
Does what it says on the can.

This is a follow up to #9472. Now that we have a control .lib, we can add tests for it. 

Unfortunately, the `TermControl` itself is a horrible mess. So this new unittest lib is empty for now. I'm working on actual tests as a part of #6842, but this PR is here to keep the diffs smaller.

Also, apparently `server.vcxproj` had the wrong GUID in it.

* [x] I work here
* [x] Adds tests
This was referenced Apr 6, 2021
Merged
Closed
@zadjii-msft zadjii-msft mentioned this issue Apr 14, 2021
Merged
5 tasks
@ghost ghost added the In-PR This issue has a related PR label Apr 14, 2021
@DHowett
Copy link
Member

DHowett commented Apr 22, 2021

  • moving between DPIs does not introduce any issues in VT mouse events, rendering, scrolling, dragging, etc

@ghost ghost closed this as completed in #9820 Apr 27, 2021
@ghost ghost removed the In-PR This issue has a related PR label Apr 27, 2021
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Apr 27, 2021
ghost pushed a commit that referenced this issue Apr 27, 2021
@zadjii-msft
Split TermControl into a Core, Interactivity, and Control layer (#9820
8910a16 
)

## Summary of the Pull Request

Brace yourselves, it's finally here. This PR does the dirty work of splitting the monolithic `TermControl` into three components. These components are: 

* `ControlCore`: This encapsulates the `Terminal` instance, the `DxEngine` and `Renderer`, and the `Connection`. This is intended to everything that someone might need to stand up a terminal instance in a control, but without any regard for how the UX works.
* `ControlInteractivity`: This is a wrapper for the `ControlCore`, which holds the logic for things like double-click, right click copy/paste, selection, etc. This is intended to be a UI framework-independent abstraction. The methods this layer exposes can be called the same from both the WinUI TermControl and the WPF control.
* `TermControl`: This is the UWP control. It's got a Core and Interactivity inside it, which it uses for the actual logic of the terminal itself. TermControl's main responsibility is now 

By splitting into smaller pieces, it will enable us to
* write unit tests for the `Core` and `Interactivity` bits, which we desparately need
* Combine `ControlCore` and `ControlInteractivity` in an out-of-proc core process in the future, to enable tab tearout.

However, we're not doing that work quite yet. There's still lots of work to be done to enable that, thought this is likely the biggest portion.

Ideally, this would just be methods moved wholesale from one file to another. Unfortunately, there are a bunch of cases where that didn't work as well as expected. Especially when trying to better enforce the boundary between the classes. 

We've got a couple tests here that I've added. These are partially examples, and partially things I ran into while implementing this. A bunch of things from #7001 can go in now that we have this.

This PR is gonna be a huge pain to review - 38 files with 3,730 additions and 1,661 deletions is nothing to scoff at. It will also conflict 100% with anything that's targeting `TermControl`. I'm hoping we can review this over the course of the next week and just be done with it, and leave plenty of runway for 1.9 bugs in post.

## References

* In pursuit of #1256
* Proc Model: #5000
* https://github.com/microsoft/terminal/projects/5

## PR Checklist
* [x] Closes #6842
* [x] Closes https://github.com/microsoft/terminal/projects/5#card-50760249
* [x] Closes https://github.com/microsoft/terminal/projects/5#card-50760258
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

* I don't love the names `ControlCore` and `ControlInteractivity`. Open to other names.
* I added a `ICoreState` interface for "properties that come from the `ControlCore`, but consumers of the `TermControl` need to know". In the future, these will all need to be handled specially, because they might involve an RPC call to retrieve the info from the core (or cache it) in the window process.
* I've added more `EventArgs` to make more events proper `TypedEvent`s.
* I've changed how the TerminalApp layer requests updated TaskbarProgress state. It doesn't need to pump TermControl to raise a new event anymore.
* ~~Something that snuck into this branch in the very long history is the switch to `DCompositionCreateSurfaceHandle` for the `DxEngine`. @miniksa wrote this originally in 30b8335, I'm just finally committing it here. We'll need that in the future for the out-of-proc stuff.~~
  * I reverted this in c113b65. We can revert _that_ commit when we want to come back to it.
* I've changed the acrylic handler a decent amount. But added tests!
* All the `ThrottledFunc` things are left in `TermControl`. Some might be able to move down into core/interactivity, but once we figure out how to use a different kind of Dispatcher (because a UI thread won't necessarily exist for those components).
* I've undoubtably messed up the merging of the locking around the appearance config stuff recently

## Validation Steps Performed

I've got a rolling list in #6842 (comment) that I'm updating as I go.
@ghost
Copy link

ghost commented May 25, 2021

🎉This issue was addressed in #9820, which has now been successfully released as Windows Terminal Preview v1.9.1445.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zadjii-msft zadjii-msft

Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Area-WPFControl Things related to the WPF version of the TermControl Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.9
Development

Successfully merging a pull request may close this issue.

Split TermControl into a Core, Interactivity, and Control layer microsoft/terminal
3 participants
@DHowett @carlos-zamora @zadjii-msft