Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Ruby code is not getting the right colors. #252

Open
cristianstu opened this issue Nov 6, 2018 · 14 comments
Open

Ruby code is not getting the right colors. #252

cristianstu opened this issue Nov 6, 2018 · 14 comments

Comments

@cristianstu
Copy link

Description

Ruby code is not getting the right colors.
Light theme seems to be ok.

Steps to Reproduce

  1. Upload to atom >= 1.32.0
  2. Open any ruby file.

Expected behavior: Show the file like this

image

Actual behavior:

image

Reproduces how often: Always

Versions

Atom : 1.32.1
Electron: 2.0.9
Chrome : 61.0.3163.100
Node : 8.9.3

apm 2.1.2
npm 6.2.0
node 8.9.3 x64
atom 1.32.1
python 2.7.15+
git 2.19.1

Additional Information

Seems like syntax--ruby class is missing on hierarchy. Switching to Ruby On Rails syntax fix the problem.

@Arcanemagus
Copy link

Atom v1.32.0 enabled a new parsing system by default. If you have specific issues about how the syntax highlighting has changed that you feel are mistakes, please take a look at the open issues on atom/language-ruby. If the specific problem that you're experiencing is not already being tracked there, please open a new issue on that repository.

If you think that this issue is related to this theme in particular could you outline what scopes aren't being colored?

Tree-sitter is a new grammar system that enables a number of advanced features including more rigorous syntax highlighting, which means more tokens will be detected correctly or more correctly than in the past. This means that colors may change because of this new information. What we're mostly looking for are situations where the underlying scopes of tokens are incorrect, not that the color is different than it was before. Some differences may be expected, such as variables now being recognized the same in every location they appear instead of differently based on the context. In general, we don't consider that a bug.

If you want to see how Tree-sitter identifies the token at the current cursor position, you can open the Command Palette using Cmd+Shift+P on macOS or Ctrl+Shift+P on other platforms, then search for and execute "Editor: Log Cursor Syntax Tree Scope". If the scope at the bottom is incorrect, please file an issue and describe what you expected instead.

Thanks!

@cristianstu
Copy link
Author

@Arcanemagus I think everything being identified correctly but is not getting the proper colors. (checked with Log Cursor)

Class names, for instance. The color should be @yellow

https://github.com/atom/solarized-dark-syntax/blob/c4ed1ac41a5dd2120b912a766f4989e5b346bc33/styles/syntax/ruby.less#L37

is not being applied because it depends on parent container class syntax--ruby, but that class is missing in the hierarchy.

I guess adding that class is a language-ruby responsibility, or it was before, but if they have changed that class, this package will need to change that styles, right?

Thanks!

@simurai
Copy link

simurai commented Nov 14, 2018

I can reproduce this as well. It seems that the example code of

class Article < ApplicationRecord
  belongs_to :category, optional: true
end

doesn't get wrapped with the syntax--source syntax--ruby classes on the parent <span>:

screen shot 2018-11-14 at 2 27 43 pm

Is that intentional? It means that all the styling in https://github.com/atom/solarized-dark-syntax/blob/c8a93df669e7a9a8ca062eb35426911bf864ed3b/styles/syntax/ruby.less#L1 doesn't get applied.

Versions

Atom    : 1.34.0-nightly12
Electron: 2.0.12
Chrome  : 61.0.3163.100
Node    : 8.9.3

I'll move this over to language-ruby.

@simurai simurai transferred this issue from atom/solarized-dark-syntax Nov 14, 2018
@Ben3eeE
Copy link
Contributor

Ben3eeE commented Nov 14, 2018

@simurai The missing syntax--source syntax--ruby should be fixed in Atom 1.32.2 and later via 0208636.

Which version of the package is nightly12 using?

@Ben3eeE
Copy link
Contributor

Ben3eeE commented Nov 14, 2018

Running a build from latest master looks like this with solarized-dark-syntax:
image

@simurai
Copy link

simurai commented Nov 14, 2018

should be fixed in Atom 1.32.2 and later

Nice! 🙌

Which version of the package is nightly12 using?

Ouch.. sorry, this is embarrassing. 😅 I had some other example code above

class Article < ApplicationRecord
  belongs_to :category, optional: true
end

... which then probably messed up the highlighting. OK, works also in nightly12. 👍 Closing! And thanks for the checking. @Ben3eeE

@simurai simurai closed this as completed Nov 14, 2018
@josephchoe
Copy link

If some of the colors still don't look right on the solarized dark theme, but the syntax--ruby classes are being populated on the hierarchy, does that mean this issue needs to go back to the atom/solarized-dark-syntax repository?

For example:

screen shot 2018-11-19 at 2 18 35 am

@simurai
Copy link

simurai commented Nov 20, 2018

@josephchoe does that mean this issue needs to go back to the atom/solarized-dark-syntax repository?

Yes, it seems that require gets intentionally overridden here: https://github.com/atom/solarized-dark-syntax/blob/74bc0df24ca7493d9d8bcfcaedccb5cc05c2c2f9/styles/syntax/ruby.less#L123-L125

Removing it, makes require blue. Not sure if that is prefered?

image

@josephchoe
Copy link

I think it's supposed to be like this? Though I might be remembering wrong.

screen shot 2018-11-22 at 9 57 07 pm

@simurai
Copy link

simurai commented Dec 3, 2018

@josephchoe does that mean this issue needs to go back to the atom/solarized-dark-syntax repository?
@josephchoe I think it's supposed to be like this?

Sorry, you're right. The scopes seem to be different:

Before (TextMate) After (Tree-sitter)
textmate tree-sitter
solarized-textmate solarized-tree-sitter

But it's different for all themes, not just Solarized. Not sure if it's possible to change the Tree-sitter grammar so that it matches TextMate's?

I'll re-open it here. It might be a duplicate of any of these tree-sitter .

@simurai simurai reopened this Dec 3, 2018
@maxbrunsfeld
Copy link
Contributor

I think it's supposed to be like this?

IMO the new behavior makes more sense. require isn't a keyword; it's a built-in function, so I think support.function is the right scope. If solarized doesn't highlight support.function, then I think a lot of languages are going to look wrong, not just Ruby, because that's the scope used for "special" functions in many languages.

@gsmetal
Copy link

gsmetal commented Apr 12, 2019

Hello, any updates here? There are a lot of problems with highlighting on tree sitter. For example:
image

  1. Aren't include and extend built-in functions same as require?
  2. Function's argument with assignment is not highlighted as argument.
  3. super is not highlighted at all.
  4. Last var is highlighted as function, but it's definitely not a function.
  5. ...

Edited: I am using atom 1.36.0, it's monokai syntax theme, but that problems on all themes including builtin.

@Ben3eeE
Copy link
Contributor

Ben3eeE commented Apr 12, 2019

  1. Aren't include and extend built-in functions same as require?

I don't know Ruby so I can't answer this question but if they are you can add them here and change it from exact to match so it's a regex. There are quite a few built in functions that are not scoped.

{exact: 'require', scopes: 'support.function'}

  1. Function's argument with assignment is not highlighted as argument.

This should be easy to fix by just adding:
'optional_parameter > identifier': 'variable.parameter.function'

Here:

'method_parameters > identifier': 'variable.parameter.function'

  1. super is not highlighted at all.

Same thing. Add a line for it 'super': 'the-scope.i-want' or '"super"': 'the-scope.i-want'. The one that works is the correct one.

  1. Last var is highlighted as function, but it's definitely not a function.

Not as easy to fix as the other things.

  1. ...

...

PRs are welcome to add these things.

@chbk
Copy link
Contributor

chbk commented Apr 28, 2021

I believe this can be closed when #291 is merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants