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

Filter nameless grammars from selection list #34

Merged
merged 7 commits into from Sep 19, 2017

Conversation

Projects
None yet
4 participants
@Alhadis
Contributor

Alhadis commented Oct 22, 2016

Prior to v1.11, the selector-list omitted from its results any grammars which lack a name field. This is desirable, because such grammars generally exist for internal use only:

Figure 1

They've reappeared in the selector results again, which convinces me it might've been a bug instead of a feature. This PR enforces their exclusion specifically, enabling grammar authors a means of hiding "helper" grammars from the end user.

/cc @lierdakil

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 23, 2016

👍

I probably should also mention that https://github.com/atom/settings-view/blob/master/lib/package-grammars-view.coffee#L50 needs the similar kind of change, since settings-vew shows nameless grammars as undefined Grammar.

Honestly, at this point I'm thinking along the lines of removing those from grammars completely and providing a local instance of GrammarRegistry in language-haskell...

lierdakil commented Oct 23, 2016

👍

I probably should also mention that https://github.com/atom/settings-view/blob/master/lib/package-grammars-view.coffee#L50 needs the similar kind of change, since settings-vew shows nameless grammars as undefined Grammar.

Honestly, at this point I'm thinking along the lines of removing those from grammars completely and providing a local instance of GrammarRegistry in language-haskell...

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Oct 23, 2016

Contributor

Ah damn, that's a good catch. Never noticed that, actually. Next PR coming up.

And yeah, I'll admit the Haskell package is a bit of an unorthodox case, but the truth of the matter is that there're many packages out there that use "modular" grammars for highlighting complicated content (such as regular expressions).

Then you have stuff like this and this which isn't nameless, but should damn well be hidden as a selection choice. I can't imagine how many users were expecting an Atomised Org-mode when they assigned TODO as a plain-text checklist's grammar...

Contributor

Alhadis commented Oct 23, 2016

Ah damn, that's a good catch. Never noticed that, actually. Next PR coming up.

And yeah, I'll admit the Haskell package is a bit of an unorthodox case, but the truth of the matter is that there're many packages out there that use "modular" grammars for highlighting complicated content (such as regular expressions).

Then you have stuff like this and this which isn't nameless, but should damn well be hidden as a selection choice. I can't imagine how many users were expecting an Atomised Org-mode when they assigned TODO as a plain-text checklist's grammar...

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Oct 23, 2016

Good point about hyperlink and todo.

lierdakil commented Oct 23, 2016

Good point about hyperlink and todo.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Dec 31, 2016

Member

This looks like it was actually intentional, though that change was way back in 2014. Like you said, name is optional: therefore there is the possibility that there are some grammars out there that don't actually have a name yet are expected to show up in the grammar selector.

Wouldn't #32 be a better solution (without the config option)?

Member

50Wliu commented Dec 31, 2016

This looks like it was actually intentional, though that change was way back in 2014. Like you said, name is optional: therefore there is the possibility that there are some grammars out there that don't actually have a name yet are expected to show up in the grammar selector.

Wouldn't #32 be a better solution (without the config option)?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Dec 31, 2016

Contributor

No. It's poor UX for a grammar to be listed as scope.whatever. Authors should be specifying a name for their grammars in the first place.

Contributor

Alhadis commented Dec 31, 2016

No. It's poor UX for a grammar to be listed as scope.whatever. Authors should be specifying a name for their grammars in the first place.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis May 11, 2017

Contributor

@50Wliu Just curious, has there been any movement with reviewing this PR?

Contributor

Alhadis commented May 11, 2017

@50Wliu Just curious, has there been any movement with reviewing this PR?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu May 11, 2017

Member

No. I'll put it on the personal backlog since it seems like a small change.

Member

50Wliu commented May 11, 2017

No. I'll put it on the personal backlog since it seems like a small change.

@50Wliu 50Wliu self-assigned this May 11, 2017

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jun 17, 2017

Member

Ok. I think that it makes sense for grammars that are missing either the name field or have no fileTypes set to not appear in the grammar selector. This way TODO and hyperlink are hidden as well.

Member

50Wliu commented Jun 17, 2017

Ok. I think that it makes sense for grammars that are missing either the name field or have no fileTypes set to not appear in the grammar selector. This way TODO and hyperlink are hidden as well.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jun 17, 2017

Contributor

This way TODO and hyperlink are hidden as well.

Heh. Yeah, I still remember how confused I was the first time I saw them listed in the languages list. Took a long time (and numerous grammar packages) before I figured that mystery out...

Contributor

Alhadis commented Jun 17, 2017

This way TODO and hyperlink are hidden as well.

Heh. Yeah, I still remember how confused I was the first time I saw them listed in the languages list. Took a long time (and numerous grammar packages) before I figured that mystery out...

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Aug 16, 2017

So... any updates on this?

lierdakil commented Aug 16, 2017

So... any updates on this?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Aug 27, 2017

Contributor

@50Wliu Any progress...?

Contributor

Alhadis commented Aug 27, 2017

@50Wliu Any progress...?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Aug 28, 2017

Member

It doesn't look like my comment #34 (comment) has been addressed?

Member

50Wliu commented Aug 28, 2017

It doesn't look like my comment #34 (comment) has been addressed?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Aug 28, 2017

Contributor

Uh no, because some grammars are intended to be assigned manually, or assigned through programmatic means.

Contributor

Alhadis commented Aug 28, 2017

Uh no, because some grammars are intended to be assigned manually, or assigned through programmatic means.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Aug 28, 2017

Contributor

@50Wliu Here's another example of a legitimate grammar that lacks a fileType. The intermediate output language generated by troff has no file extension, because it's intended to be piped between processes:

troff -Tps -man source.man | spool

However, anybody who's writing a Troff postprocessor will be thankful for the highlighting if they have to debug raw ditroff output:

x T pdf
x res 72000 1 1
x init
p1
V12000
H72000
DFd
wx font 5 TR
f5
s32000
h8000
v96000
md
tABC
wh8000
t`123'
wh8000
tXYZ
n12000 0
s10
f1
H0
s10
f1
V0
H720
V120
ch
50e44l28l28o50,w58w72o50r33l28dn120 0
x trailer
V7920
x stop
Contributor

Alhadis commented Aug 28, 2017

@50Wliu Here's another example of a legitimate grammar that lacks a fileType. The intermediate output language generated by troff has no file extension, because it's intended to be piped between processes:

troff -Tps -man source.man | spool

However, anybody who's writing a Troff postprocessor will be thankful for the highlighting if they have to debug raw ditroff output:

x T pdf
x res 72000 1 1
x init
p1
V12000
H72000
DFd
wx font 5 TR
f5
s32000
h8000
v96000
md
tABC
wh8000
t`123'
wh8000
tXYZ
n12000 0
s10
f1
H0
s10
f1
V0
H720
V120
ch
50e44l28l28o50,w58w72o50r33l28dn120 0
x trailer
V7920
x stop
@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 2, 2017

Contributor

This way TODO and hyperlink are hidden as well.

Hey wait a second, I forgot to ask... why is it critical that TODO and Hyperlink retain their name properties? If they're not seen in the grammar selection-list, then... what does it matter what they're called...?

Contributor

Alhadis commented Sep 2, 2017

This way TODO and hyperlink are hidden as well.

Hey wait a second, I forgot to ask... why is it critical that TODO and Hyperlink retain their name properties? If they're not seen in the grammar selection-list, then... what does it matter what they're called...?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 3, 2017

Member

Because, uh, the tmbundle had it. https://github.com/textmate/todo.tmbundle/blob/e77e5afc5c6b9d9387a1cbcc536755cead93e937/Syntaxes/TODO.tmLanguage#L9-L10 /shrug

So, anyway, spec/fixtures/language-with-no-name/grammars/b.json seems misleading because b.json does have a name. Could you create a new fixtures folder instead?

Member

50Wliu commented Sep 3, 2017

Because, uh, the tmbundle had it. https://github.com/textmate/todo.tmbundle/blob/e77e5afc5c6b9d9387a1cbcc536755cead93e937/Syntaxes/TODO.tmLanguage#L9-L10 /shrug

So, anyway, spec/fixtures/language-with-no-name/grammars/b.json seems misleading because b.json does have a name. Could you create a new fixtures folder instead?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 4, 2017

Contributor

What do you mean, "create a new folder"...? Don't you mean "rename the current fixtures folder so the name is more accurate?"

Contributor

Alhadis commented Sep 4, 2017

What do you mean, "create a new folder"...? Don't you mean "rename the current fixtures folder so the name is more accurate?"

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 4, 2017

Member

No, because a.json is in fact missing a name. I'm proposing the following folder structure:

- fixtures
  - language-with-no-name
    - grammars
      - a.json
  - language-with-a-name
    - grammars
      - b.json

Alternatively, do we even need b.json, given that all the other core languages have name?

Member

50Wliu commented Sep 4, 2017

No, because a.json is in fact missing a name. I'm proposing the following folder structure:

- fixtures
  - language-with-no-name
    - grammars
      - a.json
  - language-with-a-name
    - grammars
      - b.json

