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

Multiple, custom line number gutters #17736

Merged
merged 22 commits into from Aug 21, 2018

Conversation

Projects
None yet
3 participants
@smashwilson
Member

smashwilson commented Jul 24, 2018

To complete atom/github#1512, I need to be able to add multiple Gutters to an editor, each of which has a custom labelling function. I'm emulating this effect:

screen shot 2018-07-24 at 9 41 31 am

I did make a bit of an effort to do this with the existing Gutter API. I was able to get the root DOM element of an added Gutter with getElement() and render a React portal into it, but I ran into problems duplicating the logic from LineNumberGutterComponent because I couldn't find a way to trigger re-renders of the React component when the parent Etch component re-rendered, so the two would quickly get out of sync.


This adds a type property to the options object accepted by TextEditor::addGutter(). If set to 'line-number', a LineNumberGutterComponent is created instead of a CustomGutterComponent, and a labelFn option may be supplied to customize the created line number labels.


Remaining
  • Decorations added with TextEditor::decorateMarker(marker, {type: 'line-number', ...}) should only decorate the One True line-number gutter.
  • Support a className option in TextEditor::addGutter(). Add the provided CSS class to the gutter's root element. This will make it easier to style line-number gutters.
  • Support onMouseDown and onMouseMove callbacks in TextEditor::addGutter().
  • Tests green.

smashwilson added some commits Jul 24, 2018

smashwilson added some commits Jul 24, 2018

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 24, 2018

Member

Progress:

atom.commands.add('atom-workspace', 'me:add-line-number-gutters', () => {
  const editor = atom.workspace.getActiveTextEditor()
  if (!editor) {
    return;
  }

  editor.addGutter({
    name: 'custom-1',
    type: 'line-number',
    priority: 2,
    labelFn: ({bufferRow}) => `1.${bufferRow}`
  })

  editor.addGutter({
    name: 'custom-2',
    type: 'line-number',
    priority: 3,
    labelFn: ({screenRow}) => `2.${screenRow}`
  })
})

screen shot 2018-07-24 at 10 11 57 am

The custom gutters scroll correctly and everything, too 🚀

One interesting side-effect is that the line-number decorations from the git-diff package show up on every line-number gutter. That's a bit odd, so I'll fix those to target the One True line-number gutter by default; you should still be able to go through Gutter::decorateMarker() to add line-number decorations to other line-number gutters.

Member

smashwilson commented Jul 24, 2018

Progress:

atom.commands.add('atom-workspace', 'me:add-line-number-gutters', () => {
  const editor = atom.workspace.getActiveTextEditor()
  if (!editor) {
    return;
  }

  editor.addGutter({
    name: 'custom-1',
    type: 'line-number',
    priority: 2,
    labelFn: ({bufferRow}) => `1.${bufferRow}`
  })

  editor.addGutter({
    name: 'custom-2',
    type: 'line-number',
    priority: 3,
    labelFn: ({screenRow}) => `2.${screenRow}`
  })
})

screen shot 2018-07-24 at 10 11 57 am

The custom gutters scroll correctly and everything, too 🚀

One interesting side-effect is that the line-number decorations from the git-diff package show up on every line-number gutter. That's a bit odd, so I'll fix those to target the One True line-number gutter by default; you should still be able to go through Gutter::decorateMarker() to add line-number decorations to other line-number gutters.

smashwilson added some commits Jul 24, 2018

@smashwilson smashwilson referenced this pull request Jul 24, 2018

Open

Render file patches with a decorated Editor #1512

67 of 89 tasks complete

smashwilson added some commits Jul 24, 2018

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 10, 2018

Member

I'm a bit leery about touching code in TextEditorElement and friends on my own without an extra set of eyes for safety. Reviewers, when you get a moment, could you look for any obvious red flags for performance or correctness? API change feedback is welcome as well if you have any. 🙇

Member

smashwilson commented Aug 10, 2018

I'm a bit leery about touching code in TextEditorElement and friends on my own without an extra set of eyes for safety. Reviewers, when you get a moment, could you look for any obvious red flags for performance or correctness? API change feedback is welcome as well if you have any. 🙇

@maxbrunsfeld

The code all looks good to me!

Would it be worth adding a test for the rendering side of things in text-editor-component-spec.js, where you add a gutter with a custom labelFn, and assert that the right labels show up, and that the line numbers and line tiles have the right widths?

@daviwil

👍 ⚡️

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Aug 21, 2018

Member

Would it be worth adding a test for the rendering side of things in text-editor-component-spec.js, where you add a gutter with a custom labelFn, and assert that the right labels show up, and that the line numbers and line tiles have the right widths?

👍 I'll get this in place, then merge.

Thanks for the eyes 🙇

Member

smashwilson commented Aug 21, 2018

Would it be worth adding a test for the rendering side of things in text-editor-component-spec.js, where you add a gutter with a custom labelFn, and assert that the right labels show up, and that the line numbers and line tiles have the right widths?

👍 I'll get this in place, then merge.

Thanks for the eyes 🙇

@smashwilson smashwilson merged commit 4e15017 into master Aug 21, 2018

3 checks passed

VSTS: Atom Pull Requests 20180821.9 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the aw/custom-line-number-gutter branch Aug 21, 2018

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 21, 2018

Contributor

💥 Awesome job

Contributor

maxbrunsfeld commented Aug 21, 2018

💥 Awesome job

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment