Some code organization changes in ShapeLine::layout
#170
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This now conflicts after #175, please rebase. |
…bles declarations down closer to where they are used, move variables that are reset every loop down to be declared in the loop, replace Vec::new + mem::swap with mem::take
* max_ascent and max_descent declarations moved into loop since they are reset each iteration and the one spot where they are used outside the loop for pushing an empty line is if all items are empty (so they would always be 0.0 there). * For `Align::Justified`, instead of repurposing `alignment_correction` variable for expanding blank spaces, there is a new `justification_expansion` variable. This helps clarify the code. * Common code for processing ranges factored out section where ranges are iterated in opposite orders for RTL vs LTR. * We don't need to use `take_mut` on `glyphs` since the variable is not used afterwards (i.e. we can just move out of `glyphs`). * Fix bug where `scratch.scripts` was being used for logging info instead of `scripts`.
…replaced used of push_line bool with checking is layout_lines is empty
|
Rebased. Also fixed a small bug where |
jackpot51
approved these changes
Aug 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was reading through this code (to get a better understanding of how alignment is applied and potential avenues for reusing layout work done for width measurement when performing the final layout) and noticed a few things that could be easily de-duplicated or adjusted to improve clarity (IMO).
ShapeLine::layoutweren't used until the second phase which is much farther down, so I moved them down to be at the start of that phase.push_lineboolwith checkinglayout_lines.is_empty()which is effectively equivalent.visual_lineswere reset with each loop iteration, so I moved their definitions down into the loop.justification_expansioninstead of reusing thealignment_correctionvariable (which just shifts the line in all other modes, but was used to hold how much each space should be expanded when justifying). This is more clear to the reader and avoids several later checks of whether the alignment isAlign::Justifiedin spots wherealignment_correctionis used. (I may have gone overboard with the other changes but I think at least this one is pretty useful)process_rangeclosure as well as combine several cases of iterating over glyphs during this.mem::swap(&mut glyphs, &mut glyphs_swap);(glyphsis not used later and can just be directly moved out of)None of these changes should affect the behavior (If I didn't make any mistakes!).