Support rearranging layers with hotkeys #271

ajweeks · 2021-07-15T16:18:18Z

Closes #238

This change is

core/editor/src/document/document_message_handler.rs

core/editor/src/input/input_mapper.rs

client/web/src/components/widgets/inputs/MenuBarInput.vue

core/editor/src/document/document_message_handler.rs

core/editor/src/input/input_mapper.rs

client/web/wasm/src/document.rs

TrueDoctor · 2021-07-16T12:13:29Z

Oh, and in general, feel free to add new commits instead of just amending them, everything will be squashed in the end anyway, it just makes it easier for us to keep track of the changes. (even tough I fully empathize with not wanting to write commit messages)

ajweeks · 2021-07-16T16:04:36Z

This is why I force pushed before, I think I messed up the rebase 🤦‍♂️

README.md

client/web/vue.config.js

core/document/src/document.rs

.vscode/settings.json

core/document/src/layers/folder.rs

core/document/src/layers/mod.rs

core/editor/src/document/document_message_handler.rs

ajweeks · 2021-07-17T09:02:24Z

Thanks for the merge help @TrueDoctor

ajweeks · 2021-07-17T09:02:52Z

Yeah, that code will change imminently though since I'm adding support for moving multiple layers now anyway

core/editor/src/document/document_message_handler.rs

client/web/src/components/widgets/inputs/MenuBarInput.vue

core/editor/src/input/input_mapper.rs

Keavon · 2021-07-18T09:26:53Z

Looks good, thanks for making the changes. Is this ready for a final review or do you need to handle more edge cases or add features or tests or something? Let me know if you need any help completing it.

core/document/src/layers/folder.rs

ajweeks · 2021-07-18T10:07:19Z

I hadn't considered testing, that's not a bad idea though, I've added a few. Let me know if they're sufficient.

I think it's ready for a final review now, it's working as I'd expect it to.

TrueDoctor · 2021-07-18T10:18:14Z

I hadn't considered testing, that's not a bad idea though, I've added a few. Let me know if they're sufficient.

I think it's ready for a final review now, it's working as I'd expect it to.
That's nice, will get right to reviewing

core/document/src/document.rs

TrueDoctor · 2021-07-18T10:28:02Z

core/document/src/layers/folder.rs

+		let target_pos = self.layer_ids.iter().position(|x| *x == target_id).ok_or(DocumentError::LayerNotFound)?;
+
+		let mut last_pos = source_pos;
+		for layer_id in &source_ids[1..source_ids.len()] {


can you do something like source_ids.take(source_ids.len())? might be a bit more ideomatic

That isn't equivalent code, the slice excludes the first element intentionally. If there's a better way of doing that I'm all ears.

I suppose you could call skip(1) but at that point it might get longer than your current code.
Oh, wait, you are indexing source ids with source ids, in that case it should either be source_ids.skip(1) or &source_ids[1..]

The nice thing about the skip is, that it does not panic if the list is empty but that depends on what you want the code to do

Ah I see. The caller definitely shouldn't be passing in an empty list (moving a list of zero layers makes no sense).

I think &source_ids[1..] is probably the best solution here.

core/document/src/layers/folder.rs

core/editor/src/document/document_message_handler.rs

ajweeks · 2021-07-18T13:54:48Z

Looks great, thanks for adding the tests! (This is the kind of thing that can be trickier to get right so testing is indeed helpful). My only comment about the tests is just a nitpick about the comments: it took me about a minute of understand what you meant by '' until I realized that's the "same as above" indicator, my mind was thinking along the lines of single-quoted strings or anything else. If you get the chance, could you consider moving the comment to the line above the one it refers to (typically it's better stylistic practice for descriptive comments, rather than TODOs, to be on its own line) so the comment refers to the block of following lines, and the '' mark is therefore unnecessary? I really don't intend to be a stickler over comments or style like that, so please feel more than free to ignore this comment, it's truly a nitpick I'm totally happy to ignore.

Yep, that's totally fair, I agree it wasn't the clearest.

core/document/src/document.rs

core/document/src/layers/folder.rs

Fix formatting

* Support moving single layers * Fix "Move layer to top/bottom" keybinds * Rename things named "move" to "reorder" Fix formatting * Combine sorted layer helper functions * Use integer consts for moving layers to front/back * Fix merge mistake * Fix some clippy lints * Fix panic * Remove "get" prefix from functions * Bring layer menu items out to sub-menu * Support moving multiple layers at a time * Add comment explaining odd keybinding * Add reordering tests * Add negative test * Add new error type * Add layer position helper, clean up tests * Make position helper return Result * Clean up slice iteration * Simplify source_layer_ids computation Co-authored-by: Dennis Kobert <[email protected]>

TrueDoctor reviewed Jul 15, 2021

View reviewed changes

core/editor/src/document/document_message_handler.rs Show resolved Hide resolved

ajweeks commented Jul 15, 2021

View reviewed changes

core/editor/src/input/input_mapper.rs Outdated Show resolved Hide resolved