Ace editor #338

projektir · Jun 18, 2017

Closes #247.

Here's what I got so far.

I've put the license on the main ace.js file, but perhaps the files for themes and mode needs a license, too.

I wasn't entirely sure how the config is meant to be used, and it got refactored while I was working on this so I decided to not go too far with it. Perhaps the editor should get its own config?

Themes are fairly problematic. I couldn't find any that looked quite right, and there's some clashing because I had to remove highlight.js from the editable blocks and I think Ace themes also clash. Here's what it looks like right now:

tomorrow-night
dawn

budziq · Jun 19, 2017

budziq reviewed Jun 19, 2017

View changes

budziq · Jun 19, 2017

src/theme/book.css

@@ -156,6 +156,9 @@ table thead td {
 .content img {
  max-width: 100%;
 }
+pre > .buttons {
+  z-index: 2;


hi. this will get overwritten on stylus rebuild.

Ah, right. I'll fix this once I figure out how to get nib to work on my machine.

steveklabnik · Jun 19, 2017

steveklabnik approved these changes Jun 19, 2017

View changes

Awesome! I'm so psyched for this.

projektir · Jun 21, 2017

Travis build can be re-run, there was an issue with nightlies that should be fixed now: https://twitter.com/RustStatus/status/877343457991208960

budziq · Jun 21, 2017

budziq reviewed Jun 21, 2017

View changes

Hi I did not want to intrude but here are some random thoughts that might be relevant.

budziq · Jun 21, 2017

src/config/tomlconfig.rs

@@ -11,6 +11,8 @@ pub struct TomlConfig {
    pub description: Option<String>,

    pub output: Option<TomlOutputConfig>,
+
+    pub use_ace: Option<bool>,


Hi if I might suggest something. Maybe we could we have ACE as an implementation detail and change the toml/confg entry to something along the lines of:
playpen-editable: Option<bool> or even
playpen-properties:Option<Vec<PlaypenProperty>> where editability / resettability would be just one of the properties?

For instance some books might not want to enable clipboard support or play button too.

Yeah, I was thinking of doing something like this but wasn't quite sure which direction to go in.

So we would have a playpen-properties header with various options, and to use ace, the .toml would look like this:

[playpen-properties]
editable = true

Am I following you right?

Yeah, that would be my suggestion. But feel free to disregard it if it does not make sense 😸

budziq · Jun 21, 2017

src/book/mod.rs

@@ -519,6 +519,14 @@ impl MDBook {
        &[]
    }

+    pub fn uses_ace(&self) -> bool {


~~Hi we have these properties already in BookConfig I would suggest using only those and removing these two methods~~

config is a private property on an MDBook, which means I can't reach it from hbs_renderer, so I followed what seemed to be a convention in creating these extra methods in MDBook to pull out values from the private config. There does not appear to be a get_config function.

Am I missing something?

Yeah serves me right for viewing the code on mobile while riding a bus ;) Disregard the comment sorry. Anyhow I would at least suggest to expose PlaypenProperties here instead of bool.

budziq · Jun 21, 2017

src/config/bookconfig.rs

@@ -18,6 +18,9 @@ pub struct BookConfig {
    indent_spaces: i32,

    html_config: Option<HtmlConfig>,
+
+    use_ace: bool,


Maybe we could have these abstracted into another struct along the lines of Option<PlaypenEditor> (or Option<PlaypenEditorConf> )

budziq · Jun 21, 2017

src/theme/index.hbs

-
+        {{/if}}
+
+        {{#if ace}}


maybe we can use editable-playpenshere and leave ace as an implementation detail

projektir · Jun 22, 2017

projektir reviewed Jun 22, 2017

View changes

projektir · Jun 22, 2017

src/config/playpenconfig.rs

+
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
+pub struct PlaypenConfig {
+    editor: PathBuf,


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 in book.js.

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?

Something is better than nothing. 😉

This might be a good material for a refactor in another PR.

Do you have something specific in mind?

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:

expanding the playpen config to make features opt in or opt out (clipboard support, resetable if editable, code runnable)

split book.js by features or make it parametric like index hbs

possibly support for alternative playpens like integer32 or other non rust speccific ones

make mathjax opt in (it is really heavy and blocks offline reading now)

start working on alternative renderers (possibly split off the renderers into separate crates)

make themes opt in or opt out (maybe some overridable list of defaults)

expanding the playpen config to make features opt in or opt out (clipboard support, resetable if editable, code runnable)

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.

split book.js by features

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.

make mathjax opt in (it is really heavy and blocks offline reading now)

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.

start working on alternative renderers (possibly split off the renderers into separate crates)

That would be great, but a lot of refactoring is needed before we can really begin to do this.

make themes opt in or opt out (maybe some overridable list of defaults)

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 :)

All those points are great but not related to this PR.

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.

I'm actually a fan of typescript instead of JavaScript, so maybe it's worth considering switching to it?

TypeScript would be cool to see. 😄 I kept being annoyed that I don't have types when going from Rust to JavaScript.

I propose you make issues for all / some of them to discuss them further and track progress :)

@azerupi I've made 8 issues. Feel free to edit or remove them, if something does not make sense. #347 #348 #349 #350 #351 #352 #353 #354

projektir · Jun 22, 2017

projektir reviewed Jun 22, 2017

View changes

projektir · Jun 22, 2017

src/renderer/html_handlebars/hbs_renderer.rs

            // Load the editor
-            let editor = editor::Editor::new(book.get_ace_path());
+            let editor = editor::Editor::new(playpen_config.get_editor());
            book.write_file("editor.js", &editor.js)?;
            book.write_file("ace.js", &editor.ace_js)?;


I can probably abstract these into something like editor_additional_js.

The only gripe I have with this change left is that "editor" everywhere is not really that informative (both in config and through out the code). Maybe something along the lines of js_editor_config ?

Also now it might not be always obvious on the first glance which code paths lead to hljs instead of ace. Maybe comments could be added?

editor is on the same level as theme and renderer right now, so I went with a similar naming convention. I could call it playpen_editor, but this depends on what other editors mdBook plans to have, if any. Like, MathJax, is that another type of editor? Is this perhaps not the right time to worry about this?

I could add some docs to the Editor definition, though, as right now it's not too obvious what kind of editor this is, and that way someone could just read that if they need to know about the editor.

I'm not sure about code paths. It's basically "if the playpen config says its editable, add all these other files/data in addition to the preceding files/data". I think making things clearer would require some major refactoring of hbs_renderer in general and I'm not sure what comments would add here (I'm not a huge fan of comments in general).

The situation with hljs is a bit odd since it's still sort of used by editable playpens for some CSS, and it will be used regardless of whether or not editability is enabled, it just won't be used for editable playpens specifically if editability is enabled. So there really aren't two code paths that lead to different things, rather there's one code path that may additionally include the editor and then relies on JS magic to create the divergent behavior. I think the current code structure expresses that OK.

I could call it playpen_editor, but this depends on what other editors mdBook plans to have, if any. Like, MathJax, is that another type of editor? Is this perhaps not the right time to worry about this?

Well you might be right, but from my perspective playpen_editor or even playpen_editor_config would be ideal as it would be easier for new contributors to orient around the code (Somehow even though I had some understanding of what is going on in the code the bare editor did not want to "click" for me until I've red all of this PR and finally understood that this is basically ACE config file)

projektir · Jun 22, 2017

Had to rebase to fix some conflicts; switched modes and themes files for minified versions as I forgot to do that, apparently.

Michael-F-Bryan · Jun 23, 2017

It sounds like a lot of the things you mention will be made possible when we refactor the library to be more extensible and flexible. For example, the playpen stuff, mathjax, alternate renderers and different themes or JavaScript should all slot in really nicely.

Most of them are either manipulations to the in-memory representation of a "Book" (e.g. swapping out a math equation for the equivalent mathjax tags), or manipulations done to the rendered content (e.g. playing around with themes and CSS).

Alternate renderers will also be a thing (preferably split out into crates). My motivating factor for refactoring/restructuring the library was that I tried to make a PDF renderer and found it almost impossible to do in a clean way.

budziq · Jun 23, 2017

budziq approved these changes Jun 23, 2017

View changes

projektir · Jun 24, 2017

Is this good to merge or are there specific changes you guys still want to see?

azerupi · Jun 24, 2017

Is this good to merge or are there specific changes you guys still want to see?

Sorry, I didn't have the time to check this out yet. But since @steveklabnik and @budziq approved I will just review superficially, so I hope it will be quick. :)

azerupi · Jun 24, 2017

azerupi reviewed Jun 24, 2017

View changes

azerupi · Jun 24, 2017

src/config/bookconfig.rs

@@ -17,6 +18,8 @@ pub struct BookConfig {
    multilingual: bool,
    indent_spaces: i32,

+    playpen_config: PlaypenConfig,


playpen is very much tied to the html version of a book, would it not be better to put the playpen config inside HtmlConfig?

It might be something for refactoring on another day. Some features of playpen might be shared with another renderers (like styling based on class rust-lang/book#55 if we decide to have such feature) but I might be reaching here. So feel free to disregard this comment.

HtmlConfig doesn't seem to have correspondence to anything in the code so I wasn't sure what it meant. Maybe the folder you're calling theme should truly be html_renderer or something then, which would then explain why you want me to move editor there? But then there's already a renderer... so I didn't want to mess with this stuff too much.

Historically the theme dir was a catch all for all client side customization outside of book.toml. That's why @azerupi is suggesting the move (for consistency sake).

This might change in the future as you have suggested but most likely we will need a good thought along with major refactoring and split to plugins.

@budziq Makes sense. I'll move it down.

azerupi · Jun 24, 2017

azerupi reviewed Jun 24, 2017

View changes

azerupi · Jun 24, 2017

src/editor/mod.rs

+/// The `Editor` struct should be used instead of the static variables because
+/// the `new()` method
+/// will look if the user has an editor directory in his source folder and use
+/// the users editor instead


Maybe this can be added to the theme directory instead of having an additional directory?

Hmm, themes in my mind relate to the look and feel of things, while this is new functionality. I already feel that the theme folder is fairly heavy in how much it has since it has playpen configuration, navigation, etc., in it. If anything, I think playpens should get their own section somewhere in the code and then the editor would go there (I'm just not comfortable doing such large modifications to the existing codebase right now). This right now feels a bit "all JavaScript goes here". I put the editor as a separate item because it's optional and I didn't want it mixed up with everything else too much.

This would also imply that editor is a submodule of theme. But if you do think this is necessary, let me know.

I agree that the theme name does not currently reflect its purpose. W might consider rename or split in following PRs

I think we misunderstood each other. I am not talking about the code here, I am talking about fetching the resource files. If I understand this PR correctly, you would allow the user to define a new directory in their root folder called editor containing the resource files the user wants to override. I'm merely proposing to not use a new directory for that but use the existing theme directory convention.

Is there something I am misunderstanding?

I mean, they can be replaced, sure, but mostly it's for organization since it's also mirroring where the resources are to begin with and mirroring the config (which would be output > html > editor in this case). I added a new directory because there are a bunch of naturally organized files that relate to the editor and only the editor...

You want me to dump all of those into the theme folder on top of everything that's already in there? Which also implies I'd need to merge all the code, as well... I don't understand what this buys. This, in my view, would make the organization considerably worse. Many of these files are already too long and the folders already contain too many files irrelevant to development. Nor does it express the optionality of the editor in contrast to the rest of the theme.

And consider how confusing it would be to have tomorrow_night.js and tomorrow_night.css right next to each other when one of them is used only by the playpen editor and the other is always used by the main book?

Well you can add an editor sub-directory inside the theme directory. I would really just like to avoid having multiple directories that do almost the same thing...

Another solution is to remove the ability to override the files altogether for now?

projektir · Jun 24, 2017

@azerupi @budziq
So one thing that confused me in regards to using the HtmlConfig: it is marked optional, but I always need some defaults for the playpen even if the user doesn't configure it at all. I always need a playpen config to pull out in hbs_renderer.rs. I'm not sure how to do this when the entire HtmlConfig is optional?

budziq · Jun 24, 2017

@projektir

So one thing that confused me in regards to using the HtmlConfig

I took some time to glance through HtmlConfig and its usage. It looks like its not really optional. Unless I am mistaken it is guaranteed to exist with defaults and is expected in at least two places.

I have prepared a PR removing it being an option while going through the code #362 (it was easiest way to learn the code paths for me). @azerupi Please feel to drop this PR if it dies not match the project vision 😄

Anyhow:
HtmlConfig is required for proper mdBook operation in the current state of affairs (as long as the hbs_renderrer is the only one available) so playpen defaults will be always available (regardless of the #362 which just makes it explicit).

azerupi · Jun 26, 2017

The idea of it being optional is in anticipation of having multiple renderers. We can't always assume that the HTML renderer will be used. But I think the design will still change considerably as we progress towards multiple renderers..

projektir · Jun 30, 2017

Latest rebase. I've moved the PlaypenConfig inside of HtmlConfig, but I'm overall very confused on the whole putting the editor in theme thing. The editor is also currently not copied along with the theme but I wasn't sure where all this was going. Unfortunately, I'm going on vacation so I won't be able to attend to this PR until July 5th or so.

steveklabnik · Jul 5, 2017

Test failures were spurious; I re-ran the failing ones and it's all green.

budziq · Aug 2, 2017

Hi @projektir are you open to continuing work on this PR? I think that it's very valuable and nearly ready for merge. I feel that there have been some communication issues and we could clear out all misunderstandings by talking it over in this thread.

projektir · Aug 2, 2017

@budziq absolutely. I also thought that it was close to being mergeable, but what @azerupi is requesting seems to imply a rewrite... and I thought that didn't seem right so I figured I didn't understand, and I got worried about all the surrounding refactoring throwing in a wrench, so this got stalled.

azerupi · Aug 2, 2017

I am truly sorry about that 😕
It never was my intention to block this PR. Tomorrow I am going to take a fresh look at it and see how we can move it forward. Thanks for your work and patience on this!

azerupi · Aug 3, 2017

azerupi reviewed Aug 3, 2017

View changes

Ok so I reviewed this PR again and it's really good. 👍

The only thing I would like to see changed is the location of the directory where the user would overwrite the built-in files like theme-dawn.js. This was my request previously, but I think there was a big misunderstanding.

To make sure there is no room for a misunderstanding this time, here are the details.

The built-in theme files can be overwritten by the user by placing replacements in a root/theme directory in his book. You have mimicked this behaviour with the editor and that is great! Except that for the editor, the user needs to place the files in root/editor currently. And that is what bothers me. I would prefer if the user would only need to worry about one directory, the root/theme directory.

So what I propose is to look for the files needed by the editor in root/theme/editor or directly in root/theme, not sure which one is better.

If I am not mistaken, this requires only a small change in playpenconfig.rs and / or htmlconfig.rs L40, htmlconfig.rs L88

I hope this clears up the confusion and again, I'm sincerely sorry for the previous misunderstanding.

projektir · Aug 3, 2017

Ah, yes, the default directories of the files! That makes a lot of sense, actually. For some reason I thought you were talking about the code itself and needing to merge theme/mod.rs and the editor mod.rs. I'll try to get this fixed up over the weekend.

I may have to additionally update the copy functionality as it currently doesn't know about the editor at all.

Thanks!

projektir · Aug 4, 2017

I've made the editor files live in the theme/editor folder. I decided to leave theme copy alone because it seems to be outdated, and quite a bit of new code would need to be written to do that properly.

azerupi · Aug 4, 2017

azerupi approved these changes Aug 4, 2017

View changes

azerupi · Aug 4, 2017

Thanks!
For me this PR is now good to merge! 🎉

I decided to leave theme copy alone

No problem, this can still be added later. Thanks a lot for your hard work and patience on this one 😉 Very much appreciated!

@budziq @steveklabnik if you have no other comments on this, we can merge!

budziq · Aug 4, 2017

I've already approved this one but I'll have one last look after work today :)

budziq · Aug 5, 2017

budziq approved these changes Aug 5, 2017

View changes

Very nice :) Look good to me 👍

azerupi · Aug 5, 2017

Merged! 🎉
Big thanks to @projektir for the PR and @budziq for the review!

azerupi added this to the Rust By Example milestone Jun 22, 2017

This was referenced Jun 23, 2017

Open

Rethink theme management #351

Open

Rethink book.js organization #352

Open

Rethink index.hbs organization #353

Open

Rethink playpen configuration #354

projektir added some commits Jun 29, 2017

Adding ace.js, Rust highlighter, and themes #247

6601dbd

Integrating Ace #247

Loading status checks…

16aa545

azerupi merged commit 35a447d into rust-lang-nursery:master Aug 5, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

projektir deleted the projektir:ace_editor branch Aug 5, 2017

projektir referenced this pull request Aug 6, 2017
Closed
Add tooltips to code buttons #388

steveklabnik referenced this pull request Sep 12, 2017
Merged
Convert to mdbook #897

Michael-F-Bryan referenced this pull request Jan 3, 2018
Closed
Document the Ace Editor #521

rust-lang-nursery/mdBook

Join GitHub today

Ace editor #338

Conversation

projektir commented Jun 18, 2017 • edited

budziq reviewed Jun 19, 2017 View changes

This comment has been minimized.

This comment has been minimized.

steveklabnik approved these changes Jun 19, 2017 View changes

steveklabnik left a comment

This comment has been minimized.

projektir commented Jun 21, 2017

budziq reviewed Jun 21, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

budziq Jun 21, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

projektir reviewed Jun 22, 2017 View changes

This comment has been minimized.

projektir Jun 22, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

budziq Jun 23, 2017 • edited

projektir reviewed Jun 22, 2017 View changes

This comment has been minimized.

This comment has been minimized.

budziq Jun 22, 2017 • edited

This comment has been minimized.

This comment has been minimized.

azerupi added this to the Rust By Example milestone Jun 22, 2017

This comment has been minimized.

projektir commented Jun 22, 2017

This comment has been minimized.

Michael-F-Bryan commented Jun 23, 2017

This was referenced Jun 23, 2017

budziq approved these changes Jun 23, 2017 View changes

This comment has been minimized.

projektir commented Jun 24, 2017

This comment has been minimized.

azerupi commented Jun 24, 2017

azerupi reviewed Jun 24, 2017 View changes

This comment has been minimized.

This comment has been minimized.

budziq Jun 24, 2017 • edited

This comment has been minimized.

projektir Jun 24, 2017 • edited

This comment has been minimized.

This comment has been minimized.

azerupi reviewed Jun 24, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

projektir commented Jun 24, 2017

This comment has been minimized.

budziq commented Jun 24, 2017 • edited

This comment has been minimized.

azerupi commented Jun 26, 2017

This comment has been minimized.

projektir commented Jun 30, 2017

This comment has been minimized.

steveklabnik commented Jul 5, 2017

This comment has been minimized.

budziq commented Aug 2, 2017

This comment has been minimized.

projektir commented Aug 2, 2017

projektir commented Jun 18, 2017 •

edited

budziq reviewed Jun 19, 2017

View changes

steveklabnik approved these changes Jun 19, 2017

View changes

budziq reviewed Jun 21, 2017

View changes

budziq Jun 21, 2017 •

edited

projektir reviewed Jun 22, 2017

View changes

projektir Jun 22, 2017 •

edited

budziq Jun 23, 2017 •

edited

projektir reviewed Jun 22, 2017

View changes

budziq Jun 22, 2017 •

edited

budziq approved these changes Jun 23, 2017

View changes

azerupi reviewed Jun 24, 2017

View changes

budziq Jun 24, 2017 •

edited

projektir Jun 24, 2017 •

edited

azerupi reviewed Jun 24, 2017

View changes

budziq commented Jun 24, 2017 •

edited

azerupi reviewed Aug 3, 2017

View changes

projektir commented Aug 3, 2017 •

edited

azerupi approved these changes Aug 4, 2017

View changes

budziq approved these changes Aug 5, 2017

View changes

azerupi merged commit `35a447d` into rust-lang-nursery:master Aug 5, 2017
2 checks passed