Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix external consistency of grouping layers so they don't jump to the top of the hierarchy #1627

Merged
merged 8 commits into from
Feb 28, 2024
Merged

Fix external consistency of grouping layers so they don't jump to the top of the hierarchy #1627

merged 8 commits into from
Feb 28, 2024

Conversation

moOsama76
Copy link
Contributor

@moOsama76 moOsama76 commented Feb 25, 2024 •
edited by Keavon

Partly closes #1633 (fixes "Grouping" > "External Consistency").

@moOsama76
Copy link
Contributor Author

Calculate insert_index when grouping multiple selected layers using Ctrl+G shortcut

0HyperCube
0HyperCube requested changes Feb 26, 2024
View reviewed changes
Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and it works well.

I think you might have accidentally committed some unrelated changes though.

editor/src/messages/portfolio/document/document_message_handler.rs Outdated
@@ -1101,7 +1118,7 @@ impl DocumentMessageHandler {
/// When working with an insert index, deleting the layers may cause the insert index to point to a different location (if the layer being deleted was located before the insert index).
///
/// This function updates the insert index so that it points to the same place after the specified `layers` are deleted.
fn update_insert_index(&self, layers: &[LayerNodeIdentifier], parent: LayerNodeIdentifier, insert_index: isize) -> isize {
fn updatecalculated_insert_index(&self, layers: &[LayerNodeIdentifier], parent: LayerNodeIdentifier, insert_index: isize) -> isize {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure why this was renamed? If this was intentional could you please standardise the formatting with underscores between words? Thanks.

frontend/package.json Outdated
@@ -56,7 +56,7 @@
"vite-multiple-assets": "1.2.6"
},
"optionalDependencies": {
"wasm-pack": "0.12.1"
"wasm-pack": "^0.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was probably accidental (web package management can be quite fragile) - would it be possible to revert this change as well as the changes to the package-lock.json files?

@moOsama76
Copy link
Contributor Author

moOsama76 commented Feb 26, 2024 via email

@moOsama76
Copy link
Contributor Author

Mentioned wrong commits fixed

0HyperCube
0HyperCube approved these changes Feb 27, 2024
View reviewed changes
Copy link
Member

@0HyperCube 0HyperCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning that up.

@Keavon
Copy link
Member

Keavon commented Feb 27, 2024

Please discard the removal of package-lock.json from this. And in the future, please be very careful to review every file that you're including in a commit so you know which specific lines you intend to commit (basically a sanity check self-code review before any commit).

@Keavon
Copy link
Member

Keavon commented Feb 27, 2024

!build

@moOsama76
Copy link
Contributor Author

moOsama76 commented Feb 27, 2024 via email

@Keavon
Copy link
Member

Keavon commented Feb 27, 2024

frontend/package-lock.json, please do a self-code review here on GitHub and you'll see it. You can run git restore --source origin/master frontend/package-lock.json and commit it back.

@moOsama76
Copy link
Contributor Author

moOsama76 commented Feb 27, 2024 via email

@Keavon
Copy link
Member

Keavon commented Feb 27, 2024

Yes, that's corrected now. I will code review and QA this soon today.

@moOsama76
Copy link
Contributor Author

moOsama76 commented Feb 27, 2024 via email

@Keavon
Copy link
Member

Keavon commented Feb 27, 2024

Please also fix the CI errors (it looks like a formatting issue) so this can pass and build.

@moOsama76
Copy link
Contributor Author

moOsama76 commented Feb 27, 2024 via email

@Keavon
Copy link
Member

Keavon commented Feb 27, 2024

See the ❌ below the comment section? That's telling you that your code has something wrong with it. Either the tests failed, there's a formatting error, the code can't compile, or something else is wrong. In this case, you need to fix your code formatting. You should set up your editor so it automatically formats your code every time you save your file. This should come as part of our provided VS Code settings if you're using that editor, otherwise you'll need to figure that out yourself if you're using another editor.

@moOsama76
Copy link
Contributor Author

moOsama76 commented Feb 27, 2024 via email

@Keavon
Copy link
Member

Keavon commented Feb 28, 2024

!build

@github-actions GitHub Actions
Copy link

📦 Build Complete for 717e5e5
https://cdb5e66a.graphite.pages.dev

@Keavon Keavon force-pushed the GroupSelectedLayers_insert_index branch from 717e5e5 to 472f51b Compare February 28, 2024 05:56
Keavon
Keavon reviewed Feb 28, 2024
View reviewed changes
editor/src/messages/portfolio/document/document_message_handler.rs Outdated
});
responses.add(PortfolioMessage::PasteIntoFolder {
clipboard: Clipboard::Internal,
parent: LayerNodeIdentifier::new_unchecked(folder_id),
insert_index: -1,
});

let folder_id = NodeId(generate_uuid()); // TODO: Either this is a bug, or its reasoning should be explained in a comment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line looks like it was accidentally copied from above (line 490) and put here. But if it has a purpose, please explain it for me so we can discuss if it should be kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's a mistake

Keavon
Keavon requested changes Feb 28, 2024
View reviewed changes
Copy link
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve the potentially extraneous line I commented on.

@Keavon
Copy link
Member

Keavon commented Feb 28, 2024

I committed a change which improves the code to make it follow idiomatic Rust (avoiding mutation, avoiding needing to break from nested loops, etc.). Please look at the diff so you can learn from it.

@Keavon Keavon changed the title Calculate insert_index when grouping multiple layers Fix external consistency of grouping layers so they don't jump to the top of the hierarchy Feb 28, 2024
@Keavon Keavon mentioned this pull request Feb 28, 2024
Closed
5 tasks
@moOsama76
Copy link
Contributor Author

moOsama76 commented Feb 28, 2024 via email

@Keavon
Copy link
Member

Keavon commented Feb 28, 2024

!build

@github-actions GitHub Actions
Copy link

📦 Build Complete for 10eb738
https://5308cae1.graphite.pages.dev

@Keavon Keavon merged commit 8785836 into GraphiteEditor:master Feb 28, 2024
2 checks passed
Keavon added a commit that referenced this pull request Feb 29, 2024
@moOsama76 @Keavon
Fix external consistency of grouping layers so they don't jump to the…
96e52c1 
… top of the hierarchy (#1627)

* Calculate insert_index when grouping multiple layers

* no message

* Fix wrong commits

* no message

* restore frontend/package-lock.json

* Fix formatting matches

* Code review to make it idiomatic

* remove wrong line

---------

Co-authored-by: Keavon Chambers <keavon@keavon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keavon Keavon Keavon approved these changes

@0HyperCube 0HyperCube 0HyperCube approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid layers being reordered when dealing with groups
3 participants
@moOsama76 @Keavon @0HyperCube