-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Ace editor #338
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
Merged
Merged
Ace editor #338
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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 file contains hidden or 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 file contains hidden or 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 file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
pub mod bookconfig; | ||
pub mod htmlconfig; | ||
pub mod playpenconfig; | ||
pub mod tomlconfig; | ||
pub mod jsonconfig; | ||
|
||
// Re-export the config structs | ||
pub use self::bookconfig::BookConfig; | ||
pub use self::htmlconfig::HtmlConfig; | ||
pub use self::playpenconfig::PlaypenConfig; | ||
pub use self::tomlconfig::TomlConfig; |
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
use std::path::{PathBuf, Path}; | ||
|
||
use super::tomlconfig::TomlPlaypenConfig; | ||
|
||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
pub struct PlaypenConfig { | ||
editor: PathBuf, | ||
editable: bool, | ||
} | ||
|
||
impl PlaypenConfig { | ||
/// Creates a new `PlaypenConfig` for playpen configuration. | ||
/// | ||
/// ``` | ||
/// # use std::path::PathBuf; | ||
/// # use mdbook::config::PlaypenConfig; | ||
/// # | ||
/// let editor = PathBuf::from("root/editor"); | ||
/// let config = PlaypenConfig::new(PathBuf::from("root")); | ||
/// | ||
/// assert_eq!(config.get_editor(), &editor); | ||
/// assert_eq!(config.is_editable(), false); | ||
/// ``` | ||
pub fn new<T: Into<PathBuf>>(root: T) -> Self { | ||
PlaypenConfig { | ||
editor: root.into().join("editor"), | ||
editable: false, | ||
} | ||
} | ||
|
||
pub fn fill_from_tomlconfig<T: Into<PathBuf>>(&mut self, root: T, tomlplaypenconfig: TomlPlaypenConfig) -> &mut Self { | ||
let root = root.into(); | ||
|
||
if let Some(editor) = tomlplaypenconfig.editor { | ||
if editor.is_relative() { | ||
self.editor = root.join(editor); | ||
} else { | ||
self.editor = editor; | ||
} | ||
} | ||
|
||
if let Some(editable) = tomlplaypenconfig.editable { | ||
self.editable = editable; | ||
} | ||
|
||
self | ||
} | ||
|
||
pub fn is_editable(&self) -> bool { | ||
self.editable | ||
} | ||
|
||
pub fn get_editor(&self) -> &Path { | ||
&self.editor | ||
} | ||
|
||
pub fn set_editor<T: Into<PathBuf>>(&mut self, root: T, editor: T) -> &mut Self { | ||
let editor = editor.into(); | ||
|
||
if editor.is_relative() { | ||
self.editor = root.into().join(editor); | ||
} else { | ||
self.editor = editor; | ||
} | ||
|
||
self | ||
} | ||
} |
This file contains hidden or 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 file contains hidden or 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 file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe this could be another config, i.e., an
EditorConfig
. One thing, though, is that even if you abstract it away here, it's not abstracted inbook.js
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turtles all the way down ;) I guess that we should not worry about it atm. book.js and index.hbs are not as parametrized and more coupled than one would like anyway.
This might be a good material for a refactor in another PR. What do you think @azerupi?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is better than nothing. 😉
Do you have something specific in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't get me wrong. I think that this PR is really good, just there is no need to try to refactor everything at once now.
I was thinking about some possible future changes. Some random ideas:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like that very much. Once we have plugins I want to make playpen etc. plugins with their own config. That would hook into the work that @Michael-F-Bryan is doing. I don't think it is worth refactoring right now, efforts are better spend making sure that the plugin system will support all of this.
That is something we could do. I'm actually a fan of typescript instead of JavaScript, so maybe it's worth considering switching to it? The problem is that it adds a compilation phase and a dependency on the typescript compiler. One more thing to learn and setup for new contributors.
Absolutely, here again I would make MathJax a plugin in the future. But if it is really a problem we could find a temporary fix.
That would be great, but a lot of refactoring is needed before we can really begin to do this.
Yes, that is something I would want. Actually the whole 'theme' handling should be rethought so that adding new themes is easy and configurable. One thing that could be beneficial is to move from stylus to SASS, because we then can use the Rust sass bindings to compile themes at runtime.
All those points are great but not related to this PR, so I propose you make issues for all / some of them to discuss them further and track progress :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. That is what I meant when talking about refactor in another PR's. Imho this PR should not be blocked on more refactoring.
Further rewrites should be given some thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript would be cool to see. 😄 I kept being annoyed that I don't have types when going from Rust to JavaScript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@azerupi I've made 8 issues. Feel free to edit or remove them, if something does not make sense. https://github.com/azerupi/mdBook/issues/347 https://github.com/azerupi/mdBook/issues/348 https://github.com/azerupi/mdBook/issues/349 https://github.com/azerupi/mdBook/issues/350 https://github.com/azerupi/mdBook/issues/351 https://github.com/azerupi/mdBook/issues/352 https://github.com/azerupi/mdBook/issues/353 https://github.com/azerupi/mdBook/issues/354