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

Discussion: Class-level expectSize or method-level expectSize. #1320

Closed
jscarle opened this issue Feb 13, 2024 · 3 comments · Fixed by #1322
Closed

Discussion: Class-level expectSize or method-level expectSize. #1320

jscarle opened this issue Feb 13, 2024 · 3 comments · Fixed by #1322
Milestone

Comments

@jscarle
Copy link
Contributor

jscarle commented Feb 13, 2024

@WojciechNagorski @Rob-Hague

As it stands, there are two public APIs for setting the expectSize buffer.

public ShellStream CreateShellStream(string terminalName, uint columns, uint rows, uint width, uint height, int bufferSize, int expectSize)

public ShellStream CreateShellStream(string terminalName, uint columns, uint rows, uint width, uint height, int bufferSize, int expectSize, IDictionary<TerminalModes, uint> terminalModeValues)

By default, unless specified otherwise, the expectSize will default to 2 * bufferSize. Since bufferSize defaults to 1024, that means expectSize defaults to 2048. As a default, that should cover most uses cases considering a full 80 x 25 screen is 2000 characters.

Should we remove the two public APIs that I originally added in #1207 before the next release in order to prevent any dependencies on it and allow us to rework it as a method-level parameter?

@Rob-Hague
Copy link
Collaborator

Let me get my changes up for the remaining test fixes and we can discuss it then. So far the implementation looks quite different.

@jscarle
Copy link
Contributor Author

jscarle commented Feb 13, 2024

I'm adding this as a note to the conversation so I don't forget.

Implementing this at the method level would require hydration of the expect queue based on the data already contained in the incoming queue in order to produce a synchronous sliding window that parallels the existing data.

Its definately feasible, I'd just have to compare performance using benchmarks to make sure there's a gain and not a deterioration versus a class-level fixed size.

@WojciechNagorski
Copy link
Collaborator

This issue has been fixed in the 2024.0.0 version.

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 a pull request may close this issue.

3 participants