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

Limit QuickView to literal color names in some languages #8156

Merged
merged 8 commits into from Sep 4, 2014

Conversation

Projects
None yet
3 participants
@MarcelGerber
Contributor

MarcelGerber commented Jun 17, 2014

For #6836.
Limits QuickView to only literal color names in JS (including JSON), PHP and CoffeeScript.
Includes Unit tests and one new test file.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Jun 18, 2014

Member

I wonder if we should go further and switch to a whitelist -- respecting named colors only in CSS/LESS/SCSS files. I sometimes get the popover on words when editing plain .txt files, for example...

Member

peterflynn commented Jun 18, 2014

I wonder if we should go further and switch to a whitelist -- respecting named colors only in CSS/LESS/SCSS files. I sometimes get the popover on words when editing plain .txt files, for example...

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Jun 18, 2014

Contributor

On words, you'll still get a popover (right now). It's currently only disabled for object/array keys and functions.

Contributor

MarcelGerber commented Jun 18, 2014

On words, you'll still get a popover (right now). It's currently only disabled for object/array keys and functions.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Jun 24, 2014

Contributor

Definitely want something along these lines. Triage Complete.

Contributor

redmunds commented Jun 24, 2014

Definitely want something along these lines. Triage Complete.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Jun 27, 2014

Contributor

@dangoor @SAPlayer After a closer look, I'm concerned about performance. Currently, a fair amount of Quick View processing is done during mousemove event processing and this adds a lot parsing using CodeMirror tokenizer. I don't think this pull request should be merged until the Defer most Quick View processing until after delay pull request has been merged.

Contributor

redmunds commented Jun 27, 2014

@dangoor @SAPlayer After a closer look, I'm concerned about performance. Currently, a fair amount of Quick View processing is done during mousemove event processing and this adds a lot parsing using CodeMirror tokenizer. I don't think this pull request should be merged until the Defer most Quick View processing until after delay pull request has been merged.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Jun 27, 2014

Contributor

@redmunds That's ok. But btw, this check is only executed after we have at least one result.
And we can too refactor it a little to only call getModeAt() after we got an actual result.

Contributor

MarcelGerber commented Jun 27, 2014

@redmunds That's ok. But btw, this check is only executed after we have at least one result.
And we can too refactor it a little to only call getModeAt() after we got an actual result.

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Jul 4, 2014

Contributor

@redmunds @peterflynn FYI, I changed it to a whitelist with only CSS/SASS/SCSS/LESS in it.

Contributor

MarcelGerber commented Jul 4, 2014

@redmunds @peterflynn FYI, I changed it to a whitelist with only CSS/SASS/SCSS/LESS in it.

@redmunds redmunds self-assigned this Jul 23, 2014

break;
}
if (ignoreNamedColors === undefined) {
var mode = TokenUtils.getModeAt(editor._codeMirror, pos).name;

This comment has been minimized.

@redmunds

redmunds Aug 13, 2014

Contributor

TokenUtils.getModeAt() is not a cheap method to call, so we need to minimize the number of times it is called. Until we add support for inline css in style attributes, then it's safe to assume that entire line has same mode, so only need to check it once per line. It's simpler to just pass in mode instead of both editor and pos.

@redmunds

redmunds Aug 13, 2014

Contributor

TokenUtils.getModeAt() is not a cheap method to call, so we need to minimize the number of times it is called. Until we add support for inline css in style attributes, then it's safe to assume that entire line has same mode, so only need to check it once per line. It's simpler to just pass in mode instead of both editor and pos.

This comment has been minimized.

@redmunds

redmunds Aug 13, 2014

Contributor

Actually, can calculate ignoreNamedColors for line and just pass that in.

@redmunds

redmunds Aug 13, 2014

Contributor

Actually, can calculate ignoreNamedColors for line and just pass that in.

This comment has been minimized.

@MarcelGerber

MarcelGerber Aug 24, 2014

Contributor

@redmunds getModeAt is only called when there is an actual result and it's only called once per line.

@MarcelGerber

MarcelGerber Aug 24, 2014

Contributor

@redmunds getModeAt is only called when there is an actual result and it's only called once per line.

This comment has been minimized.

@redmunds

redmunds Sep 4, 2014

Contributor

Ok. I was hoping to make the loop & params a little cleaner, but that's more efficient, so I'm good with it.

@redmunds

redmunds Sep 4, 2014

Contributor

Ok. I was hoping to make the loop & params a little cleaner, but that's more efficient, so I'm good with it.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Aug 13, 2014

Contributor

Done with review.

Contributor

redmunds commented Aug 13, 2014

Done with review.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Sep 4, 2014

Contributor

Thanks. Merging.

Contributor

redmunds commented Sep 4, 2014

Thanks. Merging.

redmunds added a commit that referenced this pull request Sep 4, 2014

Merge pull request #8156 from MarcelGerber/quick-view-literal
Limit QuickView to literal color names in some languages

@redmunds redmunds merged commit 60ce7af into adobe:master Sep 4, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@MarcelGerber MarcelGerber deleted the MarcelGerber:quick-view-literal branch Sep 5, 2014

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