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.

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

Text Reflowing/Justification (2) #236

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
9a2c378
init justification code
oldaccountdeadname Aug 10, 2021
a4c741e
additional test and misc fixes for justify
oldaccountdeadname Aug 10, 2021
ebe7df5
additional test case, behavior correction
oldaccountdeadname Aug 10, 2021
6844310
rename `justify` -> `justify_str` in preperation for command
oldaccountdeadname Aug 11, 2021
5f87832
extract sel_to_range
oldaccountdeadname Aug 11, 2021
d423570
use justify command and bind it to `z` in select and select_line
oldaccountdeadname Aug 11, 2021
6ab4430
introduce prefix param
oldaccountdeadname Aug 14, 2021
d142ff1
basic prefixing
oldaccountdeadname Aug 15, 2021
e0e9f79
extra test for prefixes
oldaccountdeadname Aug 15, 2021
a31ee71
infer prefix
oldaccountdeadname Aug 15, 2021
a46f8dd
thin down command logic to a custom type
oldaccountdeadname Aug 22, 2021
677250b
don't assume that the selection is vaild.
oldaccountdeadname Aug 23, 2021
c6c0f85
don't use empty gitmodules
oldaccountdeadname Sep 5, 2021
9f22d48
don't blindly unwrap
oldaccountdeadname Sep 5, 2021
fdec763
use sel.to_range instead of manually wrapping the range
oldaccountdeadname Sep 5, 2021
1716739
don't map
oldaccountdeadname Sep 5, 2021
6adf8c2
clear names
oldaccountdeadname Sep 5, 2021
cc80df3
fix name issue
oldaccountdeadname Sep 5, 2021
c565673
shorten error handling logic
oldaccountdeadname Sep 5, 2021
04fae79
Revert "use sel.to_range instead of manually wrapping the range"
oldaccountdeadname Sep 5, 2021
2adc1ee
more verbose names
oldaccountdeadname Sep 23, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 51 additions & 25 deletions src/commands/selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,13 @@ use super::application;
use crate::errors::*;
use crate::commands::{self, Result};
use crate::util;
use crate::util::reflow::Reflow;

pub fn delete(app: &mut Application) -> Result {
if let Some(buffer) = app.workspace.current_buffer() {
match app.mode {
Mode::Select(ref select_mode) => {
let cursor_position = *buffer.cursor.clone();
let delete_range = Range::new(cursor_position, select_mode.anchor);
buffer.delete_range(delete_range.clone());
buffer.cursor.move_to(delete_range.start());
}
Mode::SelectLine(ref mode) => {
let delete_range = mode.to_range(&*buffer.cursor);
buffer.delete_range(delete_range.clone());
buffer.cursor.move_to(delete_range.start());
}
Mode::Search(ref mode) => {
let selection = mode.results
.as_ref()
.and_then(|r| r.selection())
.ok_or("Can't delete in search mode without a selected result")?;
buffer.delete_range(selection.clone());
}
_ => bail!("Can't delete selections outside of select mode"),
};
} else {
bail!(BUFFER_MISSING);
}
let rng = sel_to_range(app)?;
let buf = app.workspace.current_buffer().unwrap();
buf.delete_range(rng.clone());
buf.cursor.move_to(rng.start());

Ok(())
}
Expand Down Expand Up @@ -100,6 +80,52 @@ fn copy_to_clipboard(app: &mut Application) -> Result {
Ok(())
}

pub fn justify(app: &mut Application) -> Result {
let rng = sel_to_range(app)?;
let mut buf = app.workspace.current_buffer().unwrap();

let tar = match app.preferences.borrow().line_length_guide() {
Some(n) => n,
None => bail!("Justification requires a line_length_guide."),
};

let rfl = Reflow::new(&mut buf, rng, tar)?;
rfl.apply()?;

Ok(())
}

fn sel_to_range(app: &mut Application) -> std::result::Result<Range, Error> {
let buf = app.workspace.current_buffer().ok_or(BUFFER_MISSING)?;

match app.mode {
Mode::Select(ref sel) => {
let cursor_position = *buf.cursor.clone();
Ok(Range::new(cursor_position, sel.anchor))
},
Mode::SelectLine(ref sel) => {
Ok(util::inclusive_range(
&LineRange::new(
sel.anchor,
buf.cursor.line
),
buf
))
Comment on lines +106 to +112
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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! ✨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},
Mode::Search(ref search) => {
Ok(search
.results
.as_ref()
.and_then(|r| r.selection())
.ok_or("A selection is required.")
.unwrap()
.clone()
)
}
_ => bail!("A selection is required."),
}
}

#[cfg(test)]
mod tests {
use crate::commands;
Expand Down
2 changes: 2 additions & 0 deletions src/input/key_map/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ select:
R: git::copy_remote_url
m: view::scroll_down
f: application::switch_to_second_stage_jump_mode
z: selection::justify
"'": application::switch_to_jump_mode
",": view::scroll_up
page_up: view::scroll_up
Expand Down Expand Up @@ -263,6 +264,7 @@ select_line:
R: git::copy_remote_url
m: view::scroll_down
f: application::switch_to_second_stage_jump_mode
z: selection::justify
",": view::scroll_up
">": buffer::indent_line
"<": buffer::outdent_line
Expand Down
1 change: 1 addition & 0 deletions src/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ pub use self::selectable_vec::SelectableVec;

pub mod movement_lexer;
mod selectable_vec;
pub mod reflow;
pub mod token;

use crate::errors::*;
Expand Down
241 changes: 241 additions & 0 deletions src/util/reflow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,241 @@
use super::*;

/// Encapsulate reflow logic for buffer manipulation.
pub struct Reflow<'a> {
buf: &'a mut Buffer,
rng: Range,
txt: String,
lmt: usize,
}

impl<'a> Reflow<'a> {
/// Create a reflow instance, where buffer and range determine the target,
/// and the limit is the maximum length of a line, regardless of prefixes.
pub fn new(
buf: &'a mut Buffer, rng: Range, lmt: usize
) -> std::result::Result<Self, Error> {
let txt = match buf.read(&rng) {
Some(t) => t,
None => bail!("Selection is invalid."),
};
Ok(Self { buf, rng, txt, lmt })
}

pub fn apply(mut self) -> std::result::Result<(), Error> {
let prefix = self.infer_prefix()?;
let jtxt = self.justify_str(&prefix);
self.buf.delete_range(self.rng.clone());
self.buf.cursor.move_to(self.rng.start());
self.buf.insert(jtxt);

Ok(())
}

fn infer_prefix(&self) -> std::result::Result<String, Error> {
match self.txt.split_whitespace().next() {
Some(n) => if n.chars().next().unwrap().is_alphanumeric() {
Ok("".to_string())
} else {
Ok(n.to_string())
},
None => bail!("Selection is empty."),
}
}


fn justify_str(&mut self, prefix: &str) -> String {
let txt = self.buf.read(&self.rng).unwrap();
let mut limit = self.lmt;
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()];
Copy link
Owner

Choose a reason for hiding this comment

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

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. 😇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The playground example you gave seems to have a few issues:

  1. 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)
  2. With no prefix, everything's indented one space.
  3. Sometimes it just doesn't reflow (with your playground ex, setting limit to 15 seems to do that - no idea why)
  4. 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.

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough! I can always refactor against the test suite afterwards. 👍🏼

if prefix != "" {
space_delims[0] += prefix;
space_delims[0] += " ";
space_delims[2] += prefix;
space_delims[2] += " ";
limit -= prefix.len() + 1;
}

while let Some(par) = pars.next() {
let mut words = par.split_whitespace();
let mut len = 0;
let mut first = true;

while let Some(word) = words.next() {
if word == prefix {
continue;
}

len += word.len();

let over = len > limit;
let u_over = over as usize;
let idx = (!first as usize) * u_over + !first as usize;

justified += &space_delims[idx];
justified += word;

// if we're over, set the length to 0, otherwise increment it
// properly. This just does that mith multiplication by 0 instead of
// branching.
len = (len + 1) * (1 - u_over) + (word.len() + 1) * u_over;
first = false;
}

if pars.peek().is_some() {
justified += "\n\n"; // add back the paragraph break.
}
}

