perf: clipboard and move cells #3072
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## qa #3072 +/- ##
==========================================
- Coverage 89.51% 89.48% -0.04%
==========================================
Files 429 429
Lines 86670 86950 +280
==========================================
+ Hits 77583 77805 +222
- Misses 9087 9145 +58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I think I did a thorough job reviewing here. Approving.
Leaving on user test tag. Would like @jimniels to spend a few minutes in it as well.
|
@davidkircos final set of eyes that aren't mine before merging would be good |
quadratic-core/src/controller/execution/execute_operation/execute_data_table.rs
Outdated
Show resolved
Hide resolved
| ); | ||
| SetTopLeftCoordinate(xyToA1(topLeftCoordinate.column, topLeftCoordinate.row)); | ||
| }, WAIT_TO_SNAP_TIME); | ||
| const topLeft = pixiApp.viewport.getVisibleBounds(); |
|
|
||
| let old_size = sheet.offsets.set_default_width(size); | ||
|
|
||
| if (cfg!(target_family = "wasm") || cfg!(test)) && !transaction.is_server() { |
There was a problem hiding this comment.
Consider moving to the wasm dir
There was a problem hiding this comment.
there is more of this in the same file, needs a comprehensive refactor to isolate wasm calls
| let old_size = sheet.offsets.set_default_width(size); | ||
|
|
||
| if (cfg!(target_family = "wasm") || cfg!(test)) && !transaction.is_server() { | ||
| if let Some(sheet) = self.try_sheet(sheet_id) { |
There was a problem hiding this comment.
I think we have sheet already from link 287?
There was a problem hiding this comment.
rust complains if I use the previous one
cannot borrow `self.a1_context` as immutable because it is also borrowed as mutable
immutable borrow occurs here
| ) { | ||
| unwrap_op!(let DefaultRowSize { sheet_id, size } = op); | ||
|
|
||
| let Some(sheet) = self.try_sheet_mut(sheet_id) else { |
There was a problem hiding this comment.
This feels like an anti-pattern (I know, it's everywhere). Consider returning a result here since we have some mechanism for capturing the result in other exec files.
There was a problem hiding this comment.
we want this to fail silently, this is being done differently in different fns, needs a comprehensive refactor to make it uniform
| @@ -205,6 +205,11 @@ impl GridController { | |||
| Operation::MoveRows { .. } => self.execute_move_rows(transaction, op), | |||
| } | |||
| } | |||
| #[cfg(feature = "show-operations")] | |||
| if cfg!(target_family = "wasm") { | |||
There was a problem hiding this comment.
Consider moving to the was dir
fix format summary behind tables
Relevant issue(s)
closes #2894
Testing
Try Cut, Copy, Past, Select Move