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

Replace the URL in URLCodeHints even if it is the current content of … #11284

Merged
merged 2 commits into from
Jul 23, 2016
Merged

Replace the URL in URLCodeHints even if it is the current content of … #11284

merged 2 commits into from
Jul 23, 2016

Conversation

marcelgerber
Copy link
Contributor

…the attr

Fixes #11087. Turns out we already do what @redmunds described in his comment, but only if the hinted file name is not equal to the current value.

@redmunds It looks like you added this extra condition on purpose. Do you recall on why you did that? Was there a specific case which this was supposed to fix?

cc @redmunds

@redmunds
Copy link
Contributor

It looks like you added this extra condition on purpose. Do you recall on why you did that? Was there a specific case which this was supposed to fix?

I can't remember exactly, but it was probably an attempt to prevent unnecessary work.

@marcelgerber
Copy link
Contributor Author

In this case, this is a safe change. Right now, replaceRange is called either way, so we don't prevent unnecessary work one way or another.

@marcelgerber
Copy link
Contributor Author

@redmunds @nethip This goes down the same road as #12393 and has been around for quite a long time - would love if you could take a look at it as well :)

@redmunds
Copy link
Contributor

redmunds commented May 2, 2016

This seems like the right thing to do, but I haven't test the code.

@ficristo
Copy link
Collaborator

ficristo commented May 3, 2016

Seems to fix the issue for me. Maybe add a test?

@ficristo
Copy link
Collaborator

ficristo commented May 3, 2016

Spoke too soon: now I see an error on the console:
Uncaught TypeError: Cannot read property 'localState' of undefined
for a class like this

.test {
    color: red;
    background-image: url(index.htm
}

If I try to autocomplete the url (should be index.html) I see the error on the console. Note that should be the last rule of the class.
I didn't try on master.

@marcelgerber
Copy link
Contributor Author

Strange. I can't repro your recipe.

@marcelgerber
Copy link
Contributor Author

Also, I've just added a test case.

@ficristo
Copy link
Collaborator

ficristo commented May 3, 2016

Also, I've just added a test case.

Awesome!

Strange. I can't repro your recipe.

Sorry, the test is an HTML file. The following should suffix to reproduce.

<html>
<head>
</head>
<body>
    <style type="text/css">
        .test {
            color: red;
            background-image: url(index.htm
        }
    </style>
</body>
</html>

Sometime I can see the following exception too. It's about localState so to problem should be the same

Exception in 'editorChange' listener on Editor {document: Document, _handleDocumentChange: function, _handleDocumentDeleted: function, _handleDocumentLanguageChanged: function, _doWorkingSetSync: function…} TypeError: Cannot read property 'localState' of undefined TypeError: Cannot read property 'localState' of undefined
    at _getContextState (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/language/CSSUtils.js:95:51)
    at _isInPropValue (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/language/CSSUtils.js:138:21)
    at _getSucceedingPropValues (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/language/CSSUtils.js:355:18)
    at _getRuleInfoStartingFromPropValue (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/language/CSSUtils.js:505:40)
    at Object.getInfoAtPos (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/language/CSSUtils.js:623:20)
    at UrlCodeHints.hasCssHints (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/extensions/default/UrlCodeHints/main.js:293:30)
    at UrlCodeHints.hasHints (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/extensions/default/UrlCodeHints/main.js:267:25)
    at file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/CodeHintManager.js:486:31
    at Array.some (native)
    at _beginSession (file:///C:/Program%20Files%20(x86)/Brackets/dev/src/editor/CodeHintManager.js:485:26)

@marcelgerber marcelgerber mentioned this pull request May 3, 2016
Closed
@marcelgerber
Copy link
Contributor Author

@ficristo I see, thanks for catching this!
The bug is unrelated to this PR -> reported as #12399.

@ficristo
Copy link
Collaborator

Running Extensions \ Url Code Hinting tests I see some failing tests.
Can you take a look?

@marcelgerber
Copy link
Contributor Author

@ficristo I've seen a failure in one run (should hint site root ('/') or something along these lines), but when I then ran the same tests again, they all passed. Looks like an intermittent failure.

@ficristo
Copy link
Collaborator

I see other test failures but they seem unrelated.
I found (should hint site root ('/') intermittent too.
That said LGTM.

@marcelgerber marcelgerber merged commit 361e81d into master Jul 23, 2016
@marcelgerber marcelgerber deleted the marcel/url-hints-replace branch July 23, 2016 09:27
@marcelgerber
Copy link
Contributor Author

Thanks for reviewing, @ficristo :)

@marcelgerber marcelgerber mentioned this pull request Jul 23, 2016
Closed
@ficristo ficristo added this to the Release 1.8 milestone Aug 2, 2016
This was referenced Aug 30, 2021
Open
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 1.8
Development

Successfully merging this pull request may close these issues.

CodeHinter gets confused completing a pre-populated attribute
3 participants
@marcelgerber @redmunds @ficristo