Add support for setting prompt marks via OSC 133 #5860

kchibisov · 2022-02-06T21:53:26Z

Adds support for setting prompt marks with OSC 133 ; A ST. While
there're other markers for prompt FTCS_PROMPT (A parameter) is
likely the most widely used and useful one.

Fixes #5850.

kchibisov · 2022-02-06T21:53:34Z

@perfbot

perfbot · 2022-02-06T22:06:55Z

chrisduerr

You mentioned on IRC that a Vec might be the best option. Have you tried alternative implementations or is this the first attempt?

kchibisov · 2022-02-07T12:21:26Z

You mentioned on IRC that a Vec might be the best option. Have you tried alternative implementations or is this the first attempt?

I've tried and I don't like it, since it's more code and you have to put it on a grid into scrolling part.

chrisduerr · 2022-02-07T12:30:06Z

I've tried and I don't like it, since it's more code and you have to put it on a grid into scrolling part.

Have you considered how we would implement this feature if we wanted to add all of the possible variations instead of just FTCS_PROMPT? Would you do it the same way and always store it in the first column?

kchibisov · 2022-02-07T12:36:53Z

Have you considered how we would implement this feature if we wanted to add all of the possible variations instead of just FTCS_PROMPT? Would you do it the same way and always store it in the first column?

I think yes, and those will be offsets from the fist column to other prompt marks. It'll just move to extra storage.

CHANGELOG.md

alacritty/src/config/bindings.rs

alacritty_terminal/src/term/cell.rs

chrisduerr · 2022-03-04T06:27:03Z

alacritty_terminal/src/term/mod.rs


        let mut cursor_cell = self.grid.cursor_cell();

+        // Preserve PROMPT_MARK if we had it.


Suggested change

// Preserve PROMPT_MARK if we had it.

// Preserve PROMPT_MARK.

Also is this necessary? The prompt shouldn't usually be overwritten?

It does get overwritten due to the way everyone is sending prompt marks. They send prompt mark and then start writing the prompt, leading to overwriting the cell. The documentation and recommendations state that you should send it before writing the prompt.

Really not a big fan of this aspect of the escape. Either way though this comment should be fixed.

chrisduerr · 2022-03-04T06:28:07Z

alacritty_terminal/src/term/mod.rs

+        // We don't want to clear prompt markers.
+        let prompt_marked = *self.grid[point.line][Column(0)].flags() & Flags::PROMPT_MARK;
+


Is this necessary? Who rewrites a prompt without writing the prompt there?

alacritty_terminal/src/term/mod.rs

alacritty_terminal/src/term/search.rs

alacritty/src/input.rs

Adds support for setting prompt marks with OSC 133 ; A ST. While there're other markers for prompt `FTCS_PROMPT` (`A` parameter) is likely the most widely used and useful one. Fixes alacritty#5850.

chrisduerr

Really my only major gripe left with this is the whole reset logic and I'm not sure there's a great solution for this. It's somewhat of an issue with the escape sequence itself and I'm not confident that just deviating from what everyone else does is a great idea.

chrisduerr · 2022-03-06T03:23:07Z

alacritty_terminal/src/term/cell.rs

@@ -30,6 +30,7 @@ bitflags! {
        const ALL_UNDERLINES            = Self::UNDERLINE.bits | Self::DOUBLE_UNDERLINE.bits
                                        | Self::UNDERCURL.bits | Self::DOTTED_UNDERLINE.bits
                                        | Self::DASHED_UNDERLINE.bits;
+        const PROMPT_MARK               = 0b1000_0000_0000_0000;


Should be above ALL_UNDERLINES. The "special" constants are usually best kept at the bottom because that way one can easily see at a glance that all bits are correctly set for the other constants.

chrisduerr · 2022-03-06T03:24:29Z

alacritty_terminal/src/term/mod.rs

+            self.vi_mode_cursor.point
+        } else {
+            let display_offset = self.grid.display_offset() as i32;
+            // If we're outside of Vi mode use bottom most visible line.


Suggested change

// If we're outside of Vi mode use bottom most visible line.

// If we're outside of Vi mode, use bottommost visible line.

Topmost/bottommost are one word, as weird as it looks.

chrisduerr · 2022-03-06T03:26:18Z

alacritty_terminal/src/term/mod.rs

+        } else {
+            let display_offset = self.grid.display_offset() as i32;
+            // If we're outside of Vi mode use bottom most visible line.
+            Point::new(Line(self.screen_lines() as i32 - 1 - display_offset), Column(0))


Why are you using the bottommost line? If I have a prompt visible at the top and another prompt in the middle of my screen, I want to jump to the prompt in the middle of my screen. I don't want to jump to the next offscreen prompt.

This is the same as search, the top of the screen is always to be considered the origin of the search.

This also shouldn't be inverted for goto_previous. Even there we should still make it based on the topmost line and not switch to the bottommost. We always want our search match aligned at the top of the viewport.

chrisduerr · 2022-03-06T03:27:31Z

alacritty_terminal/src/term/mod.rs

+        }
+    }
+
+    /// Go to previous prompt farthest up in history.


We don't go to the previous prompt farthest up in history, that would mean we skip over as many prompts as possible. We go to the previous prompt in history, so the opposite of farthest. Could say closest/nearest up in history but I feel like that's unnecessary.

chrisduerr · 2022-03-06T03:27:48Z

alacritty_terminal/src/term/mod.rs

+        let point = if self.mode.contains(TermMode::VI) {
+            self.vi_mode_cursor.point
+        } else {
+            // If we're outside of Vi mode use topmost visible line.


Suggested change

// If we're outside of Vi mode use topmost visible line.

// If we're outside of Vi mode, use topmost visible line.

chrisduerr · 2022-03-06T03:29:11Z

alacritty_terminal/src/term/mod.rs


        let mut cursor_cell = self.grid.cursor_cell();

+        // Preserve PROMPT_MARK if we had it.


Really not a big fan of this aspect of the escape. Either way though this comment should be fixed.

chrisduerr · 2022-03-06T03:30:34Z

alacritty_terminal/src/term/search.rs

+        } else {
+            point.column = Column(0);
+        }


Suggested change

} else {

point.column = Column(0);

}

point.column = Column(0);

A bit shorter like this and probably compiles down to the same thing anyway?

kchibisov · 2023-04-15T15:57:34Z

Given that 586f982 (Allow mode-exclusive bindings in any mode) got merged, I'm no longer interested in this escape sequence. Using uniq symbol in a prompt is a good enough solution for me.

kchibisov requested a review from chrisduerr February 6, 2022 21:53

chrisduerr reviewed Feb 7, 2022

View reviewed changes

chrisduerr requested changes Mar 4, 2022

View reviewed changes

kchibisov requested a review from chrisduerr March 4, 2022 11:12

kchibisov force-pushed the osc-133 branch from 04a7f05 to 61dc515 Compare March 4, 2022 11:12

Add support for setting prompt marks via OSC 133

8a49dc2

Adds support for setting prompt marks with OSC 133 ; A ST. While there're other markers for prompt `FTCS_PROMPT` (`A` parameter) is likely the most widely used and useful one. Fixes alacritty#5850.

kchibisov force-pushed the osc-133 branch from 61dc515 to 8a49dc2 Compare March 4, 2022 11:25

chrisduerr requested changes Mar 6, 2022

View reviewed changes

xaverdh mentioned this pull request Oct 20, 2022

Add option to disable text reflow #4424

Closed

partiallyordered mentioned this pull request Oct 27, 2022

[Feature Request] Support marks (or scroll to last command) #725

Closed

kchibisov mentioned this pull request Mar 6, 2023

scrollbar please #6756

Closed

kchibisov closed this Apr 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for setting prompt marks via OSC 133 #5860

Add support for setting prompt marks via OSC 133 #5860

kchibisov commented Feb 6, 2022

kchibisov commented Feb 6, 2022

perfbot commented Feb 6, 2022

chrisduerr left a comment

kchibisov commented Feb 7, 2022

chrisduerr commented Feb 7, 2022

kchibisov commented Feb 7, 2022

chrisduerr Mar 4, 2022

kchibisov Mar 4, 2022

chrisduerr Mar 6, 2022

chrisduerr Mar 4, 2022

chrisduerr left a comment

chrisduerr Mar 6, 2022

chrisduerr Mar 6, 2022

chrisduerr Mar 6, 2022

chrisduerr Mar 6, 2022

chrisduerr Mar 6, 2022

chrisduerr Mar 6, 2022

chrisduerr Mar 6, 2022

kchibisov commented Apr 15, 2023


		let mut cursor_cell = self.grid.cursor_cell();

		// Preserve PROMPT_MARK if we had it.

	// Preserve PROMPT_MARK if we had it.
	// Preserve PROMPT_MARK.

		// We don't want to clear prompt markers.
		let prompt_marked = *self.grid[point.line][Column(0)].flags() & Flags::PROMPT_MARK;

	// If we're outside of Vi mode use bottom most visible line.
	// If we're outside of Vi mode, use bottommost visible line.

	// If we're outside of Vi mode use topmost visible line.
	// If we're outside of Vi mode, use topmost visible line.

Add support for setting prompt marks via OSC 133 #5860

Add support for setting prompt marks via OSC 133 #5860

Conversation

kchibisov commented Feb 6, 2022

kchibisov commented Feb 6, 2022

perfbot commented Feb 6, 2022

chrisduerr left a comment

Choose a reason for hiding this comment

kchibisov commented Feb 7, 2022

chrisduerr commented Feb 7, 2022

kchibisov commented Feb 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisduerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov commented Apr 15, 2023