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

HTML attributes code hinting filter #2263

Merged
RaymondLim merged 7 commits into from
Dec 6, 2012
Merged

HTML attributes code hinting filter #2263

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d587f36
Basic attribute discovery and hinting filter
jbalsas Nov 30, 2012
7335f94
Fixes and cleanup
jbalsas Dec 1, 2012
0aae4e6
Basic tests
jbalsas Dec 1, 2012
3306701
Fixed empty array for attributes options
jbalsas Dec 2, 2012
07be375
Inverted sage of quotes
jbalsas Dec 5, 2012
844c5fe
Update test for hints after close tag
jbalsas Dec 6, 2012
c48b3ad
Fixed last missing single quotes
jbalsas Dec 6, 2012
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
14 changes: 8 additions & 6 deletions src/extensions/default/HTMLCodeHints/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ define(function (require, exports, module) {
query.attrName = tagInfo.attr.name;
}

// TODO: get existing attributes for the current tag and add them to query.usedAttr
query.usedAttr = HTMLUtils.getTagAttributes(editor, cursor);
}

return query;
Expand Down Expand Up @@ -425,6 +425,7 @@ define(function (require, exports, module) {
attrName = query.attrName,
filter = query.queryStr,
unfiltered = [],
filtered = [],
sortFunc = null;

this.closeOnSelect = true;
Expand Down Expand Up @@ -452,13 +453,14 @@ define(function (require, exports, module) {
}
} else if (tags && tags[tagName] && tags[tagName].attributes) {
unfiltered = tags[tagName].attributes.concat(this.globalAttributes);

// TODO: exclude existing attributes from unfiltered array
filtered = $.grep(unfiltered, function (attr, i) {
return $.inArray(attr, query.usedAttr) < 0;
});
}

if (unfiltered.length) {
if (filtered.length) {
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Dec 2, 2012

Choose a reason for hiding this comment

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

Switching to filtered is ok for your if block, but it's not ok for other if blocks. Make sure you also adjust others.

console.assert(!result.length);
result = $.map(unfiltered, function (item) {
result = $.map(filtered, function (item) {
if (item.indexOf(filter) === 0) {
return item;
}
Expand Down
40 changes: 40 additions & 0 deletions src/language/HTMLUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,45 @@ define(function (require, exports, module) {
return null;
}

/**
* Compiles a list of used attributes for a given tag
* @param {CodeMirror} editor An instance of a CodeMirror editor
* @param {ch:{string}, line:{number}} pos A CodeMirror position
* @return {Array.<string>} A list of the used attributes inside the current tag
*/
function getTagAttributes(editor, pos) {
var attrs = [],
backwardCtx = TokenUtils.getInitialContext(editor._codeMirror, pos),
forwardCtx = $.extend({}, backwardCtx);

if (editor.getModeForSelection() === "html") {
if (backwardCtx.token) {
while (TokenUtils.movePrevToken(backwardCtx) && backwardCtx.token.className !== "tag") {
if (backwardCtx.token.className === "attribute") {
attrs.push(backwardCtx.token.string);
}
}

while (TokenUtils.moveNextToken(forwardCtx) && forwardCtx.token.className !== "tag") {
if (forwardCtx.token.className === "attribute") {
attrs.push(forwardCtx.token.string);
} else if (forwardCtx.token.className === "error") {
// If we type the first letter of the next attribute, it comes as an error
// token. We need to double check for possible invalidated attributes.
if (forwardCtx.token.string.trim() !== "" &&
forwardCtx.token.string.indexOf("\"") === -1 &&
forwardCtx.token.string.indexOf("'") === -1 &&
forwardCtx.token.string.indexOf("=") === -1) {
attrs.push(forwardCtx.token.string);
}
}
}
}
}

return attrs;
}

/**
* Creates a tagInfo object and assures all the values are entered or are empty strings
* @param {string=} tokenType what is getting edited and should be hinted
Expand Down Expand Up @@ -450,6 +489,7 @@ define(function (require, exports, module) {
exports.ATTR_VALUE = ATTR_VALUE;

exports.getTagInfo = getTagInfo;
exports.getTagAttributes = getTagAttributes;
//The createTagInfo is really only for the unit tests so they can make the same structure to
//compare results with
exports.createTagInfo = createTagInfo;
Expand Down
86 changes: 86 additions & 0 deletions test/spec/CodeHintUtils-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,92 @@ define(function (require, exports, module) {
var tag = HTMLUtils.getTagInfo(myEditor, pos);
expect(tag).toEqual(HTMLUtils.createTagInfo());
});

it("should not find attributes in an empty editor", function () {
var pos = {"ch": 0, "line": 0};
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs).toEqual([]);
});

it("should not find attributes before the tag is opened", function () {
var pos = {"ch": 0, "line": 0};
setContentAndUpdatePos(pos,
['<html>', '<body>', '<div class="clearfix">'],
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Dec 4, 2012

Choose a reason for hiding this comment

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

Our coding guideline is to use double quotes for all strings and the one nested inside then can use a single quote. So can you switch to double-quotes in all your test cases?

Copy link
Copy Markdown
Contributor Author

@jbalsas jbalsas Dec 4, 2012

Choose a reason for hiding this comment

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

Sure, no problem, will fix that.

I actually copy/pasted from one of the tests above and all the existing ones have the quotes inverted. Do you want me to change those as a bonus, or should we let them be for now?

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Dec 4, 2012

Choose a reason for hiding this comment

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

Yes, please do change them if you don't mind.

'', '<p id="pid" class="pclass" lang="plang" align="palign" title="">test</p>',
[ '</div>', '</body>', '</html>']);
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs).toEqual([]);
});

it("should not find attributes if there isn't a valid tag", function () {
var pos = {"ch": 0, "line": 0};
setContentAndUpdatePos(pos,
['<html>', '<body>', '<div class="clearfix">'],
'<', ' id="pid" class="pclass" lang="plang" align="palign" title="">test</p>',
[ '</div>', '</body>', '</html>']);
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs).toEqual([]);
});

it("should not find attributes after the tag is closed", function () {
var pos = {"ch": 0, "line": 0};
setContentAndUpdatePos(pos,
['<html>', '<body>', '<div class="clearfix">'],
'<p id="pid" class="pclass" lang="plang" align="palign" title="">test</p>', '',
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Dec 4, 2012

Choose a reason for hiding this comment

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

How about moving test</p> to the next argument? We don't show any attribute even inside the end tag anyway. So I think it would be better if we're testing around the begin tag.

Copy link
Copy Markdown
Contributor Author

@jbalsas jbalsas Dec 4, 2012

Choose a reason for hiding this comment

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

In that case that would become the same as the one where it "should find all the tag attributes before closing the tag", right? I'll just remove this one then.

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Dec 4, 2012

Choose a reason for hiding this comment

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

Hmm... I thought the result will be empty as the cursor is after the begin tag. ie. after >

Copy link
Copy Markdown
Contributor Author

@jbalsas jbalsas Dec 4, 2012

Choose a reason for hiding this comment

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

The test, as is right now, expects indeed an empty list.

I thought you wanted me to pass test</p> as the hintLineAfter argument, in which case the IP would be positioned between title="" and >test</p>, and that is covered already in a different test.

What would you want to do with this test?

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Dec 6, 2012

Choose a reason for hiding this comment

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

No, the test case you're referring to has the cursor before (or to the left of) > of the begin tag. What I meant is the cursor after (or to the right of) > of the begin tag. So the cursor is in the content area right before the word test and not in the begin or end tags.

Copy link
Copy Markdown
Contributor Author

@jbalsas jbalsas Dec 6, 2012

Choose a reason for hiding this comment

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

Oh, I see it now. I had totally missed the >...

Nice catch btw. I've changed it and it was returning the list of attributes instead of empty for that case, so I'll commit a fix with the changes.

[ '</div>', '</body>', '</html>']);
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs).toEqual([]);
});

it("should find all the tag attributes immediately after the tag", function () {
var pos = {"ch": 0, "line": 0};
setContentAndUpdatePos(pos,
['<html>', '<body>', '<div class="clearfix">'],
'<p ', 'id="pid" class="pclass" lang="plang" align="palign" title="ptitle">test</p>',
[ '</div>', '</body>', '</html>']);
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs.sort()).toEqual(["id", "class", "lang", "align", "title"].sort());
});

it("should find all the tag attributes before closing the tag", function () {
var pos = {"ch": 0, "line": 0};
setContentAndUpdatePos(pos,
['<html>', '<body>', '<div class="clearfix">'],
'<p id="pid" class="pclass" lang="plang" align="palign" title="ptitle" ', '>test</p>',
[ '</div>', '</body>', '</html>']);
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs.sort()).toEqual(["id", "class", "lang", "align", "title"].sort());
});

it("should find all the tag attributes backward and forward", function () {
var pos = {"ch": 0, "line": 0};
setContentAndUpdatePos(pos,
['<html>', '<body>', '<div class="clearfix">'],
'<p id="pid" class="pclass" lang="plang" ', 'align="palign" title="ptitle">test</p>',
[ '</div>', '</body>', '</html>']);
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs.sort()).toEqual(["id", "class", "lang", "align", "title"].sort());
});

it("should find valid attributes marked as errors by the tokenizer", function () {
var pos = {"ch": 0, "line": 0};
setContentAndUpdatePos(pos,
['<html>', '<body>', '<div class="clearfix">'],
'<p id="pid" c', ' class="pclass" lang="plang" align="palign" title="ptitle">test</p>',
[ '</div>', '</body>', '</html>']);
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs.sort()).toEqual(["id", "class", "lang", "align", "title"].sort());
});

it("should not find attributes in nested tags", function () {
var pos = {"ch": 0, "line": 0};
setContentAndUpdatePos(pos,
['<html>', '<body>', '<div class="clearfix">'],
'<p ', 'id="pid" class="pclass" lang="plang" align="palign" title="ptitle"><span style="sstyle"></span></p>',
[ '</div>', '</body>', '</html>']);
var attrs = HTMLUtils.getTagAttributes(myEditor, pos);
expect(attrs.sort()).toEqual(["id", "class", "lang", "align", "title"].sort());
});
});
});
});
⚡