Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Limit QuickView to literal color names in some languages #8156

Merged
merged 8 commits into from
Sep 4, 2014
Merged

Limit QuickView to literal color names in some languages #8156

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8efafdf
Limit QuickView to literal color names in some languages
Jun 17, 2014
ad80150
Merge remote-tracking branch 'upstream/master' into quick-view-literal
Jul 4, 2014
d4a5cb3
Use whitelist instead of blacklist
Jul 4, 2014
72584d6
Only call getModeAt if needed
Jul 5, 2014
93e1eaa
Merge remote-tracking branch 'upstream/master' into quick-view-literal
Aug 6, 2014
64a418c
Renamed var & method
Aug 6, 2014
bf24ee7
Merge remote-tracking branch 'upstream/master' into quick-view-literal
Aug 24, 2014
8c7fcda
Add LESS
Aug 24, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions src/extensions/default/QuickView/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ define(function (require, exports, module) {
Menus = brackets.getModule("command/Menus"),
PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
Strings = brackets.getModule("strings"),
ViewUtils = brackets.getModule("utils/ViewUtils");
ViewUtils = brackets.getModule("utils/ViewUtils"),
TokenUtils = brackets.getModule("utils/TokenUtils");

var previewContainerHTML = require("text!QuickViewTemplate.html");

Expand All @@ -53,6 +54,8 @@ define(function (require, exports, module) {
POINTER_HEIGHT = 15, // Pointer height, used to shift popover above pointer (plus a little bit of space)
POPOVER_HORZ_MARGIN = 5; // Horizontal margin

var styleLanguages = ["css", "text/x-scss", "sass"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add LESS. FYI, In general Brackets plans to support scss, but not sass, but I guess it's OK if sass is accepted here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


prefs = PreferencesManager.getExtensionPrefs("quickview");
prefs.definePreference("enabled", "boolean", true);

Expand Down Expand Up @@ -232,8 +235,9 @@ define(function (require, exports, module) {
};
}

function execColorMatch(line) {
var colorMatch;
function execColorMatch(editor, line, pos) {
var colorMatch,
literalCheck;

function hyphenOnMatchBoundary(match, line) {
var beforeIndex, afterIndex;
Expand All @@ -251,11 +255,24 @@ define(function (require, exports, module) {

return false;
}
function checkForLiteral(match) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by comment: "checkForLiteral()" is a name that leaves things vague about what's returned. Maybe something like "isNamedColor()"?

Similarly for "literalCheck" -- how about "disallowNamedColors" or "ignoreNamedColors"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (match && match[0] && /^[a-z]+$/i.test(match[0])) { // only for color names, not for hex-/rgb-values
return true;
}
}

// Hyphens do not count as a regex word boundary (\b), so check for those here
do {
colorMatch = colorRegEx.exec(line);
} while (colorMatch && hyphenOnMatchBoundary(colorMatch, line));
if (!colorMatch) {
break;
}
if (literalCheck === undefined) {
var mode = TokenUtils.getModeAt(editor._codeMirror, pos).name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

literalCheck = styleLanguages.indexOf(mode) === -1;
}
} while (hyphenOnMatchBoundary(colorMatch, line) ||
(literalCheck && checkForLiteral(colorMatch)));

return colorMatch;
}
Expand Down Expand Up @@ -347,7 +364,7 @@ define(function (require, exports, module) {
}

var gradientMatch = execGradientMatch(line),
match = gradientMatch.match || execColorMatch(line),
match = gradientMatch.match || execColorMatch(editor, line, pos),
cm = editor._codeMirror;

while (match) {
Expand Down Expand Up @@ -393,7 +410,7 @@ define(function (require, exports, module) {
if (gradientMatch.match) {
gradientMatch = execGradientMatch(line);
}
match = gradientMatch.match || execColorMatch(line);
match = gradientMatch.match || execColorMatch(editor, line, pos);
}

return null;
Expand Down
10 changes: 10 additions & 0 deletions src/extensions/default/QuickView/unittest-files/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/* Sample JS for testing the QuickView extension. */

function green() { // generate green colors
var color = "green",
tan = Math.tan;
return tan(array["red"], array[red]);
}
darkgray
// #123456
// :rgb(65, 43, 21)
45 changes: 42 additions & 3 deletions src/extensions/default/QuickView/unittests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ define(function (require, exports, module) {
Commands,
EditorManager,
QuickView,
editor;
editor,
testFile = "test.css",
oldFile;

beforeEach(function () {
// Create a new window that will be shared by ALL tests in this spec.
Expand All @@ -62,13 +64,16 @@ define(function (require, exports, module) {
runs(function () {
SpecRunnerUtils.loadProjectInTestWindow(testFolder);
});
}

if (testFile !== oldFile) {
runs(function () {
waitsForDone(SpecRunnerUtils.openProjectFiles(["test.css"]), "open test file");
waitsForDone(SpecRunnerUtils.openProjectFiles([testFile]), "open test file: " + testFile);
});

runs(function () {
editor = EditorManager.getCurrentFullEditor();
editor = EditorManager.getCurrentFullEditor();
oldFile = testFile;
});
}
});
Expand Down Expand Up @@ -191,9 +196,43 @@ define(function (require, exports, module) {
expectNoPreviewAtPos(75, 18); // cursor on white in hyphenated word @bc-white
});
});

describe("JavaScript file", function () {
runs(function () {
testFile = "test.js";
});

it("should NOT show preview of color-named functions and object/array keys", function () {
runs(function () {
expectNoPreviewAtPos(2, 12); // cursor on green()
expectNoPreviewAtPos(4, 22); // cursor on Math.tan
expectNoPreviewAtPos(5, 14); // cursor on tan()
expectNoPreviewAtPos(5, 38); // cursor on array[red]
});
});
it("should not show preview of literal color names", function () {
runs(function () {
expectNoPreviewAtPos(2, 36); // green
expectNoPreviewAtPos(3, 21); // green
expectNoPreviewAtPos(4, 11); // tan
expectNoPreviewAtPos(5, 25); // red
expectNoPreviewAtPos(7, 1); // darkgray
});
});
it("should show preview of non-literal color codes", function () {
runs(function () {
checkColorAtPos("#123456", 8, 7);
checkColorAtPos("rgb(65, 43, 21)", 9, 8);
});
});
});
});

describe("Quick view gradients", function () {
runs(function () {
testFile = "test.css";
});

it("Should show linear gradient preview for those with vendor prefix", function () {
runs(function () {
var expectedGradient1 = "-webkit-linear-gradient(top, #d2dfed 0%, #c8d7eb 26%, #bed0ea 51%, #a6c0e3 51%, #afc7e8 62%, #bad0ef 75%, #99b5db 88%, #799bc8 100%)",
Expand Down