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

Fix for Issue 3339 #3340

Merged
merged 7 commits into from
Apr 9, 2013
Merged

Fix for Issue 3339 #3340

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
990fe39
Logic fix
WebsiteDeveloper Apr 4, 2013
edab6b8
A few other cleanup fixes
WebsiteDeveloper Apr 4, 2013
5ae84c4
Fixed replacement when currently in Entity
WebsiteDeveloper Apr 5, 2013
087fa43
A bit cleanup
WebsiteDeveloper Apr 8, 2013
b200259
Fix after review
WebsiteDeveloper Apr 8, 2013
cd9004a
Fixes after first review
WebsiteDeveloper Apr 8, 2013
e63ec5f
Fixes after second review
WebsiteDeveloper Apr 9, 2013
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
40 changes: 25 additions & 15 deletions src/extensions/default/HtmlEntityCodeHints/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,7 @@ define(function (require, exports, module) {

var query = this._getQuery();

if (implicitChar === null) {
return query !== null;
}

return implicitChar === "&" || query !== null;
return query !== null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This method now returns query !== null in all cases, so remove this block:

    if (implicitChar === null) {
        return query !== null;
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but now that you made that change, I can see that you don't need query var, and this can simply be:

        return this._getQuery() !== null;

Copy link
Contributor

Choose a reason for hiding this comment

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

Original comment seemed to be buried in a hidden "Outdated Diff", so adding again. Sorry for the dupe. Don't need query var, so this can be simplified more:

        return this._getQuery() !== null;

};

/**
Expand All @@ -120,7 +116,7 @@ define(function (require, exports, module) {
var query,
result;

if (this.primaryTriggerKeys.indexOf(implicitChar) !== -1 || implicitChar === null) {
if (implicitChar === null || this.primaryTriggerKeys.indexOf(implicitChar) !== -1) {
this.currentQuery = query = this._getQuery();
result = $.map(specialChars, function (value, index) {
if (value.indexOf(query) === 0) {
Expand All @@ -129,7 +125,10 @@ define(function (require, exports, module) {
}
}).sort(this._internalSort);

query = _encodeValue(query);
if (query !== null) {
query = _encodeValue(query);
}

return {
hints: result,
match: query,
Expand Down Expand Up @@ -173,26 +172,29 @@ define(function (require, exports, module) {
var query,
lineContent,
startChar,
endChar;
endChar,
cursor = this.editor.getCursorPos();

query = "&";
if (HTMLUtils.getTagInfo(this.editor, cursor).tagName !== "") {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable name lineContent should be more accurate so code is easier to understand, so rename to something like lineContentBeforeCursor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

lineContent = this.editor.document.getRange({
line: this.editor.getCursorPos().line,
line: cursor.line,
ch: 0
}, this.editor.getCursorPos());
}, cursor);

startChar = lineContent.lastIndexOf("&");
endChar = lineContent.lastIndexOf(";");

if (endChar < startChar) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic here needs some cleanup:

        if (endChar < startChar) {
            query = this.editor.document.getRange({
                line: cursor.line,
                ch: startChar
            }, cursor);
        }

        if (endChar < startChar) {
            return query;
        }

        return null;
  1. No need to check for (endChar < startChar) twice.
  2. The (endChar < startChar) condition relies on the fact that "not found" is represented by -1 so it seems a bit cryptic. Please reverse the logic and add a comment.
  3. There was a check that startChar was found that got dropped. Maybe that was (cryptically) handled by the endChar === startChar case, but I think you need it here:
        // If no startChar, or endChar is found after startChar, then not in an entity
        if (startChar === -1 || endChar > startChar) {
            return null;
        }

        query = this.editor.document.getRange({
            line: cursor.line,
            ch: startChar
        }, cursor);

        return query;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right thats much more understandable.

query = this.editor.document.getRange({
line: this.editor.getCursorPos().line,
line: cursor.line,
ch: startChar
}, this.editor.getCursorPos());
}, cursor);
}

if (startChar !== -1 && HTMLUtils.getTagInfo(this.editor, this.editor.getCursorPos()).tagName === "") {
if (endChar < startChar) {
return query;
}

Expand All @@ -212,11 +214,19 @@ define(function (require, exports, module) {
SpecialCharHints.prototype.insertHint = function (completion) {
var start = {line: -1, ch: -1},
end = {line: -1, ch: -1},
cursor = this.editor.getCursorPos();
cursor = this.editor.getCursorPos(),
match;

end.line = start.line = cursor.line;
start.ch = cursor.ch - this.currentQuery.length;
match = this.editor.document.getLine(cursor.line).slice(cursor.ch);
end.ch = start.ch + this.currentQuery.length;

if (match.indexOf(";") !== -1 && /^(#*[0-9]+)|([a-zA-Z]+)$/.test(match.slice(0, match.indexOf(";")))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

match.indexOf(";") is called twice, so it should be put in a var.

console.log("In Entity");
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this console.log() statement.

end.ch = this.editor.document.getLine(cursor.line).indexOf(";", start.ch) + 1;
}

completion = completion.slice(0, completion.indexOf(" "));
completion = _decodeValue(completion);
if (start.ch !== end.ch) {
Expand Down