justified
}
}

#[cfg(test)]
mod tests {
use super::*;

// as simple as it gets: one character words for easy debugging.
#[test]
fn justify_simple() {
let mut buf = Buffer::new();
buf.insert("\
a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a\n");

Reflow::new(
&mut buf,
Range::new(
scribe::buffer::Position { line: 0, offset: 0 },
scribe::buffer::Position { line: 1, offset: 0 },
),
80,
).unwrap().apply().unwrap();

assert_eq!(
buf.data(),
"\
a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a
a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a"
);
}

#[test]
fn justify_paragraph() {
let mut buf = Buffer::new();
buf.insert("\
these are words to be used as demos for the thing that this is. this is text \
reflowing and justification over a few lines. this is just filler text in case \
it wasn't obvious.\n"
);

Reflow::new(
&mut buf,
Range::new(
scribe::buffer::Position { line: 0, offset: 0 },
scribe::buffer::Position { line: 1, offset: 0 },
),
80,
).unwrap().apply().unwrap();
assert_eq!(
buf.data(), "\
these are words to be used as demos for the thing that this is. this is text
reflowing and justification over a few lines. this is just filler text in case
it wasn't obvious."
);
}

#[test]
fn justify_multiple_pars() {
let mut buf = Buffer::new();
buf.insert("\
Here's more filler text! So fun fact of the day, I was trying to just copy paste \
some lorem ipsum to annoy my latin student friends, but honestly it broke the \
M-q 'justify' function in emacs, which makes it a bit difficult to work with. \
Overall, it's just not that great with code!

Fun fact of the day number two, writing random paragraphs of text is honestly \
taking way more effort than I anticipated, and I deeply apologize for the lack \
of sanity and coherence here!

Fun fact of the day number three is that I spent three hours getting this to not \
branch. There is no way that that micro-optimization will actually save three \
hours worth of time, but I did it anyway for no good reason!\n"
);

Reflow::new(
&mut buf,
Range::new(
scribe::buffer::Position { line: 0, offset: 0 },
scribe::buffer::Position { line: 5, offset: 0 },
),
80,
).unwrap().apply().unwrap();

assert_eq!(
buf.data(), "\
Here's more filler text! So fun fact of the day, I was trying to just copy paste
some lorem ipsum to annoy my latin student friends, but honestly it broke the
M-q 'justify' function in emacs, which makes it a bit difficult to work with.
Overall, it's just not that great with code!

Fun fact of the day number two, writing random paragraphs of text is honestly
taking way more effort than I anticipated, and I deeply apologize for the lack
of sanity and coherence here!

Fun fact of the day number three is that I spent three hours getting this to not
branch. There is no way that that micro-optimization will actually save three
hours worth of time, but I did it anyway for no good reason!"
);
}

#[test]
fn justify_simple_prefix() {
let mut buf = Buffer::new();
buf.insert("\
# a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a\n"
);
Reflow::new(
&mut buf,
Range::new(
scribe::buffer::Position { line: 0, offset: 0 },
scribe::buffer::Position { line: 1, offset: 0 },
),
80,
).unwrap().apply().unwrap();

assert_eq!(
buf.data(), "\
# a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a
# a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a a"
);
}

#[test]
fn justify_paragraph_prefix() {
let mut buf = Buffer::new();
buf.insert("\
// filler text meant
// to do stuff and things that end up with text nicely \
wrappped around a comment delimiter such as the double slashes in c-style \
languages.\n"
);

Reflow::new(
&mut buf,
Range::new(
scribe::buffer::Position { line: 0, offset: 0 },
scribe::buffer::Position { line: 2, offset: 0 },
),
80,
).unwrap().apply().unwrap();

assert_eq!(
buf.data(), "\
// filler text meant to do stuff and things that end up with text nicely
// wrappped around a comment delimiter such as the double slashes in c-style
// languages.",
);
}
}