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

Highlight matching line #290

Merged
merged 6 commits into from Sep 5, 2017

Conversation

Projects
None yet
7 participants
@seattlevine
Contributor

seattlevine commented Jun 7, 2017

Requirements

  • In regards to #180, this is a bit of a simpler modification. It's purpose is to highlight the line number with the matching bracket

Description of the Change

bracket-matcher already creates markers and decorates them. This pull request adds a highlight decoration to the line-number gutter if it is there.

Alternate Designs

I attempted to merge #180 into the current code base, but that is way out of date and there were modifications I did not understand. This is the first package I've worked with in Atom, and my first time w/ CoffeeScript, so I kept it simple.

Benefits

If you happen to have long blocks of code, you can more easily find the matching bracket.

Possible Drawbacks

  • Only works if you are showing line numbers. (Atom 1.18 and earlier only. In Atom 1.19+, this modification works when line numbers are hidden.)

Applicable Issues

#97

seattlevine added some commits Jun 7, 2017

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jun 7, 2017

Member

But really, who would have a light scheme? There is a known strong correlation between light color scheme users and short function writers, so it should be fine.

😞

Have you tried using the theme variables that are available?

Member

50Wliu commented Jun 7, 2017

But really, who would have a light scheme? There is a known strong correlation between light color scheme users and short function writers, so it should be fine.

😞

Have you tried using the theme variables that are available?

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Jun 7, 2017

Contributor

Have you tried using the theme variables that are available?

Can you throw me a bit more of a tip? I see how I could get the loaded themes, and presumably figure out the active theme and from there the style. But I don't see how to set the style. StyleManager has no setter methods.

Contributor

seattlevine commented Jun 7, 2017

Have you tried using the theme variables that are available?

Can you throw me a bit more of a tip? I see how I could get the loaded themes, and presumably figure out the active theme and from there the style. But I don't see how to set the style. StyleManager has no setter methods.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jun 7, 2017

Member

You should be able to directly reference variables declared in https://github.com/atom/atom/blob/master/static/variables/ui-variables.less. Each theme then overrides those variables. Choose one that you think matches the purpose.

Member

50Wliu commented Jun 7, 2017

You should be able to directly reference variables declared in https://github.com/atom/atom/blob/master/static/variables/ui-variables.less. Each theme then overrides those variables. Choose one that you think matches the purpose.

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Jul 6, 2017

Contributor

Any other comments/questions on this?

Contributor

seattlevine commented Jul 6, 2017

Any other comments/questions on this?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jul 13, 2017

Member

This looks good to me. @ungb maybe you could take this for a quick test run?

Member

50Wliu commented Jul 13, 2017

This looks good to me. @ungb maybe you could take this for a quick test run?

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Aug 8, 2017

Contributor

Atom 1.19 renders the line number gutter even if line numbers are off. This also allows this modification to work even when line numbers are off.

Contributor

seattlevine commented Aug 8, 2017

Atom 1.19 renders the line number gutter even if line numbers are off. This also allows this modification to work even when line numbers are off.

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Aug 31, 2017

Contributor

Any decision on this?

Contributor

seattlevine commented Aug 31, 2017

Any decision on this?

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Sep 1, 2017

Contributor

Hey @seattlevine,

I've just tested this and everything looks good to me! I tested this with line numbers on and off off atom/atom in master.

I've reached out to the team for review and will let you know what I hear back.

Contributor

ungb commented Sep 1, 2017

Hey @seattlevine,

I've just tested this and everything looks good to me! I tested this with line numbers on and off off atom/atom in master.

I've reached out to the team for review and will let you know what I hear back.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Sep 1, 2017

Contributor

@seattlevine Thanks for adding this new enhancement.

One of the concerning things is having this on by default since it changes the default behavior. I'll merge this change if we can have the default set to false for now. Please let me know if you can make that change. If not, let me know and I can help and get this PR merged.

Contributor

ungb commented Sep 1, 2017

@seattlevine Thanks for adding this new enhancement.

One of the concerning things is having this on by default since it changes the default behavior. I'll merge this change if we can have the default set to false for now. Please let me know if you can make that change. If not, let me know and I can help and get this PR merged.

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Sep 1, 2017

Contributor
Contributor

seattlevine commented Sep 1, 2017

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Sep 2, 2017

Contributor

Ah - misread that - you were looking for me to make that change. I'll do it soon.

Contributor

seattlevine commented Sep 2, 2017

Ah - misread that - you were looking for me to make that change. I'll do it soon.

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Sep 2, 2017

Contributor

OK - all set.

Contributor

seattlevine commented Sep 2, 2017

OK - all set.

@Ben3eeE Ben3eeE added the needs-review label Sep 3, 2017

@ungb ungb merged commit 7ff5d47 into atom:master Sep 5, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Sep 5, 2017

Contributor

Thanks for making these changes @seattlevine!

Contributor

ungb commented Sep 5, 2017

Thanks for making these changes @seattlevine!

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Sep 6, 2017

Contributor

@ungb you can decide if this closes #97 or not

Contributor

seattlevine commented Sep 6, 2017

@ungb you can decide if this closes #97 or not

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Sep 7, 2017

