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

Don't write on stdout if all commands are queued for stderr #264

Closed
Canop opened this issue Sep 29, 2019 · 23 comments
Closed

Don't write on stdout if all commands are queued for stderr #264

Canop opened this issue Sep 29, 2019 · 23 comments

Comments

@Canop
Copy link
Contributor

Canop commented Sep 29, 2019

When using the Command API and the alternate screen, I'd like to only use stderr for all displays in order to precisely decide what I write on stdout.

It looks like it's almost possible but there are still some chars written on stdout even if I only queue on stderr.

I suspect this is related to the few places where there are calls to write_cout without the first argument.

Could we fix this ? Is it already in process ?

It's possible a few API would have to change, for example for the alternate screen.

@zrzka
Copy link
Contributor

zrzka commented Sep 29, 2019

It can be fixed for sure. It's not in process. I would not worry much about breaking changes, we can always bump the minor. Also there's ongoing discussion / analysis about "1.0" to make the whole crate more mature / usable. From this point of view, it's the right time to say what's wrong, buggy, ... where's room for improvement, ... Result will be major overhaul of the whole crate.

@Canop
Copy link
Contributor Author

Canop commented Sep 29, 2019

As it's a major concern for my broot, the last point on the road to my 1.0, I'm willing to try make the changes myself.

@TimonPost
Copy link
Member

TimonPost commented Sep 29, 2019

Do you know which api calls are doing this?

@Canop
Copy link
Contributor Author

Canop commented Sep 29, 2019

I analyzed the problem starting with the alternate_screen_command example that I submitted.

I found 3 places where stdout is used:

  1. a call to write_cout in AlternateScreen::to_alternate(true)?. We should probably change the API to avoid this.

  2. another one in queue!(stdout, PrintStyledFont(style(astring).with(Color::Red).on(Color::Blue)))?;, due to the reset.

  3. another one in cursor.hide();. I determined that it's my fault, it works if I do queue!(stderr, Hide)?;. I submitted a PR to fix the example.

@Canop
Copy link
Contributor Author

Canop commented Sep 29, 2019

I suppose we could have two commands: EnterAlternate and LeaveAlternate to replace the AlternateScreen::to_alternate(true)?.

The lib user would have to manage the drop of the application but that's not a problem IMO if documented.

@TimonPost
Copy link
Member

TimonPost commented Sep 29, 2019

Reset could be implemented like this:

pub struct Reset;

impl Command for Reset {
    type AnsiType = String;

    fn ansi_code(&self) -> Self::AnsiType {
        ansi::reset_csi_sequence()
    }

    #[cfg(windows)]
    fn execute_winapi(&self) -> Result<()> {
        WinApiColor::new().reset()

        Ok(())
    }
}

I love the idea of those alternate screen commands. I am doubting about the names. Either enable/disable or enter/leave

@zrzka
Copy link
Contributor

zrzka commented Sep 29, 2019

In case of future, like the desire to have multiple alternate screen buffers (not just main & alternate), are these names fine? Isn't something like EnterMainScreen, EnterScreen(u16) a better alternative? Where this u16 will be ignored for now or it can even mean 0 = main, 1 = alternate, >= 2 unsupported now till implemented?

Re Reset ... Shouldn't this command have some suffix? First look at the command name, what it does? Won't be the user puzzled? You can reset colors, cursor position, even size, ...

Once we merge these sub-crates to crossterm (applies even now because they're reexported there), what does Hide mean? Cursor, screen, ...

IMHO we should keep old commands as they're (no breaking changes), but for the new ones, we should start using prefixes / suffixes to make them clear and to make them future compatible to avoid other breaking changes.

Just thinking aloud here, it came to my mind when I was reading this issue ...

@Canop
Copy link
Contributor Author

Canop commented Sep 29, 2019

I agree that exported commands like Show and Hide should be more explicit. The same applies to Reset.

As for alternate screens, is there really a plan to offer more than the standard feature ?

@zrzka
Copy link
Contributor

zrzka commented Sep 29, 2019

My response was based on the #167. But not sure if it's worth the hassle, so Timon has to comment it.

@TimonPost
Copy link
Member

TimonPost commented Sep 29, 2019

Reset

Yes was a very quick draft, let's call it ResetColor ?

Better names commands

Yes, we probably want to change that on the road to 1.0, I will address that comment in that 1.0 idea issue.

Multiple Screens

When I thought about those screen commands the same toughts came up to me. Now I have more time, I will elaborate on that as well. Yes, there is that issue open for supporting multiple screens however that will take some time before that get's in. The question here is how often would people really use more than to screens. Like termion only supports main/alternate as well, and how many people are using that.

Secondly, I don't think that the command API is going to be relevant for the screen issue. This has to do with the idea that a Screen is going to contain specific state of an certain terminal screen buffer. This state could be something like our own stdout buffer etc. Because of this state inside those Screens the user has to keep those around until it is no more needed.

The alternate screen is something supported by terminals and windows/Linux will keep track of main/alternate screen state. It is just a matter for us to execute that ANSI-command and we will set that alternate screen as the active one. However, if we would have our own screens we can't use those ANSI-codes anymore, and we can't simply write an ANSI-code to the terminal to switch to those screens. Therefore we probably want to have functions on Screen it's self like Screen::set_active(&self). We won't be using the alternate screen anymore and will do it all our selves. And thus we are probably not going to do this via the CommandAPI. (hopefully, you understand what I am trying to say here)

Where this u16 will be ignored for now or it can even mean 0 = main, 1 = alternate, >= 2 unsupported now till implemented?

This was my first thought as well, but because of that state in the Screen, and due to the fact that the user has to keep an instance of those screens around as long it wants to use the screens, I can't yet think of a way to move all this logic behind a screen number that will be used to indicate a specific screen. Maybe we want something like a Console type, which has a hashmap of Screens, each of those screens has some identifier. When the user wants to switch to a screen it could use a command like in ToScreen(&console, 3). This command would call console.set_active_screen(3) which would in its turn call Screen::set_active().

That said, I think it is to early to tell specifically if and how we might support multiple screens, for now, I think it would be fine to create those two commands. And just support only two screens. Just note that probably 90% of the users would have enough with one screen and possible a view more than that.

@andersk
Copy link
Contributor

andersk commented Sep 30, 2019

The alternate screen on Linux is more than just a separate buffer. In at least VTE-based terminals like GNOME Terminal, the main screen has a scrollback history that can be scrolled with the scrollbar or Shift+PgUp/PgDn or the mouse wheel; while the alternate screen has no history, a locked scrollbar, passes through Shift+PgUp/PgDn to the application, and reinterprets the mouse wheel as equivalent to arrow key presses (unless the application has requested mouse events). So trying to emulate the alternate screen using the main screen would have a number of unexpected side effects.

If there’s demand for multiple alternate screens, it may make sense to try to emulate them using a single alternate screen. But it seems like it’d be hideously complicated to accurately mirror the state of each screen well enough to switch between them, especially if you think about what should happen when the terminal is resized. And I’m not convinced u16 would be a good API for that—a screen should be an object, not a number.

@Canop
Copy link
Contributor Author

Canop commented Sep 30, 2019

@TimonPost @zrzka Sticking to the topic of this issue, do I let you handle the two remaining points (Reset and Enter/LeaveAlternate) or do you want some help ?

@TimonPost
Copy link
Member

TimonPost commented Sep 30, 2019

@Canop it is the topic we need to think about future changes and how what the api should look like. We have to take the multiple screens support into consideration because it might be related to the Enter/Leave command and if this affects how those commands should look. My conclusion - which was a reaction on @zrzka - was that we probably should not mind that issue for now and we can stick with just those two commands .

@zrzka
Copy link
Contributor

zrzka commented Sep 30, 2019

@Canop there ResetColor was done here crossterm-rs/crossterm-style#2. I can do the EnterAternateScreen / LeaveAlternateScreen commands, but I've got couple of question for @TimonPost:

  • EnterAlternateScreen & LeaveAlternateScreen are the final names?
  • EnterAlternateScreen should be just pub struct EnterAlternateScreen; or should it mimick to_alternate, ie. pub struct EnterAlternateScreen(pub bool) (aka enable raw mode as well)
  • Shouldn't we introduce EnterRawMode & LeaveRawMode commands as well? Or EnableRawMode & DisableRawMode?

@TimonPost
Copy link
Member

TimonPost commented Sep 30, 2019

The reason why we have that boolean for alternate screen is because of the fact that most people using alternate screen are probably going to use raw mode as well. The question becomes: Do we want one command to perform multiple actions. Single action commands will keep the API convenient and the behavior is more predictable. Another thing to notice when we are going to implement those commands is that they can't return anything. Because they are executed in a later stage.

I prefer EnterAlternateScreen/LeaveAlternateScreen and EnterRawMode/LeaveRawMode.

@zrzka
Copy link
Contributor

zrzka commented Sep 30, 2019

I prefer just pub struct EnterAlternateScreen;. Do you want raw? Just go with EnterRawMode as well. Or wrap it with something on your own.

@TimonPost
Copy link
Member

Okay, agree there. Keep it simple stupid.

@andersk
Copy link
Contributor

andersk commented Sep 30, 2019

Entering raw mode should be an operation on the input stream, not the output stream (#188), so I’m not sure it makes sense to shoehorn that into the command API.

@TimonPost
Copy link
Member

That is a valid point, and in fact, there are no ANSI-codes involved and thus Command can't even be implemented right for raw mode.

@zrzka
Copy link
Contributor

zrzka commented Oct 1, 2019

Mea culpa, that's what happens when you're replying and thinking about something else :)

@Canop
Copy link
Contributor Author

Canop commented Oct 1, 2019

While we're discussing naming, can you please rename Show/Hide to ShowCursor/HideCursor ?

@TimonPost
Copy link
Member

@Canop I moved that discussion over here #268

@TimonPost
Copy link
Member

TimonPost commented Oct 15, 2019

I am closing this issue. Commands are added, and naming discussion has been moved to #268

crossterm-rs/crossterm-screen#7
crossterm-rs/crossterm-style#2

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

No branches or pull requests

4 participants