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

Allow unmanaged output above the app in standard renderer #249

Merged
merged 12 commits into from Jun 22, 2022

Conversation

Adjective-Object
Copy link
Contributor

@Adjective-Object Adjective-Object commented Feb 28, 2022

Addresses #179

  • adds Println and Printf to tea, which will log a message above the tea app fold on the next flush
  • consolidates consecutive cusrosrUp() ANSI sequences in flush()
  • adds cursorUpBy to support multi-line jumps

@Adjective-Object Adjective-Object changed the title Println renderer Add Println message for the standard renderer Feb 28, 2022
}
r.linesRendered = numLinesThisFlush
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newLines might be greater than the size of the appliation, so we can't use it as a loop tracker anymore for the above loop.

Might be worth renaming linesRendered to something like "linesInAppOnLastFlush", or "appHeight" or something similar to avoid confusing the size of the current render with the size we're considering the bubbletea-controlled area of the terminal.

@Adjective-Object Adjective-Object changed the title Add Println message for the standard renderer Allow unmanaged output above the app in standard renderer Feb 28, 2022
@meowgorithm
Copy link
Member

Thanks for this! I’m just getting into it; excited to give it a look.

standard_renderer.go Outdated Show resolved Hide resolved
@Adjective-Object
Copy link
Contributor Author

Another thing missed while porting to the standard renderer: we need to request a repaint when a message comes in

@meowgorithm
Copy link
Member

Good catch. Still testing, but so far it's really good. I'm going to push an example to your branch.

@meowgorithm
Copy link
Member

Oh also, Println and Printf will need to be implemented as Cmds (i.e. a func() Msg). I can make that edit.

@Adjective-Object
Copy link
Contributor Author

Adjective-Object commented Feb 28, 2022

What's the intent behind making them Cmds? I see the argument for the renderer generally being controlled by the app state, but in this case since Println is something we explicitly want to not be tracked in the app state, I don't see the motivation.

As a Cmd, if you want to log something from outside the app's render loop you'd have to send a custom message to your app, handle that message in your Update, and return the Println command from your Update loop. I think that means it forces an app render on each Println and an additional switch/case + message event in the client app's Update function.

@meowgorithm
Copy link
Member

It would be easy enough to add some logic to not send messages produced by Println/Printf commands through the program as well as a not have it trigger a repaint. As a Cmd printing would sit well with other “system-level” operations like Quit and EnterAltScreen.

func Update(msg tea.Msg) (tea.Model, tea.Cmd) {
    // Press any key to send a print
    if _, ok := msg.(tea.KeyMsg); ok {
        return m, tea.Println("Hello!")
    }

    return m, nil
}

@Adjective-Object
Copy link
Contributor Author

Alrighty, seems reasonable. My specific scenario only cares about pushing updates from outside the app Update loop, so I suppose it's reasonable that might warrant some additional plumbing.

Is there a way to push a cmd from outside the Update loop? I think I could program.Send(Println("Hello!")()), but that strikes me as a bit of a hack.

@meowgorithm
Copy link
Member

meowgorithm commented Feb 28, 2022

Ah, I see where you're coming from. So for that case like that we may as well add methods on Program as well:

func (p *Program) Println(string)

func (p *Program) Printf(string, ...string)

These would be in-line with the Program.Quit helper function. Pushing my small changes and an example in a moment.

@Adjective-Object
Copy link
Contributor Author

This might be a windows termnial only bug, but I noticed an off-by-one error where the skipLines logic was checking the wrong the content at line n-1 when the cursor was on line n.

I think this is because the cursor was skipping back by 1 at the beginning of the printout.

Kind of wish there were a better way to protect this with tests! At any rate, I pushed the fix.

@meowgorithm
Copy link
Member

Thanks! Yeah, this is definitely a hard one to test. Because it affects something as core as the renderer we'll need to manually check it pretty thoroughly. In my experience, at least, most terminals tend to behave similarly, with the main outlier being the macOS stock terminal which is missing support for a few ANSI sequences.

@Adjective-Object
Copy link
Contributor Author

Looks like the fix caused an off by one in the other direction.

The sequence compresion code is the root of the problem so I've reverted it for now, along with cursorUpBy. Maybe I'll take another shot at it in a separate change.

@Adjective-Object
Copy link
Contributor Author

Adjective-Object commented Mar 1, 2022

Going to keep building my project against this branch and see if I can surface any issues. So far it's been stable since 1efd40f.

@meowgorithm
Copy link
Member

Sounds good. This will definitely need some good testing before we can merge it.

One question (haven't been able to ascertain it from reading the code yet) is the support for having the renderer completely ignore lines still present?

@meowgorithm meowgorithm added the enhancement New feature or request label Mar 1, 2022
@Adjective-Object
Copy link
Contributor Author

Adjective-Object commented Mar 1, 2022 via email

@meowgorithm meowgorithm mentioned this pull request Mar 26, 2022
@meowgorithm
Copy link
Member

Hey there, @Adjective-Object. Just wanted to check and see if you ran into any more issues with this one. We'll plan on testing a bit more as well: this will be a good one to get in before it gets too stale.

@Adjective-Object
Copy link
Contributor Author

Nope, I've been using this for a few weeks on an internal project and haven't run into any major issues.

@meowgorithm
Copy link
Member

Okay, awesome. We'll look into this one a bit more then. We need to be fairly thorough we're touching the renderer; that's really the only concern.

@maaslalani
Copy link
Member

This PR is looking great! I just tested all (29) of the examples in examples/ (on MacOS + Kitty inside tmux) with the new changes and everything works great so I do believe that it doesn't cause anything to break.

And the package-manager example works really well :)

@meowgorithm
Copy link
Member

Just rebased on master 〜 this one seems good to go.

@meowgorithm meowgorithm merged commit ebabec7 into charmbracelet:master Jun 22, 2022
@meowgorithm
Copy link
Member

Thanks again for your help with this feature, @Adjective-Object. It's something we've had our eyes on for a long time. I'm looking forward to releasing this one.

This was referenced Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants