Allow for Commands to take visible or all text as input #3709
Conversation
|
r @chrisduerr , since he has this done to some point already in one of his branches. note, I can review that as well, if you don't have time, just let me know. |
|
(Force-pushed to fix |
There was a problem hiding this comment.
I'm wondering if we should support escapes for colors and similar. Especially because then we're going to run into the same problem we ran into with the pager implementation.
Any thoughts @kchibisov? I think if we add this, the question of passing colors is definitely going to come up.
alacritty_terminal/src/grid/mod.rs
Outdated
| let row = &self[y]; | ||
| for x in 0..*self.num_cols() { | ||
| let cell = &row[Column(x)]; | ||
| s.push_str(&cell.to_string()); // TODO: this is not cheap, support for Cell only? |
There was a problem hiding this comment.
Passing a &mut String like s in this case and then writing to it is a lot cheaper than calling to_string().
Since both the primary cell content and the zerowidth chars are accessible, you can just do all the conversion out here rather than inside the cell's methods.
You can see how I've done that here:
https://github.com/chrisduerr/alacritty/blob/pager/alacritty_terminal/src/term/mod.rs#L2152-L2178
Though your implementation is much, much more trivial, since you're not trying to handle escapes.
There was a problem hiding this comment.
In this case the cell content is actually not accessible because I'm only assuming GridCell. Hence the need for ToString, and implementing it for Cell, to be able to do this at all here. Can/should I extend GridCell, or instead impl only for Grid<Cell> here as the comment suggests?
There was a problem hiding this comment.
Could be implemented on top of term, where you know the type.
There are a couple of options available, just a matter of comparing to see which is the cleanest one.
There was a problem hiding this comment.
Sure. I pushed a commit that moves it on top of Term since that seemed the most straightforward — I can squash it later, figured it's better to have the options open for now. I'll leave it up to you to determine what's cleanest as I'm not really familiar with the codebase and what sort of abstraction boundaries you prefer to keep.
It depends how slow it'll be compared to just passing text, I think not that slow, and the slowest part will be opening pager in new instance. |
It's about 50/50. So it'll take over a second and if writing to the child is done from Alacritty, it might be blocking too. |
|
Colour and other escape support is a fair point. For my use cases I don't need it, and in fact would prefer to not have it — or to have an option to disable it for a command — since most external tools don't understand them. You're right though that people will definitely ask for it for pagers and the like. |
To take advantage of the fact that we know it's a Grid<Cell>, so we no longer need to use the expensive Cell::to_string.
|
A few things I'd like to note about this. I do not think we should support colors for it. With 100k scrollback that's just far too inefficient and for most things you might want to do with the scrollback buffer it's more harmful than beneficial. If your goal is navigation, then we can use Search/vi mode instead. So we should have two different options: Copy the entire scrollback buffer as text and copy just the visible region as text. I'm not even sure if the latter is required. Since this is something that can be easily parallelized, we should also try to make use of multiple cores to speed up the collection of text into strings. I'm not sure if I'd go all out and spawn as many threads as OS threads available, but at least a little bit of parallelization should be greatly beneficial to performance since it should scale very well. Maybe even something simple like |
|
Closing this in favor of #4550. |
Fixes #1615. (I finally got around to it!)
There are a few warts here so I don't expect this to be accepted in its current form.
I went with
ToStringas suggested in #1615 (comment), but that doesn't provide clean support for the distinction between "only visible text" and "all text". I also implemented it for allGrid<T> where T: GridCell + ToStringwhich means allocating a lot of small strings, but I'm not sure if it would be acceptable to implement it only forGrid<Cell>. I left a TODO about this because it really feels bad to me as-is.The "visible text" and "all text" naming doesn't seem great, but I don't know what would be better.
I changed
start_daemon, but I've only tested this on Linux i.e. thecfg[windows]one is completely untested.There are no tests for the command-spawning part or the new configuration possibilities, but that seems to be the status quo anyway?