Skip to content

Add a selected_layers() function and refactor code to use it #314

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

Merged
merged 2 commits into from
Aug 1, 2021

Conversation

Keavon
Copy link
Member

@Keavon Keavon commented Jul 28, 2021

This change is Reviewable

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 28, 2021

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: ffb3038
Status: ✅  Deploy successful!
Preview URL: https://64fac0ee.graphite-master.pages.dev

View logs

@TrueDoctor TrueDoctor marked this pull request as ready for review July 28, 2021 08:09
@TrueDoctor TrueDoctor requested a review from henryksloan July 28, 2021 08:09
@TrueDoctor TrueDoctor force-pushed the select-layers-refactor branch 2 times, most recently from 53ae4c7 to c5d2153 Compare July 28, 2021 08:17
@Keavon
Copy link
Member Author

Keavon commented Jul 28, 2021

I believe we also wanted, in this PR, to change selected_layers_sorted to not create a new Vec. @TrueDoctor

@TrueDoctor
Copy link
Member

I believe we also wanted, in this PR, to change selected_layers_sorted to not create a new Vec. @TrueDoctor

We actually can't because, as the name suggests, we need to sort the layers which is not possible without collecting them first

@Keavon Keavon changed the title Add a selected_layers() function and refactor selected_layers_sorted() Add a selected_layers() function and refactor code to use it Jul 28, 2021
@AyeTbk
Copy link
Contributor

AyeTbk commented Jul 29, 2021

Should this method (selected_layers) be in document_file.rs, on the Document type, instead of in document_message_handler.rs? To render the selection's bounding box in #282, the selected layers are needed (I currently compute them on my own, but I would like to reuse a prexisting method, if there is one). It also makes more sense to me for the Document to tell us what its selected layers are, instead of having code external to it having to toy with its innards to determine them...
(this goes for a lot of the methods in document_message_handler, I guess)

@TrueDoctor
Copy link
Member

That sounds like a good idea

@Keavon
Copy link
Member Author

Keavon commented Jul 31, 2021

That sounds like a good idea

@TrueDoctor Is that a change you could make then merge this? I only just realized we hadn't merged this yet. I am not sure I understand exactly what change to make per AyeTbk's suggestions.

@TrueDoctor
Copy link
Member

That sounds like a good idea

@TrueDoctor Is that a change you could make then merge this? I only just realized we hadn't merged this yet. I am not sure I understand exactly what change to make per AyeTbk's suggestions.

Oh lol, I tought rhis code had already been merged xD

@TrueDoctor
Copy link
Member

I dont think this pr is conceptually the right place to make that change.
The pr does what it says on the label and that is a good thing

@TrueDoctor TrueDoctor force-pushed the select-layers-refactor branch from c5d2153 to ffb3038 Compare August 1, 2021 04:21
@TrueDoctor
Copy link
Member

Should this method (selected_layers) be in document_file.rs, on the Document type, instead of in document_message_handler.rs? To render the selection's bounding box in #282, the selected layers are needed (I currently compute them on my own, but I would like to reuse a prexisting method, if there is one). It also makes more sense to me for the Document to tell us what its selected layers are, instead of having code external to it having to toy with its innards to determine them...
(this goes for a lot of the methods in document_message_handler, I guess)

I think we should do a seperate pr in which we move lines 131-202 into the document_file but it makes sense to move them as a bunch

@TrueDoctor TrueDoctor requested a review from AyeTbk August 1, 2021 04:27
@TrueDoctor
Copy link
Member

I will address the movement to the document file + further refactoring in the upcoming transforms api

@TrueDoctor TrueDoctor merged commit 399111d into master Aug 1, 2021
@TrueDoctor TrueDoctor deleted the select-layers-refactor branch August 1, 2021 08:29
Keavon added a commit that referenced this pull request Jun 16, 2022
* Add a selected_layers() function 

* Refactor AlignSelectedLayers

Co-authored-by: Dennis Kobert <[email protected]>
Keavon added a commit that referenced this pull request Jul 30, 2023
* Add a selected_layers() function 

* Refactor AlignSelectedLayers

Co-authored-by: Dennis Kobert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants