Text Reflowing/Justification (2) #236
Conversation
im so sorry about the filler text really i swear
make sure that extraneous (early and late) line breaks with extra unwieldy prefixes don't break things.
There was a problem hiding this comment.
This is coming along really nicely! 🥳
Looking at the current implementation, I think there's an opportunity to take this a step further by extracting some of this logic into a dedicated util::Reflow type. At a high level, there are two groups of actions in the reflow process; the first is mostly setup work that requires application-level information (e.g. preferences, current mode):
- determine the reflow limit (using the line length guide)
- map the selection to a range
The second group is focused entirely on the buffer:
- read the range into a string
- analyze the string to infer a prefix
- reflow the string, using the prefix and limit
- delete the buffer selection
- move the cursor
- insert the reflowed content
Why is this worth doing? Commands in the amp codebase are primarily procedural glue code, stitching together disparate types with cross-cutting functions that span many different objects. Their function signatures are intentionally broad (taking in the application itself) to allow for this, but it also makes it easy for them to balloon in size. When I see that, especially when it's paired with other non-public functions that relate to the command, I think it's likely to benefit from being grouped together in a discrete type. If you've used MVC web frameworks, I treat commands like controller actions, which are encouraged to be "skinny", deferring to models/service objects for the bulk of their logic.
If we can keep the command contents focused on the steps in the first group (which requires the broad Application type), we can extract the actual reflow logic and all of its supporting functions and tests into a dedicated type that'll encapsulate that. Then, we would have a justify command that looks like this:
- determine the reflow limit (using the line length guide)
- map the selection to a range
- call something like
Reflow::new(&buffer, selection, limit).process
which is pretty straight-forward/simple. If you want to get into the gory details of how the process works, you can dip into the Reflow type itself to see.
Sorry for the long-winded reply. You've done great work on this, and before I ask for more of your time, I wanted to share my reasoning behind the request. I don't think there's too much work involved to get this tidied up. Let me know what you think! 😄
|
Thanks, this is really helpful! I've implemented that - tell me if there's anything I've missed! |
Use explicit error handling when we get at the selection the Reflow type constructor.
There was a problem hiding this comment.
Really digging this refactor; thanks for putting that together! 😎
I've left some more feedback below. That feedback aside, I'd love it if we could avoid abbreviations in the variable names (e.g. buf, rng, txt, lmt) as they're harder to read at a glance. Most of those are small enough that we're not saving that much on typing, anyway. I hate to bring up little nits like that in a PR, but I don't have an established style guide for the project (yet). Little things like that do keep the project tidy and consistent, though. 🙂
LMK what you think. 🍻
| Ok(util::inclusive_range( | ||
| &LineRange::new( | ||
| sel.anchor, | ||
| buf.cursor.line | ||
| ), | ||
| buf | ||
| )) |
There was a problem hiding this comment.
| Ok(util::inclusive_range( | |
| &LineRange::new( | |
| sel.anchor, | |
| buf.cursor.line | |
| ), | |
| buf | |
| )) | |
| Ok(sel.to_range(&*buf.cursor)) |
should do all of this for you! ✨
There was a problem hiding this comment.
hi - looks like I pushed this without testing and for some reason reading the resulting range (specifically on a one-line file, though maybe in more scenarios) from the selection with .read returns an error. I've just reverted that for now, but I can try to see what's going on with that later.
| let mut justified = String::with_capacity(txt.len()); | ||
| let mut pars = txt.split("\n\n").peekable(); | ||
|
|
||
| let mut space_delims = ["".to_string(), " ".to_string(), "\n".to_string()]; |
There was a problem hiding this comment.
This part of the method implementation is clever, but hard to read/understand.
I put together a Rust playground example of an implementation that uses fold to carry the length accumulator. It also avoids pre-building space delimiters, opting instead to use them literally where they're needed.
It's possible I've missed an edge case, but I think leaning into that kind of an approach is easier to follow, and would help with long-term maintainability. 😇
There was a problem hiding this comment.
The playground example you gave seems to have a few issues:
- It'll add a newline regardless of whether or not the line is the first of a paragraph (so with the 3rd bug, it just has the bizzare effect of moving everything down one line)
- With no prefix, everything's indented one space.
- Sometimes it just doesn't reflow (with your playground ex, setting limit to 15 seems to do that - no idea why)
- The first output branch doesn't seem to account for the space between the prefix and the word (though thats an easy fix)
I personally feel that once that's fixed, we'd end up with something about the same length and nearly as complex as what's currently in the fork. I completely agree with you about the complexity, but imo reflowing is non-trivial to implement trivially --- it might just be more efficient to leave it as it is.
There was a problem hiding this comment.
Fair enough! I can always refactor against the test suite afterwards. 👍🏼
Co-authored-by: Jordan MacDonald <[email protected]>
Co-authored-by: Jordan MacDonald <[email protected]>
This reverts commit fdec763.
lmt -> limit rng -> range txt -> text ..etc
|
Hey @LincolnAuster, I've renamed the |
See #220