WIP: Docs

[pfitzseb]

Implement an InlineDoc class and refactor stuff a bit.

[MikeInnes]

This is great stuff! Let's get the ink changes through to start with and then move on to other bits.

The main thing I would say is, I'm still not sold on the subclassing design for this. A lot of the functionality in EditorOverlay makes sense only for Results, and what is reused is essentially small utility functions that could just be imported. What particularly bothers me is that EditorOverlay and its subclasses seem overly dependent on each other's structure. As much as anything else, this is going to make maintenance difficult – the files don't really stand alone, and you have to remember whether the naming of @disposables is important in InlineResult, or what subclasses EditorOverlay might have when changing remove (which seems antithetical to the purpose of inheritance).

I can see the worry about code duplication, but if you split remove in two, for example, there wouldn't really be any duplication – the two implementation are really quite different. As another data point, it might be worth checking out the stepper implementation – superficially, this is even more similar to inline results than underline results are, but there's virtually nothing that needs sharing with them. (Save for perhaps fading in/out and styles, which could admittedly be better – but mixins would be a better approach here.) What do you think?

Anyway, hope that seems reasonable, I'm looking forward to getting this working!

[pfitzseb]

Yeah, I have no clue on how to do OOP at all (good OOP, that is), so I'm always happy to get better suggestions! :)

I'll just have a go at re-implementing this in a minimal way, and we can build from there I think -- as you said, many of the methods that are currently shared aren't really necessary for docs.

[MikeInnes]

Haha no worries :) Yeah, given the differences I think it's ok to start with docs being totally independent, we can look at any duplication that's there and figure out the best way to share it.

[pfitzseb]

Alright, reimplementation's mostly done. There's only two duplicate (static) methods: @removeCurrent and @removeLines, so yes, this seems much more reasonable.

There's one problem i'm not sure how to solve: Only one command can be associated with one selector in the keymap, so we can't just clear Results and InlineDocs with Escape just like that. I guess we could handle that with a wrapper method that does the dispatch, but maybe there's a more elegant solution?

[MikeInnes]

Yup, this is a huge improvement I think, thanks for going ahead with that.

I'm pretty sure you can associate multiple handlers with a given command, so you could just add the result-clearing handler to inline-results:remove?

[pfitzseb]

Sure, that works, but seems a bit strange since the docs aren't results and the command should therefore be inline-docs:remove-current.

The latest commit renames inline-results:remove-current to inline:remove-current, which removes all results and docs for the given line.

[pfitzseb]

This good to go, @MikeInnes?

[MikeInnes]

Yes, this is looking great! Go ahead and bump anything else you want me to review as well.