Alternatively, do we even need b.json, given that all the other core languages have name?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 5, 2017

Contributor

Alternatively, do we even need b.json, given that all the other core languages have name?

That's... actually a really good point. 😖 Will remove.

Contributor

Alhadis commented Sep 5, 2017

Alternatively, do we even need b.json, given that all the other core languages have name?

That's... actually a really good point. 😖 Will remove.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 5, 2017

Contributor

Done. The fact I didn't even need to modify a spec kinda proves how pointless that extra fixture was. 😀

Contributor

Alhadis commented Sep 5, 2017

Done. The fact I didn't even need to modify a spec kinda proves how pointless that extra fixture was. 😀

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 5, 2017

Member

What happens when someone chooses a grammar without a name? What does the Grammar Selector show in those scenarios, both in the modal panel and the status bar?

Member

50Wliu commented Sep 5, 2017

What happens when someone chooses a grammar without a name? What does the Grammar Selector show in those scenarios, both in the modal panel and the status bar?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 5, 2017

Contributor

Nothing.

The status bar displays the scopeName:
Figure 1: Status Bar

The selector simply defaults to the top entry ("Auto detect"):
Figure 2: Grammar Selector

Contributor

Alhadis commented Sep 5, 2017

Nothing.

The status bar displays the scopeName:
Figure 1: Status Bar

The selector simply defaults to the top entry ("Auto detect"):
Figure 2: Grammar Selector

@lierdakil

This comment has been minimized.

Show comment
Hide comment
@lierdakil

lierdakil Sep 7, 2017

What happens when someone chooses a grammar without a name? What does the Grammar Selector show in those scenarios, both in the modal panel and the status bar?

Isn't that a moot point though? How can someone select a grammar if it's not in grammar selector? Forcing it from code is an option, yes, but isn't the point of this whole PR that unnamed grammars aren't supposed to be used directly in TextEditor?

lierdakil commented Sep 7, 2017

What happens when someone chooses a grammar without a name? What does the Grammar Selector show in those scenarios, both in the modal panel and the status bar?

Isn't that a moot point though? How can someone select a grammar if it's not in grammar selector? Forcing it from code is an option, yes, but isn't the point of this whole PR that unnamed grammars aren't supposed to be used directly in TextEditor?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 7, 2017

Contributor

He was probably just checking to make sure there were no glaring anomalies in the UI when switching grammars programmatically. And I had to dig through code to remind myself how to set an editor's grammar the proper way (since the procedure for doing so has changed a number of times over the years... heh).

Contributor

Alhadis commented Sep 7, 2017

He was probably just checking to make sure there were no glaring anomalies in the UI when switching grammars programmatically. And I had to dig through code to remind myself how to set an editor's grammar the proper way (since the procedure for doing so has changed a number of times over the years... heh).

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 7, 2017

Contributor

@50Wliu, I think it's safe to merge this. I've been using this fork for almost a year now, and so far I've spotted zero regressions, and zero UI oddities.

And you know how astute I am with picking up on crap nobody else would notice or care about, right? ;)

Contributor

Alhadis commented Sep 7, 2017

@50Wliu, I think it's safe to merge this. I've been using this fork for almost a year now, and so far I've spotted zero regressions, and zero UI oddities.

And you know how astute I am with picking up on crap nobody else would notice or care about, right? ;)

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 18, 2017

Contributor

@50Wliu Anything I can do to move this along...?

Contributor

Alhadis commented Sep 18, 2017

@50Wliu Anything I can do to move this along...?

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Sep 18, 2017

@50Wliu I'm fine with merging this if you want to shepherd that through.

iolsen commented Sep 18, 2017

@50Wliu I'm fine with merging this if you want to shepherd that through.

@50Wliu 50Wliu merged commit 68b0ff8 into atom:master Sep 19, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 19, 2017

Contributor

Thank you so much!!! ❤️

Contributor

Alhadis commented Sep 19, 2017

Thank you so much!!! ❤️

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 19, 2017

Member

Thanks for creating the PR!

Member

50Wliu commented Sep 19, 2017

Thanks for creating the PR!

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Sep 19, 2017

Contributor

I'll be able to write a dedicated modelines package someday that encompasses both Emacs and Vim modelines, with injected highlighting that won't be listed in the selection list. :D

Emphasis on the word "someday". 😫

Contributor

Alhadis commented Sep 19, 2017

I'll be able to write a dedicated modelines package someday that encompasses both Emacs and Vim modelines, with injected highlighting that won't be listed in the selection list. :D

Emphasis on the word "someday". 😫

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