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
Allow unmanaged output above the app in standard renderer #249
Conversation
} | ||
r.linesRendered = numLinesThisFlush |
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.
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.
Thanks for this! I’m just getting into it; excited to give it a look. |
Another thing missed while porting to the standard renderer: we need to request a repaint when a message comes in |
Good catch. Still testing, but so far it's really good. I'm going to push an example to your branch. |
Oh also, |
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. |
It would be easy enough to add some logic to not send messages produced by 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
} |
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 |
Ah, I see where you're coming from. So for that case like that we may as well add methods on
These would be in-line with the |
9660feb
to
87e4f0c
Compare
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. |
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. |
f034fdd
to
94b5738
Compare
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. |
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. |
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? |
Skip lines should still work in the latest version, I'm pretty sure.
…On Tue, Mar 1, 2022, 17:04 Christian Rocha ***@***.***> wrote:
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?
—
Reply to this email directly, view it on GitHub
<#249 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAI62SXRVEGADAG6BSMLVRDU52H7RANCNFSM5PP3LRVQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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. |
Nope, I've been using this for a few weeks on an internal project and haven't run into any major issues. |
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. |
This PR is looking great! I just tested all (29) of the examples in And the |
…ippedLines local to jumpedLines to clarify they are separate comments
1efd40f
to
a3d0fd3
Compare
Just rebased on |
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. |
Addresses #179