-
Notifications
You must be signed in to change notification settings - Fork 56
mps.tables: Support copying, pasting and deleting of table selections #1598
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
base: maintenance/mps20232
Are you sure you want to change the base?
mps.tables: Support copying, pasting and deleting of table selections #1598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- CopyPasteSupport#setTable should be called setTableNode.
- In TableSelectionActionMap there are multiple calls to support.setTable(node) that probably can be removed (e.g. this one)
- TableCopyPaste: should it rather have an
isApplicable(node)
method thangetConcept()
? What if it's also applicable to subconcepts or some nodes but not others? - I'm not quite sure about classloading issues with TableCopyStorage being an IDEA lightweight service. What's the benefit as opposed to having a static variable? The comment says the data will be preserved after MPS exit but I don't see how it's achieved.
- The storage should probably be cleared on copy-paste (including from some other application) and possibly when the nodes are unloaded? And/or it probably should store SNodeReferences rather than SNodes?
- TableData.iterateRowwiseWithGaps looks complicated but also unused?
- Can we make copy-pasting a default behavior so that action maps are only required to override it with something custom?
- Would it be possible to implement node-to-text transformation for copying based on editor cells' renderText() instead of node presentation? This combined with the previous point would make copy-paste work for many tables out of the box.
|
9a5f39d
to
e9995b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like UndirectedTableRangeSelection extending from TableRangeSelection. I don't see the benefits of this. I'd rather go the other way and replace it with a TableRange class that's just a rectangle.
The change from row/column start/end to two intervals is problematic because the intervals are now mutable, making the selection mutable as well, whereas it was immutable before.
Instead of passing SRepository
to methods like CopyPasteSupport.paste
or nodeDataToStringData
let's pass EditorContext. Also perhaps we should pass EditorContext to the new isApplicable methods? And in general to all methods that may be defined by the user, such as CopyPasteSupport#copy
or #delete
.
DataTransformer.isApplicate → isApplicable.
I was initially skeptical about the approach with the default action map. The pattern used so far is generating calls to EditorCell.setAction if there is no action map or the action map does not have a particular action defined (example). But I think in the end the default action map is a cleaner solution so let's keep it. The linked code does not take into account action map imports, for example.
Please also update the changelog to match the current state of the changes. And instead of 'In Addition' split the entry into two for clarity.
Tables now support copying, cutting and deleting when multiple cells are selected with the mouse. Add the action map TableSelectionActionMap in the inspector of the table and implement the extension point TableCopyPaste to support these features for a specific table. In Addition, two new intention are available for tables that implement the extension point DataTransformation which allow parsing table data in textual form (comma- or tab-separated) and paste it to table (e.g., 10 as a number literal).
Note: This feature was initially implemented by @svott7. I moved it to MPS-Extensions and modified it a bit to be more generic.