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

Inline result overhaul #127

Merged
merged 121 commits into from
May 25, 2017
Merged

Inline result overhaul #127

merged 121 commits into from
May 25, 2017

Conversation

pfitzseb
Copy link
Member

@pfitzseb pfitzseb commented Apr 20, 2017

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:
    result1
    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):
    result2
  • It's now possible to select text in results and copy it using Ctrl-C (or Cmd-C):
    result3
    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:

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

@Datseris
Copy link

please, please yes!!!

keymaps/ink.cson Outdated
'atom-text-editor:not([mini])':
'escape': 'inline:clear-current'

'.platform-darwin atom-text-editor:not([mini])':
'cmd-i cmd-c': 'inline-results:clear-all'
'cmd-t': 'inline-results:toggle'
Copy link
Member

Choose a reason for hiding this comment

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

This opens the file switcher on mac. alt-t is actually not taken (aside from inserting a special character) so that may actually be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, okay. Mac keybindings are always so confusing to me... :D

@MikeInnes
Copy link
Member

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
Copy link
Member Author

pfitzseb commented Apr 21, 2017

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

pfitzseb commented May 4, 2017

  • 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 pfitzseb merged commit b7ad0f4 into master May 25, 2017
@pfitzseb pfitzseb deleted the sp/newrsults branch May 25, 2017 09:59
@MikeInnes
Copy link
Member

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

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.

4 participants