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

Ensure Dequeue is not attempted if buffer size is 0 #1876

Closed
wants to merge 1 commit into from

Conversation

bruno-f-cruz
Copy link
Contributor

Closes #1875

@glopesdev glopesdev added this to the 2.8.4 milestone Jul 3, 2024
@glopesdev glopesdev added the fix Pull request that fixes an issue label Jul 3, 2024
Copy link
Member

@PathogenDavid PathogenDavid left a comment

Choose a reason for hiding this comment

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

My original intent here was that bufferSize should always be greater than 0, I just didn't enforce it robustly enough.

A better approach I think would be to just enforce that, IE: Replace

bufferSize = (int)((textBox.ClientSize.Height - 2) / lineHeight);

with

bufferSize = Math.Max(1, (int)((textBox.ClientSize.Height - 2) / lineHeight));

That way if the buffer size would've been 0, there's always at least a single entry kept around so it appears when the user resizes the form.

(As it's written right now there's also other places assuming bufferSize is non-zero too so they'd still be broken too.)

@@ -37,7 +37,7 @@ protected override void ShowBuffer(IList<Timestamped<object>> values)
var sb = new StringBuilder();

// Trim old values if bufferSize was reduced
while (buffer.Count >= bufferSize)
while ((buffer.Count >= bufferSize) & (buffer.Count > 0))
Copy link
Member

Choose a reason for hiding this comment

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

Should be &&, which would also make these groupings unnecessary

@bruno-f-cruz
Copy link
Contributor Author

Sounds good! Feel free to edit the PR directly or just open a new one! Thanks for looking into it!

@PathogenDavid
Copy link
Member

Created #1882 (there was one other small change that was needed so it was a good thing I took a closer look.)

@glopesdev glopesdev closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull request that fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ObjectTextVisualizer attempts to Dequeue empty buffer
3 participants