Fix for Issue 3339 #3340

Merged
merged 7 commits into from Apr 9, 2013

Conversation

Projects
None yet
3 participants
@WebsiteDeveloper
Contributor

WebsiteDeveloper commented Apr 4, 2013

Fix #3339

@WebsiteDeveloper

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 4, 2013

Contributor

Also added fixes for two other cases @peterflynn mentioned.

Contributor

WebsiteDeveloper commented Apr 4, 2013

Also added fixes for two other cases @peterflynn mentioned.

@WebsiteDeveloper

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 5, 2013

Contributor

@peterflynn also added code to replace the entire token, when invoking the in hints inside a token.

Contributor

WebsiteDeveloper commented Apr 5, 2013

@peterflynn also added code to replace the entire token, when invoking the in hints inside a token.

@WebsiteDeveloper

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 5, 2013

Contributor

unit tests will be added in a separate pull after this got merged.

Contributor

WebsiteDeveloper commented Apr 5, 2013

unit tests will be added in a separate pull after this got merged.

@ghost ghost assigned redmunds Apr 8, 2013

@peterflynn

This comment has been minimized.

Show comment Hide comment
@peterflynn

peterflynn Apr 8, 2013

Member

We should really add unit tests here too -- both for some entity hinting cases and for the tag hinting cases that are currently broken (and which the existing tag hinting tests haven't caught).

Member

peterflynn commented Apr 8, 2013

We should really add unit tests here too -- both for some entity hinting cases and for the tag hinting cases that are currently broken (and which the existing tag hinting tests haven't caught).

@peterflynn

This comment has been minimized.

Show comment Hide comment
@peterflynn

peterflynn Apr 8, 2013

Member

Oops sorry, just saw the above note about adding unit tests later. Ideally we like to do it all in one pull request, but this seems ok since we definitely need to get the fix part landed ASAP before sprint end.

Member

peterflynn commented Apr 8, 2013

Oops sorry, just saw the above note about adding unit tests later. Ideally we like to do it all in one pull request, but this seems ok since we definitely need to get the fix part landed ASAP before sprint end.

@WebsiteDeveloper

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

thats what i thougth ;)

Contributor

WebsiteDeveloper commented Apr 8, 2013

thats what i thougth ;)

@@ -96,7 +96,7 @@ define(function (require, exports, module) {
return query !== null;
}
- return implicitChar === "&" || query !== null;
+ return query !== null;

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

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

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

redmunds Apr 8, 2013

Contributor

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

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

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

Fixed.

@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

Fixed.

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

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; 
@redmunds

redmunds Apr 8, 2013

Contributor

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; 
- query = "&";
+ if (HTMLUtils.getTagInfo(this.editor, cursor).tagName !== "") {
+ return null;

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

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

@redmunds

redmunds Apr 8, 2013

Contributor

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

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

Fixed.

@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

Fixed.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

Although you haven't changed it in this pull request, I just noticed this function:

    function _encodeValue(value) {
        return (value.indexOf("#") === -1) ? value.replace("&", "&") : value.replace("&", "&").replace("#", "#");
    }

There's no need to search for "#" because replace will do nothing if it's not found:

    function _encodeValue(value) {
        return value.replace("&", "&").replace("#", "#");
    }

Question: Why is the semi-colon (;) not also encoded since it is decoded in the associated _decodeValue() function? In general, encode/decode functions should do the exact opposite of each other.

Contributor

redmunds commented Apr 8, 2013

Although you haven't changed it in this pull request, I just noticed this function:

    function _encodeValue(value) {
        return (value.indexOf("#") === -1) ? value.replace("&", "&") : value.replace("&", "&").replace("#", "#");
    }

There's no need to search for "#" because replace will do nothing if it's not found:

    function _encodeValue(value) {
        return value.replace("&", "&").replace("#", "#");
    }

Question: Why is the semi-colon (;) not also encoded since it is decoded in the associated _decodeValue() function? In general, encode/decode functions should do the exact opposite of each other.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

Please don't push any changes until I say "review is complete". I'm looking at the code in both the github pull request diff and in Brackets, so it's making it difficult for me to keep them in sync. Thanks.

Contributor

redmunds commented Apr 8, 2013

Please don't push any changes until I say "review is complete". I'm looking at the code in both the github pull request diff and in Brackets, so it's making it difficult for me to keep them in sync. Thanks.

@WebsiteDeveloper

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

sorry i'll certainly do so.
As to your question, encoding the semi-colon also would break things because of the order of replacements.
With something like that:

function _encodeValue(value) {
        return value.replace("&", "&").replace("#", "#").replace(";", "&#59;");
}

we would break the whole functionality because the replace would replace the semi-colons from the previous entities.
If we would reverse the order like:

function _encodeValue(value) {
        return value.replace("&", "&").replace(";", "&#59;").replace("#", "#");
}

the hash of the semi-colon entity would be replaced.

Contributor

WebsiteDeveloper commented Apr 8, 2013

sorry i'll certainly do so.
As to your question, encoding the semi-colon also would break things because of the order of replacements.
With something like that:

function _encodeValue(value) {
        return value.replace("&", "&").replace("#", "#").replace(";", "&#59;");
}

we would break the whole functionality because the replace would replace the semi-colons from the previous entities.
If we would reverse the order like:

function _encodeValue(value) {
        return value.replace("&", "&").replace(";", "&#59;").replace("#", "#");
}

the hash of the semi-colon entity would be replaced.

- startChar = lineContent.lastIndexOf("&");
- endChar = lineContent.lastIndexOf(";");
+ startChar = lineContentBeforeCursor.lastIndexOf("&");
+ endChar = lineContentBeforeCursor.lastIndexOf(";");
if (endChar < startChar) {

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

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;
@redmunds

redmunds Apr 8, 2013

Contributor

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;

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

you're right thats much more understandable.

@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

you're right thats much more understandable.

end.ch = start.ch + this.currentQuery.length;
+
+ if (match.indexOf(";") !== -1 && /^(#*[0-9]+)|([a-zA-Z]+)$/.test(match.slice(0, match.indexOf(";")))) {
+ console.log("In Entity");

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

Remove this console.log() statement.

@redmunds

redmunds Apr 8, 2013

Contributor

Remove this console.log() statement.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

Regarding "encoding the semi-colon": Unfortunately, it's not easy to do, but, if it's required, you just have to be smarter about it. In this code, I don't think it's required because _encodeValue() is only used to encode query strings. If so, then give it a slightly different name such as _encodeQueryValue() and add a comment explaining why it's not an exact match for _decodeValue().

Contributor

redmunds commented Apr 8, 2013

Regarding "encoding the semi-colon": Unfortunately, it's not easy to do, but, if it's required, you just have to be smarter about it. In this code, I don't think it's required because _encodeValue() is only used to encode query strings. If so, then give it a slightly different name such as _encodeQueryValue() and add a comment explaining why it's not an exact match for _decodeValue().

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

Done with initial review.

Contributor

redmunds commented Apr 8, 2013

Done with initial review.

@WebsiteDeveloper

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

actually _encodeValue() is used elsewhere to encode the ui string, but encoding the semi-colon is actually not needed as far as i can see. I have no idea why i did need it once ^.
So i'll just get rid of it.

Contributor

WebsiteDeveloper commented Apr 8, 2013

actually _encodeValue() is used elsewhere to encode the ui string, but encoding the semi-colon is actually not needed as far as i can see. I have no idea why i did need it once ^.
So i'll just get rid of it.

@WebsiteDeveloper

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 8, 2013

Contributor

@redmunds fixes pushed.

Contributor

WebsiteDeveloper commented Apr 8, 2013

@redmunds fixes pushed.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

In the sorting function, I noticed this after I merged the original code: this line of code:

            return (num1 === num2) ? 0 : (num1 > num2) ? 1 : -1;

Can be simplified to:

            return (num1 - num2);

This is because sort functions aren't limited to returning only -1, 0, or 1. They just need to return 0 if equal, or a positive or negative number to indicate greater than or less than.

Contributor

redmunds commented Apr 8, 2013

In the sorting function, I noticed this after I merged the original code: this line of code:

            return (num1 === num2) ? 0 : (num1 > num2) ? 1 : -1;

Can be simplified to:

            return (num1 - num2);

This is because sort functions aren't limited to returning only -1, 0, or 1. They just need to return 0 if equal, or a positive or negative number to indicate greater than or less than.

end.ch = start.ch + this.currentQuery.length;
+
+ if (match.indexOf(";") !== -1 && /^(#*[0-9]+)|([a-zA-Z]+)$/.test(match.slice(0, match.indexOf(";")))) {

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

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

@redmunds

redmunds Apr 8, 2013

Contributor

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

- }
-
- return implicitChar === "&" || query !== null;
+ return query !== null;

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

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;
@redmunds

redmunds Apr 8, 2013

Contributor

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;
@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

Done with second review.

Contributor

redmunds commented Apr 8, 2013

Done with second review.

@WebsiteDeveloper

This comment has been minimized.

Show comment Hide comment
@WebsiteDeveloper

WebsiteDeveloper Apr 9, 2013

Contributor

@redmunds changes pushed.

Contributor

WebsiteDeveloper commented Apr 9, 2013

@redmunds changes pushed.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Apr 9, 2013

Contributor

Looks good. Merging.

Contributor

redmunds commented Apr 9, 2013

Looks good. Merging.

redmunds added a commit that referenced this pull request Apr 9, 2013

@redmunds redmunds merged commit d26d885 into adobe:master Apr 9, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment