-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Spec for shell integration marks #14792
Conversation
This comment has been minimized.
This comment has been minimized.
the terminal to expose quick actions for: | ||
|
||
* Quickly navigating the history by scrolling between commands | ||
* Re-running a previous command in the history |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the terminal know what input it needs to send in order to rerun the command?
- user presses Ctrl+V Escape to input an ESC character and the shell displays it as
^[
- user presses Ctrl+V Ctrl+J to input an LF character and the shell displays it as a line break
- user presses Enter within a quoted string and the shell displays a continuation prompt
- user types a command including an exclamation point, and the shell invokes history expansion and echoes the result of expansion before it runs the command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All excellent questions :)
Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly reviewed the technical aspects of the spec. There's just one inaccuracy that I think should be addressed. It might also help us confidently pick alternative ways of storing marks without worrying about performance issues.
I also noticed that while the spec discusses storage and reflow of the marks, I personally think that the hardest aspect is how to draw potentially thousands of marks in the scrollbar. The way it's currently done, Windows Terminal starts to lag around quite a bit during lengthy sessions with large scrollback buffer, because it draws so many rectangles and WinUI and its overhead makes it 100x worse on top (unfortunately). As such I think we need to consider algorithms to downsample marks to match the expected number of marks on the scrollbar. Since iterating through marks is supposedly very fast, such an algorithm will likely end up being very simple but make the UI less laggy. I think we might want to consider this at least by adding such a section as a placeholder.
Reflow still sucks though - we'd need to basically iterate over all the marks as | ||
we're reflowing, to make sure we put them into the right place in the new | ||
buffer. That is super annoying. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reflow should not be a (realistic) performance problem either IMO. The problem is that we need to associate marks with rows during the reflow, which either downright forces us to store marks in our rows (instead of a separate vector) or store them sorted. This is because in the future we'll copy entire strings of text from the old to the new buffer, which means that all we have is a row/y coordinate and two x coordinates: Where the copied text started and ended. From these 3 numbers we need to get the range of matching marks and translate them to the new text buffer, just like how we translate and copy TextAttribute
s.
I'm also open to alternative ways of drawing the marks, vs just.... 9000 |
I think the most performant solution would be for us to draw our own scrollbar as part of Alternatively, we could write a custom WinUI control, create a simple, 1px wide bitmap with Direct2D and stretch it onto our WinUI control. Then we overlay that control on top of the scrollbar. That's about 30 lines of code - if you were to use Direct2D and DirectComposition directly. I have no idea however how to actually create a custom control for WinUI. There's no documentation for it and all the issues over on the repository that did talk about it found no answer either. Does WinUI even support using the underlying Direct2D? I know that Win2D exists, but that can't possibly be the answer, right? That one creates an entire swap chain per surface. Compared to that I think 9000 rectangles are faster and simpler. 🫤 So, I think we should just focus on downsampling the marks. For instance, even on my 4K display my 30 row window is only about 900px tall and thus we could reduce the cost of marks by up to 10x. If the marks are sorted by scrollbar position, the downsampling could occur with a simple loop where we ignore marks if they touch the same pixel as a previous mark (for instance). |
I'm not sure this needs to go in the spec, but just FYI, there were a few other operations supported by Final Term that aren't listed here. For example, So from the user's perspective, you would go into a directory, execute an There was also a progress operation, Various other terminals have added their own proprietary |
This comment has been minimized.
This comment has been minimized.
`autoMarkPrompts` setting (which auto-marks <kbd>enter</kbd>) as the _end of the | ||
prompt_. That would at least allow cmd.exe to emit a {command finished}{prompt | ||
start}{prompt...}{command start} in the prompt, and have us add the command | ||
executed. It is not perfect (we wouldn't be able to get error information), but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also do a mini spec for "precmd" and "postcmd" CMD hooks if you want ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HKCU\Software\Microsoft\Command Processor\precmd
HKCU\Software\Microsoft\Command Processor\postcmd
*shudder*
### Actions | ||
|
||
In addition to driving marks via the output, we will also want to support adding | ||
marks manually. These can be thought of like "bookmarks" - a user indicated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh. I don't mind them being similar. As long as we refer to them consistently as "Prompt Marks"?
### Actions | ||
|
||
In addition to driving marks via the output, we will also want to support adding | ||
marks manually. These can be thought of like "bookmarks" - a user indicated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since they're really not bookmarks in all cases (since they can be emitted by the app)
* [ ] marks should be stored in the `TextBuffer` | ||
|
||
## Future Considerations | ||
* adding a timestamp for when a line was marked? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh, very interesting
the margins | ||
* Marks are currently just displayed as "last one in wins", they should have a | ||
real sort order | ||
* Should the height of a mark on the scrollbar be dependent on font size & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should, why not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a few comments from last time that still aren't resolved.
For the default value discussions, I'm open to figuring that out when the PRs are being reviewed. Just let me know if that's what you prefer and I'll resolve them.
- [x] `color`: a color for the scrollbar mark. (in [#12948]) | ||
- [ ] `category`: one of `{"prompt", "error", "warning", "success", "info"}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea: could we add a theme (or maybe a setting in the appearance config?) that defaults the category to a color?
For example:
- "prompt" --> white
- "error" --> red
- "warning" --> yellow
- "success" --> green
- "info" --> white
It'd be really annoying for the user to have to update each addMark
action separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, I just use the color values from the color scheme, so that they feel seamless. So error
is the red from the color palette. Does that seem good?
## Summary of the Pull Request > ## Abstract > > Multiple related scenarios have come up where it would be beneficial to display > actionable UI to the user within the context of the active terminal itself. This > UI would be akin to the Intellisense UI in Visual Studio. It appears right where > the user is typing, and can help provide immediate content for the user, based > on some context. The "Suggestions UI" is this new ephemeral UI within the > Windows Terminal that can display different types of actions, from different > sources. > ## Detailed Description of the Pull Request / Additional comments _\*<sup>\*</sup><sub>\*</sub> read the spec <sub>\*</sub><sup>\*</sup>\*_ Similar to #14792, a lot of this code is written. This stuff isn't checked in though, so I'm presenting formally before I start yeeting PRs out there. ## PR Checklist - [x] This is a spec for #1595. It also references: * #3121 * #10436 * #12927 * #12863
Summary of the Pull Request
Detailed Description of the Pull Request / Additional comments
*** read the spec ***
In all seriousness, I've already implemented a pile of this. This is just putting the finishing touches of formalizing it.
PR Checklist