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

Giant PR making demo crate kinda presentable #52

Merged
merged 63 commits into from
Oct 26, 2019
Merged

Conversation

e-matteson
Copy link
Collaborator

Split out the CoreEditor, implement mode-style and menu-style keymaps, and implement a proper callstack for the concatenative language.

Instead of automatically treating everything in a CommandGroup as 1 undo group.
We'll need more flexibility in undos when implementing a fancier call stack.

feat: replace commandgroup with metacommand
Instead of returning it from get_content(). Should be fairly static, and this
simplifies things.
This could lead to their unused undo stacks growing really big...
And move the `Keymaps` manager into another file.
Instead of requiring it to be 'static.
Copy link
Owner

@justinpombrio justinpombrio left a comment

Choose a reason for hiding this comment

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

Yay, modes and menus! And a call stack! And Engine/Server separation!

Comment on lines 16 to 19
lazy_static! {
pub static ref LANG_SET: LanguageSet = LanguageSet::new();
pub static ref NOTE_SETS: GrowOnlyMap<LanguageName, NotationSet> = GrowOnlyMap::new();
}
Copy link
Owner

Choose a reason for hiding this comment

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

These should be (i) made in main, and (ii) probably put in a struct

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made in main, not in a struct for now

}
}

pub struct Core<'l> {
Copy link
Owner

Choose a reason for hiding this comment

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

CoreEditor

Copy link
Owner

Choose a reason for hiding this comment

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

wait no, Engine!

use crate::keymaps::{FilterRule, TextKeymapFactory, TreeKeymapFactory};
use crate::prog::{Prog, Value, Word};

pub fn make_node_map<'l>(lang: &Language) -> TreeKeymapFactory<'l> {
Copy link
Owner

Choose a reason for hiding this comment

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

These 4 functions could use doc strings

};

let divider = PaneNotation::Fill {
ch: '=',
Copy link
Owner

Choose a reason for hiding this comment

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

Might be impressive & pretty to use a unicode character here. Some possibilities:
░▒▓
═ // this is not an equal sign
▄▅▆▇█

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Characters don't work well anyways because there's no DynWidth option for the text in the status bar. Maybe we should reverse the background instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And PaneNotations don't really support applying a Style to a whole Pane right now, so I'm leaving this as-is until those pretty changes shake out.

Comment on lines 76 to 79
// Note: we can't use the inner DocError as the error source because it
// contains a non-static lifetime.
#[error("doc error: {0}")]
DocExec(DocError<'l>),
Copy link
Owner

Choose a reason for hiding this comment

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

As discussed, we should eliminate lifetime parameters on error messages:

  • Eliminate lifetime parameter on ServerError, EngineError, DocError.
  • Delete the replace error in DocError
  • Instead of docs having a replace command, have a "insert fresh node with no children" command.
  • Change how keyhints are created to accommodate this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the lifetime parameter from errors. We need to keep the Replace command though, for implementing undo. Because of that, and because the Doc doesn't have access to the language set and notation sets needed to create a new node, I'd rather continue to have the Engine create new nodes and pass them to the Doc in a Replace command. The Doc won't be able to give back a rejected node to the Engine, but that doesn't matter.

Comment on lines 86 to 94
let tree_context = if doc.in_tree_mode() {
Some(FilterContext {
required_sort: doc.self_sort(),
self_arity: doc.self_arity_type(),
parent_arity: doc.parent_arity_type(),
})
} else {
None
};
Copy link
Owner

Choose a reason for hiding this comment

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

Consider putting this in its own method. get_tree_context()

Comment on lines 162 to 172
fn next_event(&mut self, keymap: &FilteredKeymap) -> Result<Prog<'l>, ServerError<'l>> {
match self.frontend.next_event() {
Some(Ok(Event::KeyEvent(Key::Ctrl('c')))) => Err(ServerError::KeyboardInterrupt),
Some(Ok(Event::KeyEvent(key))) => self
.keymap_manager
.lookup(key, keymap)
.ok_or_else(|| ServerError::UnknownKey(key)),
Some(Err(err)) => Err(err.into()),
_ => Err(ServerError::UnknownEvent),
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Should probably inline this. As a bonus, it will make the borrow checker happy about the FilteredKeymap struct change.

Ok(ed)
}

pub fn run(&mut self) -> Result<(), ServerError<'l>> {
Copy link
Owner

Choose a reason for hiding this comment

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

Could use comments. (Regular comments, not doc comments.)

fn update_key_hints(&mut self, keymap: &FilteredKeymap) -> Result<(), ServerError<'l>> {
let lang_name = self.engine.lang_name_of(&DocLabel::KeyHints)?;

let mut dict_node = self.engine.new_node("dict", lang_name)?;
Copy link
Owner

Choose a reason for hiding this comment

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

Rename:

  • dict -> keymap
  • entry -> binding

}
}

fn update_key_hints(&mut self, keymap: &FilteredKeymap) -> Result<(), ServerError<'l>> {
Copy link
Owner

Choose a reason for hiding this comment

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

For the future: make a "Virtual Doc" that implements PrettyDocument, b.c. ASTs have a terrible API for this use case, as demonstrated in this method's code. (Add a TODO.)

Leftover from the old implementation of keymap filtering. If we want them again
someday for something else, we can re-add them.
@justinpombrio justinpombrio merged commit a813e70 into master Oct 26, 2019
@justinpombrio justinpombrio deleted the split-core-editor branch February 23, 2020 21:05
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.

2 participants