Copy and paste layers MVP

[tillarnold]

corresponding issue: #218

@TrueDoctor I'm not sure if this is what you imagined here so I created this PR in a bit of an unfinished state to ask for some feedback. Since the copy buffer is in the editor I think I need to send Layers wrapped as operations and since operations are serializable I needed to make Layers serializable too. And in turn I needed to enable to serde feature of kurbo. I’m not sure if this is desired.

Also I think there is a problem with the keybindings: I had to add the control-V shortcut to the beginning of the mapping! because otherwise the SelectTool(ToolType::Select) message bound to V did take precendence.

Furthermore the way I select the active layers in CopySelectedLayers (which I think is the same way we do it in DuplicateSelectedLayers and DeleteSelectedLayers) is flawed as it appears to also take into account some sort of temporary layers. This is why I added the warn log in process_action for CopySelectedLayers. To reproduce: create a rect and then press control-C which correctly copies the rect and logs a debug message to the console. Now just click the canvas with the rect tool which will not create a new layer and then press control-C. You’ll now see the warning message in the console indicating that layer_data contained a selected layer which does not actually exist in the real document layers.

---

This change is 

[TrueDoctor]

@TrueDoctor I'm not sure if this is what you imagined here so I created this PR in a bit of an unfinished state to ask for some feedback. Since the copy buffer is in the editor I think I need to send Layers wrapped as operations and since operations are serializable I needed to make Layers serializable too. And in turn I needed to enable to serde feature of kurbo. I’m not sure if this is desired.

Yes this is the messiness I was talking about, it is not ideal. One option would be not turning on serialization and just storing the layer as a byte array but that gets messy as soon as we have heap allocations (not sure if kurbo uses any)

Also I think there is a problem with the keybindings: I had to add the control-V shortcut to the beginning of the mapping! because otherwise the SelectTool(ToolType::Select) message bound to V did take precendence.

This is atm by design

Furthermore the way I select the active layers in CopySelectedLayers (which I think is the same way we do it in DuplicateSelectedLayers and DeleteSelectedLayers) is flawed as it appears to also take into account some sort of temporary layers. This is why I added the warn log in process_action for CopySelectedLayers. To reproduce: create a rect and then press control-C which correctly copies the rect and logs a debug message to the console. Now just click the canvas with the rect tool which will not create a new layer and then press control-C. You’ll now see the warning message in the console indicating that layer_data contained a selected layer which does not actually exist in the real document layers.

Will have to look into that

[tillarnold]

Question: Is there already code that receives the selected layers in order/sorts layers by their order in the document or should I implement such a thing as part of this PR?

[TrueDoctor]

Question: Is there already code that receives the selected layers in order/sorts layers by their order in the document or should I implement such a thing as part of this PR?

such does currently not exist. This also bears the question of whether we should honor the folder structure when copy pasting (paste the layer into a folder structure that resembles the location they were copied from)

[Keavon]

such does currently not exist. This also bears the question of whether we should honor the folder structure when copy pasting (paste the layer into a folder structure that resembles the location they were copied from)

No, except when the layers that are selected are the groups. Let's think of groups as layers which automatically include their children. If you select various layers (some shapes, some groups with their children included), then that set of selected layers will be pasted in a flat structure ordered from top to bottom as they would have been read by scrolling through the layer panel (i.e. a depth first traversal, I think?). Regardless of the folder topology that they are copied from, they end up in a flat list where you paste them, but when groups are selected then those are also pasted flatly as groups with their children included. If a group and some of its children are selected, I think it's the same story and you end up with multiple copies of the children, flat and within the folder.

[TrueDoctor]

No, except when the layers that are selected are the groups. Let's think of groups as layers which automatically include their children. If you select various layers (some shapes, some groups with their children included), then that set of selected layers will be pasted in a flat structure ordered from top to bottom as they would have been read by scrolling through the layer panel (i.e. a depth first traversal, I think?). Regardless of the folder topology that they are copied from, they end up in a flat list where you paste them, but when groups are selected then those are also pasted flatly as groups with their children included. If a group and some of its children are selected, I think it's the same story and you end up with multiple copies of the children, flat and within the folder.

Just keep in mind that this might create issues with blend modes. In that case layers might look differently when pasting them which might be confusing

[Keavon]

Blend modes really screw up most approaches, don't they? That said, I think that is fine— there have to be compromises somewhere if you are effectively reordering things in a random way that removes them from their hierarchy.

[tillarnold]

Am I correct in my assumption that currently folders and groups cannot be created from the GUI? Should i try to implement this and then write some tests (since manual testing with the GUI is unavailable)?

[Keavon]

That is correct. And automated tests would always be excellent additions!

If you are so inclined, adding folders to the frontend (#149) is an unclaimed issue that might be a good next issue once your current pipeline is free. The core of the issue is a moderately simple algorithm but its interplay with other features will take some care. For example it'll make rearranging layers (#141) more difficult, once that is implemented. Another edge case is document tab switching.

[TrueDoctor]

That is correct and is currently tracked here: #149
It would be awesome if you could implement that and also make testing easier.
It should actually not be too hard to do so. you have to change how the frontend stores layers to insert the expand messages into a tree structure and add some left padding for indented layers

[TrueDoctor]

wow same brain xD

[tillarnold]

So I added some tests. I had to add them to dispatcher.rs instead of making them integration tests because I needed access to the document_message_handler field to make asserts about the document. Is this the style of tests you imagined? If yes I will add some more test as I haven't yet added any tests for layers in folders.

I added a EditorTestUtils trait to make writing these sorts of tests easier. If you think that this is a good way of implementing tests we'd probably want to move EditorTestUtils to its own file to be able to reuse it for other editor tests.

Also I added some initial code to order the layers correctly. My code is horrendous but the overall approach I use is to take a Layer path (Vec<LayerId>) and map it to a path of indices into the tree of layers (Vec<usize>). This is implemented in indices_for_path. I then use that in selected_layers_sorted to sort the layers. Most of the ugliness of selected_layers_sorted comes from me trying to handle the before described case of active_document().layer_data containing layers paths which later cannot be found in the actual Document. If that wasn't the case selected_layers_sorted could probably be implemented in terms of just a sort_by_cached_key. Do you think this sorting approach is overall OK?

Also what I don't fully understand yet is the difference between a Folder and a Group and where Groups are implemented in the code.

[Keavon]

I'll have to defer to @TrueDoctor for his expertise on the other questions.

Also what I don't fully understand yet is the difference between a Folder and a Group and where Groups are implemented in the code.

Folders are Groups should be synonymous. I believe our preference is to call them Folders. This should ideally be standardized to avoid intermixed terminology. Conceptually, a folder is equivalent to a document (the open document is the root folder, and subfolders could also be opened in new tabs) and additionally equivalent to a subgraph (in the node/layer graph panel, you can view folders as child graphs by jumping into the breadcrumb trail of the folder subgraph). In the three different concepts for a document/folder/graph (subdocument/subfolder/subgraph), they can either be edited in situ, in isolation, or independently:

Document:

• In situ: drag things around the parent document viewport and change layer properties, even if they're grouped into a subfolder
• In isolation: a specific element can be isolated in the viewport, dimming the parents/siblings in the background, so it can be edited without the distraction of the (sibling and parent) content
• Independently: a subdocument can be opened and edited in an entirely new tab of the Graphite editor, saved changes show up in the parent unless saved as a separate document

Folder:

• In situ: folders can be modified in the Layer Tree panel (rearranged, properties modified, etc.)
• In isolation: a breadcrumb trail in the Layer Tree panel is shown, and only the isolated subfolder contents are listed in the tree (this is how we see the folder containing your mask while you're creating the selection mask in Mask Mode)
• Independently: Again, open the document in a separate tab

Graph:

• In situ: expand the layer (node) graph for a folder to see its children
• In isolation: jump into the folder's graph in isolation mode, dimming the parent graph in the background, causing a breadcrumb trail to show at the top of the graph viewport
• Independently: Again, open the document in a separate tab

[TrueDoctor]

I'll try to look at the code soon

[Keavon]

@tillarnold to follow up on that explanation with a few more details that you haven't seen yet, here's the design for a document. A Graphite document contains several blocks, including a set of layers and an embedded storage block. The layers form a set that maps an ID (always incremented within the document, never reused), the name and other metadata for the layer, and data of the layer like its various properties and any pointers to referenced data. Those pointers to referenced data are strings that can point to a path on your local file system, a URL to some resource on the internet, a path on the asset store, a path on your local network (useful for studios), and embedded data which is stored in binary format within the Graphite document (.GDD) file in the embedded storage block. That block is just a bunch of data with pointers to it. Either it has a name given by the user or it has a UUID provided automatically for subgraphs/subdocuments. This embedded data section is where actual folders get stored, since they are actually embedded Graphite documents that live within the file. Unless you take one of your folders, save it to its own GDD file on disk, and convert it from embedded to linked, then by default it will be stored internally within the embedded storage block. This works recursively, since an embedded document also has a set of layers block and an embedded storage block.

[TrueDoctor]

I think I know the reason for some of the issues, when using the working folder, the paths are added to the current selection, but don't get removed upon ClearWorkingFolder. We probably need a document response that removes elements from the selection cache which we can then call during the handling of ClearWorkingFolder

[tillarnold]

I tried to merge master into this branch but it appears that some changes make the approach of serializing layer impossible/more difficult because glam::DAffine2 does not appear to implement Serializable?

[0HyperCube]

@tillarnold Use the serde feature flag on glam https://docs.rs/glam/0.17.0/glam/index.html#feature-gates to get the serde derives

[tillarnold]

@0HyperCube I already added the serde feature flag and glam::DVec2 appears to implement Serializable (at least I'm not getting any errors for it). I only get errors for DAffine2.

[tillarnold]

@Keavon just as a status update for you on this issue: Once the code compiles again (when the above problem about DAffine2 not being Serializable is solved) and I get the OK from @TrueDoctor about the overall approach (and the tests) I should be able to implement your desired behavior for Folders and finish writing the test and then hopefully this PR is in a shape where it can be merged.

[0HyperCube]

@tillarnold Sorry for late reply, I've never really used serde before. What you can do is add:

#[derive(Serialize, Deserialize)]
#[serde(remote = "glam::DAffine2")]
struct DAffine2Ref {
    pub matrix2: DMat2,
    pub translation: DVec2,
}

above the Layer struct and then use:

#[serde(with = "DAffine2Ref")]
pub transform: glam::DAffine2,

when declaring the layer

[tillarnold]

@TrueDoctor should the issue with ClearWorkingFolder be solved in this PR or will there be a separate one?

also the CI for my latest commit failed in a strange way. Is this a known issue?

[Keavon]

@TrueDoctor should the issue with ClearWorkingFolder be solved in this PR or will there be a separate one?

also the CI for my latest commit failed in a strange way. Is this a known issue?

The CI can occasionally break before it actually runs the code. In those cases, it's best to just rerun it. If you don't have permission to trigger a rerun, you can either ping me or True Doctor to rerun it, or make some trivial changes (hopefully improvements, maybe add some useful comments?) and push those to cause it to rerun.

[TrueDoctor]

@TrueDoctor should the issue with ClearWorkingFolder be solved in this PR or will there be a separate one?

also the CI for my latest commit failed in a strange way. Is this a known issue?

Up to you

[tillarnold]

@TrueDoctor the code in this PR currently handles the additional elements in the selection cache so do you think we could first get this PR ready to merge and tackle the ClearWorkingFolder problem later? If yes I think this is ready for review now.

[TrueDoctor]

Technically it would be cleaner to remove that special case handling but I don't ha a strong opinion on that issue

[Keavon]

By the way @tillarnold, I'm happy to merge this once you've had a chance to look at my recent comments. Please let me know if there's anything else I can do to help you!

[tillarnold]

@TrueDoctor @0HyperCube one small issue: #230 removes PartialEq for Layer but since Operation is PartialEq and PasteLayer contains a Layer this breaks this PR. Should I readd PartialEq for Layer? Or is there a better solution?

[Keavon]

We can easily revert #230 but let's wait to hear @0HyperCube's thoughts first on the best path forward.

[0HyperCube]

one small issue: #230 removes PartialEq for Layer but since Operation is PartialEq and PasteLayer contains a Layer this breaks this PR. Should I readd PartialEq for Layer? Or is there a better solution?

@tillarnold Solved by removing PartialEq for operation in #232

[Keavon]

@tillarnold #232 was merged so you should be unblocked now. If not, please let me know ASAP and I'll do what I can to make sure your needs are met. So I'm up-to-date with the status, how much work is left to do on this before we're ready to merge?

[tillarnold]

@Keavon if I didn't miss anything i think this should be ready to merge?

[Keavon]

Reviewed 8 of 18 files at r5, 1 of 1 files at r6, 2 of 5 files at r7, 3 of 5 files at r8, 1 of 2 files at r10, 1 of 1 files at r11, 1 of 1 files at r13, 3 of 3 files at r14.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tillarnold)

[Keavon]

Alrighty, merged! Thank you @tillarnold. And for future reference, since I recently added you to the repository, you should be able to delete your GitHub user's fork and work directly on branches in this official repository now. That will also mean that Cloudflare Pages does deployment runs on pushes to your branches in this repo which is helpful during code review.

If you have time this week (before the end of the sprint this Saturday), there are more items (a couple of them are pretty quick: #201 and #219) in the Tasks Board under the Available category.