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

Add optional buffered input for terminal #530

Merged
merged 5 commits into from
Mar 5, 2025
Merged

Conversation

monte-monte
Copy link
Contributor

As discussed in discord.

@monte-monte
Copy link
Contributor Author

Forgot to make #define TERMINAL_INPUT_BUFFER 0

if( buffer[r - 1] == '\n' ) new_line = 1;
if( new_line == 0 ) strcpy( print_buf, TERMINAL_CLEAR_PREV ); // Go one line up and erase it
else strcpy( print_buf, TERMINAL_CLEAR_CUR ); // Go to the start of the line and erase it
strncat( pline_buf, (char *)buffer, r ); // Add newely received chars to line buffer
Copy link
Owner

Choose a reason for hiding this comment

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

Please protect the sizes of these buffers from overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I use strncat or better switch to sprintf?

uint32_t appendword = 0;
do
{
uint8_t buffer[256];
#if TERMINAL_INPUT_BUFFER
char print_buf[300]; // Buffer that is filled with everything and will be written to stdout (basically it's for formatting)
Copy link
Owner

Choose a reason for hiding this comment

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

This is not large enough to encapsulate the pline and input buffers.

@@ -195,6 +195,24 @@ struct InternalState
#define DLLDECORATE
#endif

#define TERMINAL_INPUT_BUFFER 1
Copy link
Owner

Choose a reason for hiding this comment

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

No need to ifdef this, but I Do want to hide it behind a flag of some kind. Or maybe a flag to turn it off.

There's a LOT of setups where using terminal colors will just look uuuugly. So it would be better to make sure we aren't on one of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I mentioned I forgot to set it to 0. Or do you mean something else?

@cnlohr
Copy link
Owner

cnlohr commented Mar 4, 2025

How hard would it be to instead make this configurable runtime? I'd love to include this in the binary but you have to use some other flag to invoke it?

@monte-monte
Copy link
Contributor Author

With new commit it now detects file/pipe stdout automagically. BUT it doesn't work with cmd.exe (that thing don't understand terminal formatting, but still identifies as a terminal). So I can either make it as additional flag to add to -T or I can make different behavior for windows/linux.

@cnlohr
Copy link
Owner

cnlohr commented Mar 5, 2025

With new commit it now detects file/pipe stdout automagically. BUT it doesn't work with cmd.exe (that thing don't understand terminal formatting, but still identifies as a terminal). So I can either make it as additional flag to add to -T or I can make different behavior for windows/linux.

I would rather it be a manual on/off thing... Maybe we should wait until we get better command support?

@monte-monte
Copy link
Contributor Author

What do you mean by better command support?
Maybe merge it with default build flag set to 0 while we figuring out the better way to do this, and then switch this to be default when/if you feel it ready?

@cnlohr
Copy link
Owner

cnlohr commented Mar 5, 2025

Sounds good. Let's do that.

@cnlohr
Copy link
Owner

cnlohr commented Mar 5, 2025

Spaces/tabs but I'll merge anyway!

@cnlohr cnlohr merged commit 3d5d3ee into cnlohr:master Mar 5, 2025
83 checks passed
@monte-monte
Copy link
Contributor Author

Yeah sorry for that. I'll fix in other commits.

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

Successfully merging this pull request may close these issues.

2 participants