Contributor

@seattlevine I'll add a comment to that issue and close it out once this change is in atom core. currently. We're working on fixing #314 before we update atom/atom to use the latest from bracket-matcher.

Contributor

ungb commented Sep 7, 2017

@seattlevine I'll add a comment to that issue and close it out once this change is in atom core. currently. We're working on fixing #314 before we update atom/atom to use the latest from bracket-matcher.

@paradoxxxzero

This comment has been minimized.

Show comment
Hide comment
@paradoxxxzero

paradoxxxzero Oct 4, 2017

This makes the lineno almost unreadable with atom one dark theme.

screenshot from 2017-10-04 15-01-19

paradoxxxzero commented Oct 4, 2017

This makes the lineno almost unreadable with atom one dark theme.

screenshot from 2017-10-04 15-01-19

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Oct 4, 2017

Contributor

The highlight uses 'text-color-subtle', so you can set that to what you like.

Contributor

seattlevine commented Oct 4, 2017

The highlight uses 'text-color-subtle', so you can set that to what you like.

@seattlevine seattlevine deleted the seattlevine:highlight-matching-line branch Oct 4, 2017

@paradoxxxzero

This comment has been minimized.

Show comment
Hide comment
@paradoxxxzero

paradoxxxzero Oct 4, 2017

Are you suggesting I need to fork a default theme for it to be readable?

paradoxxxzero commented Oct 4, 2017

Are you suggesting I need to fork a default theme for it to be readable?

@seattlevine

This comment has been minimized.

Show comment
Hide comment
@seattlevine

seattlevine Oct 4, 2017

Contributor
  • I thought you could override text-color-subtle. Instead, you can override the style directly in your styles.less. Open it via menu Atom -> Stylesheet...
    Add this:
    .line-number.bracket-matcher { background-color: <my_color>; }

  • You can also turn off the feature via the bracket matcher setting 'Highlight Matching Line Number'.

Contributor

seattlevine commented Oct 4, 2017

  • I thought you could override text-color-subtle. Instead, you can override the style directly in your styles.less. Open it via menu Atom -> Stylesheet...
    Add this:
    .line-number.bracket-matcher { background-color: <my_color>; }

  • You can also turn off the feature via the bracket matcher setting 'Highlight Matching Line Number'.

@paradoxxxzero

This comment has been minimized.

Show comment
Hide comment
@paradoxxxzero

paradoxxxzero Oct 5, 2017

Yes I did do that but I still think this is a problem: a default package should work at least with all default themes. I think using a text color variable for a background is not a great idea.

paradoxxxzero commented Oct 5, 2017

Yes I did do that but I still think this is a problem: a default package should work at least with all default themes. I think using a text color variable for a background is not a great idea.

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE
Member

Ben3eeE commented Oct 5, 2017

/cc: @simurai

@lexicalunit

This comment has been minimized.

Show comment
Hide comment
@lexicalunit

lexicalunit Oct 6, 2017

Love this! But I also had the issue where the line number was super hard to read. Quickly coded up this solution to make the line number readable no matter if I'm using a light or dark theme (I switch between the two sometimes depending on the ambient light in the room).

.line-number.bracket-matcher {
  & when (luminance(@syntax-background-color) <= 50%) {
    background-color: lighten(average(@syntax-background-color, @syntax-selection-color), 10%);
  }
  & when (luminance(@syntax-background-color) > 50%) {
    background-color: darken(average(@syntax-background-color, @syntax-selection-color), 10%);
  }
}

screen shot 2017-10-06 at 11 02 09 am

screen shot 2017-10-06 at 11 01 52 am

lexicalunit commented Oct 6, 2017

Love this! But I also had the issue where the line number was super hard to read. Quickly coded up this solution to make the line number readable no matter if I'm using a light or dark theme (I switch between the two sometimes depending on the ambient light in the room).

.line-number.bracket-matcher {
  & when (luminance(@syntax-background-color) <= 50%) {
    background-color: lighten(average(@syntax-background-color, @syntax-selection-color), 10%);
  }
  & when (luminance(@syntax-background-color) > 50%) {
    background-color: darken(average(@syntax-background-color, @syntax-selection-color), 10%);
  }
}

screen shot 2017-10-06 at 11 02 09 am

screen shot 2017-10-06 at 11 01 52 am

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Dec 8, 2017

Member

There are also these variables that might fit:

.line-number.bracket-matcher {
  color: @syntax-gutter-text-color-selected;
  background-color: @syntax-gutter-background-color-selected;
}

screen shot 2017-12-08 at 2 25 53 pm

Although not all themes made it strong enough. For example one-dark-syntax:

screen shot 2017-12-08 at 2 30 18 pm

I'll make a PR and update it.

Member

simurai commented Dec 8, 2017

There are also these variables that might fit:

.line-number.bracket-matcher {
  color: @syntax-gutter-text-color-selected;
  background-color: @syntax-gutter-background-color-selected;
}

screen shot 2017-12-08 at 2 25 53 pm

Although not all themes made it strong enough. For example one-dark-syntax:

screen shot 2017-12-08 at 2 30 18 pm

I'll make a PR and update it.

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