Inline result overhaul - Githubissues

pfitzseb commented 7 years ago

This should make inline results much nicer to work with:

Maximum width on inline results is now adjusted dynmically such that the result can take up all available space: Old behaviour was to limit a result's width to 400px.
Double clicking a result (or using Alt-T -- not sure what a good Mac shortcut would be) will expand it to the full editor width (most uesful for results on/after a long line):
It's now possible to select text in results and copy it using Ctrl-C (or Cmd-C): Context menu copy still has the old behaviour.

All in all this should minimize scrolling and make results a bit more predictable. Let me know what you think! :)

On a related note: result.coffee needs an overhaul, which I might get to soonish. That's orthogonal to these improvements though.

TODO:

[x] Make resizing faster (no layout thrashing, batched repaints etc)
[x] Use a MarkerLayer for inline results
[x] Figure out how to properly handle clicks (regarding Tree expansion)

Datseris commented 7 years ago

please, please yes!!!

MikeInnes commented 7 years ago

Good stuff, will have to play around with this branch.

Code looks good, my main immediate worry is – where are we listening for resizes? We should be very careful about anything that might impact editor performance.

pfitzseb commented 7 years ago

Yup, that's why I labeled this WIP. Performance doesn't seem to be bad even with many inline results, but there certainly is some hit on performance (that's with 200 inline results and resizing the editor a couple of times): perf That could be improved by adding only one resize listener per editor and then calling getBoundingClientRect() only once per editor.editorElement resize event. The implementation would be a bit more complex though, and I was hesitant to do it that way without consensus that this is the right approach ;)

pfitzseb commented 7 years ago

I dunno if someone has tried this yet, but what I was thinking about is changing the unfolding behaviour to expanding trees in results by default and focus the result. The latter might enable us to support a completly keyboard based result workflow, if we get keybindings in place for navigating trees etc. Any opinions?

MikeInnes commented 7 years ago

Some notes from using this:

Double clicking also expands trees, so we need to be smarter about that
I occasionally get errors along the lines of .getBoundingClientRect is not a function. Guess this might be a use-after-free with editors, or something?
The border rounding has gone. I think it would probably ok to stick with just the side border even in underline mode (but I haven't tried it).

Not sure I understand your suggestion. Do you mean to have all trees expanded by default once you open the first?

pfitzseb commented 7 years ago

Yeah, tree expansion and result expansion don't work well together, but I'm not sure how to solve this without delaying tree expansion until the doubleclick time frame has passed
That's missing error handling in updateResultWidth, I think, and should be gone in the latest version.
Heh, I just wanted to try something new ;) We can have the top border and rounding without any problems. Without the top border the results aren't distinctive enough imho.

What I meant by my (somewhat poorly worded) suggestion is that expanding a result will always automatically expand the topmost tree (kinda like what happens now if you click on the tree instead of next to it) and focus the result, so you can't continue typing in the editor etc. Then we could enable Tab navigation through the result.

Apart from that the performance impact is pretty big... I've been trying to work around that for the last couple of hours, without too much success. The basic problem here is layout thrashing, i.e. having interleaved reads and writes to the DOM that'll force webkit to update the layout. And since atom core is updating the layout when the editor is resized and we're doing the same afterwards, performance is going down the drain :(

pfitzseb commented 7 years ago

By the way, I'm still pretty unhappy with using double clicks to expand the result: There's not only a conflict with tree expansion, but also with text selection. Does anyone have an idea for a better way to expand results with clicks (the Alt-T shortcut is pretty nice, imho)?

MikeInnes commented 7 years ago

Yeah, maybe we should just stick with the keyboard shortcut for now.

Longer term it might be nice to have some kind of toolbar/tooltip that pops up over results to give you options (like this, or moving them to the console).

pfitzseb commented 7 years ago

Alright, I'm kinda happy with the latest iteration of this: Results look normal, and double clicking them does nothing special: screenshot_20170509_191411 But when hovering them, the results expand to the left and a Expand Result button appears, which will do exactly that on a single click: screenshot_20170509_191518 This doesn't change the position of anything inside of the result, so it's still easy to expand trees and such. The one disadvantage is that this is a bit distracting, since it can flicker quite a bit. Might be able to work around that though. Thoughts?

pfitzseb commented 7 years ago

Okay, I liked the toolbar idea too much to go with anything else: newresults Opinions, anyone (e.g. @MikeInnes or @ChrisRackauckas)?

MikeInnes commented 7 years ago

That's pretty nice. I think it needs to be a bit more subtle, maybe just knock down the transparency a bit, and it would be nicer on the ~~left~~ right.

pfitzseb commented 7 years ago

Yeah, having the toolbar half transparent when not hovering it is much nicer. Do you think the toolbar should extend to the right, or is upwards fine and you'd rather have it on the right of the result?

MikeInnes commented 7 years ago

Yeah I think above and on the right is the right choice

pfitzseb commented 7 years ago

Not totally convinced about having the toolbar to the right. I mean, I can see the appeal of not having it in front of code, but imho that makes it a bit to unpredictable since results can change their width pretty arbitrarily (this also nessecitates much longer mouse movements): newresults

MikeInnes commented 7 years ago

I'm getting a lot of "Cannot read property 'getBoundingClientRect' of null" trying this out (triggered by focusing the editor)

pfitzseb commented 7 years ago

Huh, interesting. Can you try reverting the last commit? I didn't get these errors at all, but maybe that depends on system speed or something.

pfitzseb commented 7 years ago

Alright, so I finally benchmarked this stuff properly: Without any result resizing and ~300 overlay decorations Atom gives something like 60-80ms frametimes (when resizing the current editor), while the result resizing will give something between 60ms and 100ms. Without any decorations I'm getting 30-40ms.

All in all, this doesn't seem too bad -- Atom manages to have bad performance without me doing anything at all :)

@MikeInnes Care to try this again and let me know if you're still getting 'getBoundingClientRect' of null errors?

MikeInnes commented 7 years ago

Yeah those errors seem to be fixed. Perhaps try the toolbar on the left underneath? It should be possible to get the positioning without any js, I think.

pfitzseb commented 7 years ago

Sure, we don't need JS for that, I just wanted to reuse our generic Tooltip implementation instead of making something new. Is there a reason for not using JS, apart from possible performance problems that should be pretty minor all in all?

MikeInnes commented 7 years ago

That's fair, just that using CSS for the positioning should avoid problems when results move around (which the generic tooltip could do too, right?)

This is without looking at the code, so feel free to ignore if it doesn't make sense. I'll give this a proper do-over during the week.

pfitzseb commented 7 years ago

Ah right, forgot about the repositioning -- will rearrange things so we can use css (or at least have the tooltip attached to the result).

pfitzseb commented 7 years ago

This now needs https://github.com/JunoLab/atom-julia-client/commit/27640d32e27cb7d2fd6e129bf9477a557eaa7c90 to work. The toolbar is now css positioned, ~~but there's still an issue with very small results and toolbar positioning~~. Apart from that this should be at least a bit nicer than before... And it's JS now! :D

MikeInnes commented 7 years ago

Currently I get "Uncaught SyntaxError: Failed to execute 'add' on 'DOMTokenList': The token provided must not be empty." on first use.

Also, I think there's a CSS change in here that breaks wrapping; e.g. for a 2D array

pfitzseb commented 7 years ago

Are you on the julia-client branch referenced above? If not that seems to be the most likely cause of that error. CSS issue is fixed now.

MikeInnes commented 7 years ago

That fixes it, thanks. Might be worth aiming for backwards compatibility to make the transition smoother.

pfitzseb commented 7 years ago

Yeah, I thought about that too, but ultimately concluded that r.view.classList.add 'julia' is super iffy anyways -- we absolutely can't change anything about our result structure without breaking that line (or adding a ResultView.classList.add wrapper *shudder*).

MikeInnes commented 7 years ago

Agreed. Maybe we can release that removal early, if it doesn't break too much.

Will be worth checking that this doesn't break proto-repl as well.

pfitzseb commented 7 years ago

I've implemented a Send to Console button now: https://github.com/JunoLab/atom-julia-client/pull/344/commits/19e382954c5ffea9cd668ca2ceb66f8c685581e1 It works like a charm, but the implementation is super ugly... not sure what a nice API would be.

Still haven't done any proper benchmarks though... Have you noticed any problems so far, @MikeInnes?

KristofferC commented 7 years ago

I am not sure if this was introduced with this PR but there seems to be some problem with showing a string containing initial whitespace:

It left me scratching my head a while.

pfitzseb commented 7 years ago

@KristofferC That should be fixed since https://github.com/JunoLab/atom-ink/pull/127/commits/72e86acf05ac74a5d3d36a6a06cddc06c8246629, so try getting on the latest commit on this branch? Works fine for me at least.

pfitzseb commented 7 years ago

@MikeInnes Just thought about performance again: IIUC we could just use etch for this and get all those optimizations for free: "etch.update is asynchronous, batching multiple DOM updates together in a single animation frame for efficiency". Not sure why I didn't think of that sooner... probably because I still don't really understand how to use etch idiomatically :D

And regarding proto-repl: I think submitting a PR would be better -- mocking a DOM element seems very hackish to me ;)

MikeInnes commented 7 years ago

Etch might be useful insofar as it should hook into Atom's update cycle (which is supposed to be a Good Thing, but hey, this is basically voodoo to me).

Again though, I think you've already got batching and async in the code you've written. If you write something like

requestAnimationFrame(() => {
  for blah {
    readDom()
    writeDom()
  }
}

Then the animation frame is async and, as long as the reads and writes don't use callbacks, everything is batched together. Am I missing something here?

pfitzseb commented 7 years ago

Etch might be useful insofar as it should hook into Atom's update cycle (which is supposed to be a Good Thing, but hey, this is basically voodoo to me).

Yeah, my thoughts exactly.

If I understand things correctly, then

requestAnimationFrame(() => {
  for blah {
    readDom()
    writeDom()
  }
}

is bad, while

requestAnimationFrame(() => {
  for blah {
    readDom()
  }
  for blah {
    writeDom()
  }
}

is good (or at least better). Interleaving reads and writes to the DOM is bad insofar that the write has to trigger a relayout/repaint/whatever for the read to be correct. Chromium should be able to batch all the reads and writes together in the second version, which means that my usage of fastdom here should be unnessecary. The "multiple updates batched together" part is kinda handled by the timeout I think, but maybe debounceing the animation is worthwhile too (I think I had that previously).

So all in all, etch wouldn't bring us much here feature-wise, but it might make this easier to maintain (or something) and hook into Atom's rendering.

MikeInnes commented 7 years ago

Ok, I had in mind that the relayout would only be triggered once the JS frame ended, but that could be totally wrong. (I should really just read up on this a bit more so we're not just doing rain dances for the performance gods :)

Without knowing how fastdom works I think we risk things becoming interleaved with Atom's reads/writes anyway, so I think we may be better off just handwriting that version. Like I said, though, don't feel it has to block this PR, this is something we can experiment with and iterate on (and it's probably my turn to do some of the profiling work on this).

pfitzseb commented 7 years ago

Okay, I got rid of fastdom (because it doesn't do anything anymore) and made the setTimeout change (because it does indeed make more sense like this). Performance seems good enough for now (this isn't drastically slower than Atom's build in overlay handling), so barring any objections I'll merge this soonish. 🎉

MikeInnes commented 7 years ago

Amazing 🎆 Your reward will be with your comrades in Valhalla, or maybe in the Gitter room, whichever comes first.

JunoLab / atom-ink

Inline result overhaul #127