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 #220

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
1ec8501
extra docs
oldaccountdeadname Apr 12, 2021
6264898
inline extraneous function for clarity
oldaccountdeadname Apr 12, 2021
5d125f6
docs for View::new and shorten lines
oldaccountdeadname Apr 12, 2021
dd497c9
introduced (unused) range_from function, statement slide on delete
oldaccountdeadname Apr 15, 2021
74ee5fc
use range_from function
oldaccountdeadname Apr 15, 2021
680fdfe
move range_from
oldaccountdeadname Apr 15, 2021
93b125f
justify command
oldaccountdeadname Apr 16, 2021
4dee3fb
initial justification code - just breaks on whitespace
oldaccountdeadname Apr 16, 2021
a1dc7e0
basic greedy justification
oldaccountdeadname Apr 16, 2021
d6f8172
readd whitespace justification
oldaccountdeadname Apr 16, 2021
bcda5be
account for whitespace in justification
oldaccountdeadname Apr 16, 2021
4152494
justification doesnt trim words
oldaccountdeadname Apr 17, 2021
5ef56ac
remove extraneous calls
oldaccountdeadname Apr 17, 2021
34ef570
keep parargaphs when justifying
oldaccountdeadname Apr 17, 2021
b4c9861
just test justify function
oldaccountdeadname Apr 17, 2021
296bd77
get line length from line_length_guide
oldaccountdeadname Apr 17, 2021
2b87ece
reflow around comment marks
oldaccountdeadname Apr 17, 2021
e1a68ba
parametrize comment regex
oldaccountdeadname Apr 17, 2021
060539a
forgot to update the test when extracting the comment prefix regex
oldaccountdeadname Apr 17, 2021
14057c9
apply suggestion replace panic with bail
oldaccountdeadname May 9, 2021
fe2ad07
Update src/commands/selection.rs - use app preferences instead of reg…
oldaccountdeadname May 9, 2021
ccd53b2
Update src/commands/selection.rs - unwrap_or instead of match
oldaccountdeadname May 9, 2021
8fad821
Update src/commands/selection.rs - use text length as capacity instea…
oldaccountdeadname May 9, 2021
00c701f
Update src/commands/selection.rs - use &str instead of &String
oldaccountdeadname May 9, 2021
c5811a3
use result for range_from
oldaccountdeadname May 9, 2021
2b51ff9
use string over regex
oldaccountdeadname May 9, 2021
fe68b08
add test for comment jutsification
oldaccountdeadname May 9, 2021
2b54b67
Switch to normal mode after reflowing
oldaccountdeadname May 9, 2021
8cdd072
Merge branch 'master' of https://github.com/lincolnauster/amp
oldaccountdeadname May 9, 2021
03dff12
only add extra whitespace when necessary
oldaccountdeadname May 9, 2021
9dc7eb5
use ok_or(...) over unwrap()
oldaccountdeadname Aug 9, 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
179 changes: 156 additions & 23 deletions src/commands/selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,32 +6,47 @@ use crate::commands::{self, Result};
use crate::util;

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 {
if app.workspace.current_buffer().is_none() {
bail!(BUFFER_MISSING);
}
Comment on lines +9 to +11
Copy link
Owner

Choose a reason for hiding this comment

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

Now that the buffer declaration is only used in a single branch of the match statement, let's remove this guard clause and inline the existence check; see below for the suggestion on that.

Suggested change
if app.workspace.current_buffer().is_none() {
bail!(BUFFER_MISSING);
}

match app.mode {
Mode::Select(_) | Mode::SelectLine(_) | Mode::Search(_) => {
let delete_range = range_from(app)?;
let buffer = app.workspace.current_buffer().unwrap();
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
let buffer = app.workspace.current_buffer().unwrap();
let buffer = app.workspace.current_buffer().ok_or(BUFFER_MISSING)?;


buffer.delete_range(delete_range.clone());
buffer.cursor.move_to(delete_range.start());
}
_ => bail!("Can't delete selections outside of select mode"),
};

Ok(())
}

pub fn justify(app: &mut Application) -> Result {
if app.workspace.current_buffer().is_none() {
bail!(BUFFER_MISSING);
}

let range = range_from(app)?;

// delete and save the range, then justify that range
let path = &app.workspace.path.clone();
let buffer = app.workspace.current_buffer().unwrap();

if let Some(text) = buffer.read(&range.clone()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unneeded .clone() which can be removed.

Suggested change
if let Some(text) = buffer.read(&range.clone()) {
if let Some(text) = buffer.read(&range) {

buffer.delete_range(range.clone());
buffer.cursor.move_to(range.start());

buffer.insert(
&justify_string(
&text,
app.preferences.borrow().line_length_guide().unwrap_or(80),
Copy link
Owner

Choose a reason for hiding this comment

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

We already specify a default value in the preferences file itself. If someone has explicitly disabled the line length guide, let's raise an error here:

Suggested change
app.preferences.borrow().line_length_guide().unwrap_or(80),
app.preferences.borrow().line_length_guide().ok_or("Cannot justify without line length guide")

&app.preferences.borrow().line_comment_prefix(path).unwrap_or("".to_string()),
)
);
}

Ok(())
}

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

/// Get the selected range from an application in a selection mode. *Requires*
/// that the application has a buffer and is in mode Select, SelectLine, or
/// Search.
fn range_from(app: &mut Application) -> std::result::Result<Range, Error> {
let buffer = app.workspace.current_buffer().ok_or(BUFFER_MISSING)?;

Ok(match app.mode {
Mode::Select(ref select_mode) => {
Range::new(*buffer.cursor.clone(), select_mode.anchor)
}
Mode::SelectLine(ref select_line_mode) => {
select_line_mode.to_range(&*buffer.cursor)
}
Mode::Search(ref mode) => {
mode.results
.as_ref()
.and_then(|r| r.selection())
.ok_or("Cannot get selection outside of select mode.")
.unwrap()
.clone()
}
_ => bail!("Cannot get selection outside of select mode."),
})
}

/// Wrap a string at a given maximum length (generally 80 characters). If the
/// line begins with a comment (matches potential_prefix), the text is wrapped
/// around it.
fn justify_string(text: &str, max_len: usize, potential_prefix: &str) -> String {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it makes more sense to move the default value handling for a prefix into this function, and the Option type makes the "potential" aspect of this argument self-describing.

Suggested change
fn justify_string(text: &str, max_len: usize, potential_prefix: &str) -> String {
fn justify_string(text: &str, max_len: usize, prefix: Option<&str>) -> String {

let mut justified = String::with_capacity(text.len());
let mut paragraphs = text.split("\n\n").peekable();
while let Some(paragraph) = paragraphs.next() {
let mut paragraph = paragraph.split_whitespace().peekable();
let prefix;
let max_len_with_prefix;
if paragraph.peek().is_some()
&& (Some(&potential_prefix) == paragraph.peek())
{
prefix = paragraph.next().unwrap().to_owned() + " ";
max_len_with_prefix = max_len - prefix.len();
justified += &prefix;
} else {
prefix = String::new();
max_len_with_prefix = max_len;
}

let mut len = 0;

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

len += word.len() + 1;
if len > max_len_with_prefix {
len = word.len();
justified.push('\n');
justified += &prefix;
}
justified += word;

if paragraph.peek().is_some() {
justified.push(' ');
}
}

if paragraphs.peek().is_some() {
justified += "\n\n"; // add the paragraph delim
}
}

justified
}

#[cfg(test)]
mod tests {
use crate::commands;
Expand Down Expand Up @@ -219,4 +308,48 @@ mod tests {
String::from("amp\nitor\nbuffer")
)
}

#[test]
fn justify_justifies() {
let text = String::from(
"\nthis is a very \n long line with inconsistent line \nbreaks, even though it should have breaks.\n"
);
assert_eq!(
super::justify_string(&text, 80, "//"),
String::from("this is a very long line with inconsistent line breaks, even though it should \nhave breaks.")
);
}

#[test]
fn justify_justifies_with_comment() {
let text = String::from(
"\n// this is a very \n long line with inconsistent line \nbreaks, even though it should have breaks.\n"
);
assert_eq!(
super::justify_string(&text, 80, "//"),
String::from("// this is a very long line with inconsistent line breaks, even though it \n// should have breaks.")
);
}

#[test]
fn justify_justifies_paragraphs() {
let text = String::from(
"\nthis is a very \n long \n\n line with inconsistent line \nbreaks, even though it should have breaks.\n"
);
assert_eq!(
super::justify_string(&text, 80, "//"),
String::from("this is a very long\n\nline with inconsistent line breaks, even though it should have breaks.")
);
}

#[test]
fn justify_justifies_paragraphs_with_comment() {
let text = String::from(
"// this is a very \n long \n\n line with inconsistent line \nbreaks, even though it should have breaks.\n"
);
assert_eq!(
super::justify_string(&text, 80, "//"),
String::from("// this is a very long\n\nline with inconsistent line breaks, even though it should have breaks.")
);
}
}
1 change: 1 addition & 0 deletions src/input/key_map/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ select:
ctrl-a: selection::select_all
ctrl-z: application::suspend
ctrl-c: application::exit
a: selection::justify
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably should be added to the select_line mode as well, would be useful to have there too.


select_line:
up: cursor::move_up
Expand Down
13 changes: 6 additions & 7 deletions src/models/application/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,13 @@ pub struct Application {
}

impl Application {
/// Initialize an empty application in normal mode with any discovered
/// repository, and default preferences.
pub fn new(args: &Vec<String>) -> Result<Application> {
let preferences = initialize_preferences();
let preferences =
Rc::new(RefCell::new(
Preferences::load().unwrap_or_else(|_| Preferences::new(None)),
));
Comment on lines -58 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should stay as-is. This does not provide any benefits and not having all things crammed into one function is very good for readability and maintainability.

The comment is all well and can stay of course, such things are always nice to have.


let (event_channel, events) = mpsc::channel();
let mut view = View::new(preferences.clone(), event_channel.clone())?;
Expand Down Expand Up @@ -218,12 +223,6 @@ impl Application {
}
}

fn initialize_preferences() -> Rc<RefCell<Preferences>> {
Rc::new(RefCell::new(
Preferences::load().unwrap_or_else(|_| Preferences::new(None)),
))
}

fn create_workspace(view: &mut View, preferences: &Preferences, args: &Vec<String>) -> Result<Workspace> {
// Discard the executable portion of the argument list.
let mut path_args = args.iter().skip(1).peekable();
Expand Down
10 changes: 8 additions & 2 deletions src/view/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,14 @@ pub struct View {
}

impl View {
pub fn new(preferences: Rc<RefCell<Preferences>>, event_channel: Sender<Event>) -> Result<View> {
let terminal = build_terminal().chain_err(|| "Failed to initialize terminal")?;
/// Return a new view. This will initialize the terminal, load the theme,
/// and begin to listen for events.
pub fn new(
preferences: Rc<RefCell<Preferences>>,
event_channel: Sender<Event>
) -> Result<View> {
let terminal = build_terminal()
.chain_err(|| "Failed to initialize terminal")?;
Comment on lines -49 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, completely unrelated change?
Not needed IMHO since this line just about tickles the 100 character barrier, which is reasonable to default to for todays standards.

The comment again is nice and can stay, but the reformatting is somewhat unneeded. Especially since there are more occurences of this sort in this file and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sorry, I was playing with the codebase a bit trying to understand it, and I think I accidentally commited this instead of starting from the original codebase when I tried to implement line wrapping.

let theme_path = preferences.borrow().theme_path()?;
let theme_set = ThemeLoader::new(theme_path).load()?;

Expand Down