fix: Input Tables cannot paste more rows than number of visible rows#2152
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2152 +/- ##
==========================================
+ Coverage 46.62% 46.67% +0.05%
==========================================
Files 685 692 +7
Lines 38493 38623 +130
Branches 9589 9628 +39
==========================================
+ Hits 17948 18028 +80
- Misses 20535 20584 +49
- Partials 10 11 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
A case that's needs the failure case cleaned up: If you copy multiple columns, and then paste in the right-most column you get an error. e.g.
from deephaven import empty_table, input_table
copy_here = empty_table(1000).update(["X=i", "Y=-i"])
paste_here = input_table(init_table=empty_table(1).update(["X=0", "Y=0", "Z=0"]))
Copy from copy_here, then paste in column Z in paste_here. The error thrown is an actually a TypeError, when we should be throwing an error saying the Copy/Paste area are not the same size.
| (GridRange.rowCount([range]) === tableHeight && | ||
| GridRange.columnCount([range]) === tableWidth) |
There was a problem hiding this comment.
Why are we checking if row/column count are the full table size? Need to update the comment above this to reflect what we're checking (since it just says if each cell is a single selection).
There was a problem hiding this comment.
Removed - stale check.
| return false; | ||
| } | ||
|
|
||
| // Editing the aggregations/totals which are floating above/below is not allowed |
There was a problem hiding this comment.
As mentioned on our previous call, I think we still need this check - we need to make sure we don't paste and edit the aggregation row. If you have an input table and Add Aggregation, right now you can edit that aggregation row (though it's also not displaying the correct value when placed on the Bottom, which is another bug which should be fixed).
There was a problem hiding this comment.
Having this check will automatically fail cases where the pasting size is larger, given that the endRow will always be larger than the topRowCount + table size + pendingRowCount -- which is why I had initially opted to remove it, and then after we had spoken, I had tested further, but had not re-added it. With regards, to the aggregation row, is there any other way we can tell if a row is an aggregation row, and then based on that, I can have a check to basically check if the endRow is going beyond the aggregation row?
With regards to the separate bug you identified, as we discussed in call, I had created a seperate ticket for that: #2156
There was a problem hiding this comment.
Yea in terms of aggregation row we have a bit of a conflict there... it uses floatingBottomRowCount when aggregations are pinned to the bottom, and assumes the aggregations are the last rows in the table; but with these pending rows we actually want an "infinite" number of rows/always able to scroll. One of the downsides of the design of floating rows (as opposed to having it as a separate "table" of data for just the floating rows or something).
I think we'll just need to address that with #2156, but I won't hold it up for this (since the aggregations don't appear correctly already anyway).
3 participants
Resolves #2089