Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Fix for table panic #508

Closed
wants to merge 1 commit into from

Conversation

andschneider
Copy link

Description

This is an initial fix for issue #470.

After walking through one of the cases that caused the panic in a debugger, I found that it was due to a miscalculation (it seems like a rounding issue) in the column widths from the cassowary solver. I don't fully understand the parameters for this solver and as such I didn't feel comfortable trying to implement a fix into the get_column_widths function. Changing the solver also seems to have potential knock-on effects in how all tables are rendered, which sounds like a disruptive change.

So, I tried to make a minimal fix by simply checking for overflow in the set_style function and clamping the value if there would have been an overflow.

I'm not exactly sure if this is a completely robust solution, but it seems to work for the two test cases we have so far.

@fdehau would love your thoughts on how robust of a fix this is!

Testing guidelines

I added the two example tables that were causing a panic from the issue in widgets_table_columns_dont_panic test function. All this test does is render the tables and ensures a panic doesn't occur. It doesn't verify the output of the table.

Checklist

Initial fix for issue fdehau#470
@andschneider
Copy link
Author

So after a little more testing:

for i in 10..1000 {
    test_case(&mut state, table2.clone(), i);
}

I found that with table widths below 100, table2 from the tests still panics sometimes. Not sure why, but this fix is definitely not completely robust. It does work above 100 though, and table1 still does not panic.

@deepu105
Copy link
Contributor

I'll test this branch as soon as I find some time

Copy link
Contributor

@deepu105 deepu105 left a comment

Choose a reason for hiding this comment

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

This seems to work, atleast for the scenarios I tested. but regrdless its better than the current behaviour

@deepu105
Copy link
Contributor

@fdehau WDYT? it would be nice to have this in a patch release

@fdehau
Copy link
Owner

fdehau commented Aug 1, 2021

Thanks for looking into it and sorry for the late response. I'm not very fond of the proposed fix because it can potentially hide a lot of bugs which will be harder to track and fix. Thanks to your provided regression test, I was able to identify an issue in the column widths computation. I've opened this PR which reuses existing code and should fix the issue.

@fdehau fdehau closed this Aug 1, 2021
@andschneider
Copy link
Author

Thanks @fdehau! Glad there was a better solution 💯

@andschneider andschneider deleted the fix/widget-table branch August 3, 2021 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants