SVG Code Hints (#10084) #10294

sprintr · 2015-01-01T11:58:35Z

This is a follow up of #10084, adds Code Hints for SVG.

marcelgerber · 2015-01-01T13:35:45Z

src/extensions/default/SVGCodeHints/SVGAttributes.json

+    "clip-rule": {
+        "attribOptions": ["evenodd", "inherit", "nonzero"]
+    },
+    "color": {


We should try to align this with ColorUtils.js.
Unfortunately, we only have a comprehensive regexp right now, including rgb, hex, ...

You should at least include rebeccapurple.

rebeccapurple was not available in SVG 1.1 spec.

We currently do not provide the list of defined colors for CSS Code Hints. For the sake of consistency, seems like we should provide them in both places or neither. Thoughts?

Even though rebeccapurple is not in the spec, I just tested using it in Chrome (in a SVG, of course) and it worked. I guess some will use it, and it won't hurt if we have it in there.

@marcelgerber I have removed all color properties for now to be consistent with CSS hinting. Will add it later including rebeccapurple once we get them in CSS too.

redmunds · 2015-01-01T19:45:01Z

src/extensions/default/SVGCodeHints/SVGTags.json

@@ -0,0 +1,242 @@
+{


In the spec, attributes are organized into groups such as "conditional processing attributes", "core attributes", "graphical event attributes", "presentation attributes", "xlink attributes", etc.

I think it would reduce a lot of redunancy which would make future maintenance easier if this organization could be captured in the json data. For example, define each of the "attribute groups" as a list of attributes, and then any tag that uses a particular attribute group could simply specify a flag (e.g. "coreAttrs": true).

What do you think?

Yep, I'll get that organized.

This format looks great!

Since this data has both tags and attributes, I think the term "groups" is a little vague. What about using "attributeGroups" ?

I like attributeGroups.

redmunds · 2015-01-01T20:02:16Z

@sprintr This is an awesome contribution! Thanks for all of the unit tests.

I made a first pass through the extension. Things generally look and work good. I'll need to make a more detailed pass through the code, but I wanted to give my initial feedback.

marcelgerber · 2015-01-01T21:56:43Z

src/extensions/default/SVGCodeHints/SVGUtils.js

+    function _getTagAttributes(editor, constPos) {
+        var pos, ctx, ctxPrev, ctxNext, ctxTemp, tagName, exclusionList = [], shouldReplace;
+
+        pos = $.extend({}, constPos);


Better use _.clone(constPos) here. It's more obvious (but you'll need to getModule thirdparty/lodash)

(same below)

$.extend({}, constPos) is OK here. It's used in many places in the code.

Note that since constPos is multiple levels deep, I think you need to do a deep copy:

pos = $.extend(true, {}, constPos);

Well, constPos is not multiple levels deep, but ctx (a few lines below) is, right?

Right. I thought it was a range.

Well, pos works both ways.

Yeah, it works both ways, but only use deep copying if needed.

sprintr · 2015-01-02T10:50:51Z

Alright, will make the proposed changes today. I don't know about copyright of it.

sprintr · 2015-01-03T07:20:59Z

@redmunds @marcelgerber I have made the changes, Do I have to write unit tests for SVGUtils too after I rename it to XMLUtils and keep it in languages directory?

sprintr · 2015-01-03T07:25:03Z

@redmunds @marcelgerber The new format of SVGTags.json

sprintr · 2015-01-03T13:20:51Z

@redmunds I will work on caching attributes tomorrow.

marcelgerber · 2015-01-03T13:56:36Z

src/extensions/default/SVGCodeHints/SVGUtils.js

+        TokenValue  = 3;
+
+    // Regex to find whether or not an attribute value is quoted in single or double quotation marks.
+    var regexQuotedValue = /^['"][\W\w]*?['"]?$/;


[\W\w] is everything (like .), right?
In that case, you should rather use .

Was that done because JSLint complained about using .*?? If so, you can add the regexp: true jslint var.

Yes, that was the reason.

redmunds · 2015-01-03T18:41:26Z

@sprintr

The new format of SVGTags.json

Very nice! So much easier to read and update.

redmunds · 2015-01-03T19:22:48Z

@sprintr

Do I have to write unit tests for SVGUtils too after I rename it to XMLUtils and keep it in languages directory?

That would be great! The token values set in CodeMirror have changed in the past and broken things, so this will make it easier to detect and fix any cases in the future.

There are really only 2 API calls that you're providing (the others are static values), so it only needs to be a few tests that the API call returns expected values.

marcelgerber · 2015-01-03T23:31:44Z

src/extensions/default/SVGCodeHints/SVGUtils.js

+     * Return the tagName and a list of attributes used by the tag.
+     *
+     * @param {Editor} editor An instance of active editor
+     * @param {line: int, ch: int} constPos The position of cursor in the active editor


Should be something like {!{line: number, ch: number}}

sprintr · 2015-01-07T11:31:28Z

I am facing a problem with brackets.getModule, after transferring XMLUtils.js to language/, I am not able to require it via brackets.getModule.

When I use brackets.getModule("language/XMLUtils"), I face the following error.

[Extension] failed to load G:/brackets/brackets-svg/src/extensions/default/SVGCodeHints - Error: Module name "language/XMLUtils" has not been loaded yet for context: _

and using require("language/XMLUtils"), the result is.

GET file:///G:/brackets/brackets-svg/src/extensions/default/SVGCodeHints/language/XMLUtils.js net::ERR_FILE_NOT_FOUND
[Extension] failed to load G:/brackets/brackets-svg/src/extensions/default/SVGCodeHints - Module does not exist: file:///G:/brackets/brackets-svg/src/extensions/default/SVGCodeHints/language/XMLUtils.js

Working with XMLUtils = require("../../../language/XMLUtils"), works just fine. any suggestion?

CC: @redmunds @marcelgerber

marcelgerber · 2015-01-07T14:19:05Z

Yeah, you have to require it in brackets.js. Also make sure to include it in brackets.test.

redmunds · 2015-01-08T02:35:58Z

Note that SVG code hints do not work in <svg> tags embedded in HTML docs. We'll need to submit a change to the CodeMIrror htmlmixed mode to fix this, so it's not required for this pull request.

redmunds · 2015-01-08T02:39:42Z

src/extensions/default/SVGCodeHints/unittests.js

+            });
+
+            it("should exclude hints if they are already been used.", function () {
+                // In first <rect, after <rect+space


Grammar: should be "should exclude hints if they have already been used."

Oh, How did I miss that, will get it fixed tomorrow.

redmunds · 2015-01-08T03:07:40Z

Done with code review.

sprintr · 2015-01-08T03:11:36Z

@redmunds I will work on caching and those grammatical mistakes today. I think caching also needs a review.

sprintr · 2015-01-09T17:13:10Z

@redmunds caching has been implemented in getTagAttributes(), you may wanna have a look at it.

redmunds · 2015-01-09T17:46:23Z

src/extensions/default/SVGCodeHints/main.js

+                    cachedAttributes[tagName] = cachedAttributes[tagName].concat(tagData.attributeGroups[group]);
+                }
+            });
+        }


This caching looks good. The only change I would make would be to sort the list here so it doesn't have to be sorted every time the cached list is used. It's ok if getHints() calls sort() on list every time since it is fast if list is already sorted.

redmunds · 2015-01-09T17:54:42Z

@sprintr Just ~~one~~ a couple more comments about the code.

Note that after that's fixed the commits will need to be squashed into a single commit to keep the code base cleaner.

sprintr · 2015-01-09T18:02:45Z

@redmunds For HTML, brackets is not using fuzzy matching. That's what I kept in mind.

redmunds · 2015-01-09T18:12:25Z

@sprintr

For HTML, brackets is not using fuzzy matching.

OK. Implement it if you want, but it's ok if not.

sprintr · 2015-01-13T18:24:17Z

@redmunds Fuzzy hinting has been implemented, you can try it. If you think it is OK, then I can squash commits.

redmunds · 2015-01-14T02:20:06Z

@sprintr Looks good. Squash away!

sprintr · 2015-01-14T02:57:18Z

@redmunds squashed!

Update copyright notice. Change SVGTags.json to newer format. Adjust to the new format. Remove color from hints Add new line to unittests.js Format Simplify attribute options exclusion list More simplification to SVGUtils.js Updated comment Updated unittests.js for the changes. Improved format of SVGTags.json Refactoring Removed an error Remove alias properties to increase simplicity Add missing SVG attributes Cleanup Some more refactoring Updated unit tests Moved XMLUtils to core Added XMLUtils to brackets.js Added tests for XMLUtils-test.js Require XMLUtils Removed tabs Removed some more tabs. New line at the end. Removed tab. Removed unused variables. Remove XMLUtils from integration testing modules. Correct whitespace regex. Made proposed changes. Removed unused variables. Use a better name for attribute groups. Removed mistakes in unit tests Implement caching of attributes More optimization Refactor XMLUtils Sort cached attributes. Move preferences up Some more restrictions in XMLUtils Add fuzzy hinting Format Fix a bug with hinting Merge SVG code hints

redmunds · 2015-01-14T16:56:49Z

Thanks for this awesome contribution! Merging.

SVG Code Hints (#10084)

sprintr · 2015-01-14T17:33:59Z

Cool! It has been a privilege to work on Brackets.

rawat11 · 2015-02-02T06:06:18Z

@sprintr I tried embedding svg code directly in the html file since brackets supports HTML5, and was able to view it in the browser but Live Preview doesn't work in this case ??? I have to explicitly refresh the page ??
Have we implemented this ?

sprintr · 2015-02-02T13:24:25Z

htmlmixed mode (of CodeMirror) doesn't support SVG, so we don't have code hints inside HTML files. Live preview for SVG has not been implemented.

rawat11 · 2015-02-03T04:58:49Z

@sprintr okkay.
So for brackets we have htmlmixed mode. But code hints work in it.

sprintr · 2015-02-03T08:50:37Z

Brackets uses htmlmixed mode for HTML documents, and it doesn't support SVG right now. SVG code hints for now only work in SVG documents.

marcelgerber reviewed Jan 1, 2015
View reviewed changes

redmunds self-assigned this Jan 1, 2015

redmunds reviewed Jan 1, 2015
View reviewed changes

marcelgerber reviewed Jan 1, 2015
View reviewed changes

marcelgerber reviewed Jan 3, 2015
View reviewed changes

redmunds changed the title ~~[REVIEW ONLY] SVG Code Hints~~ [REVIEW ONLY] SVG Code Hints (#10084) Jan 7, 2015

redmunds reviewed Jan 8, 2015
View reviewed changes

redmunds reviewed Jan 9, 2015
View reviewed changes

redmunds added the PR Triage Complete label Jan 12, 2015

sprintr changed the title ~~[REVIEW ONLY] SVG Code Hints (#10084)~~ SVG Code Hints (#10084) Jan 14, 2015

redmunds added a commit that referenced this pull request Jan 14, 2015

Merge pull request #10294 from sprintr/master

ba96598

SVG Code Hints (#10084)

redmunds merged commit ba96598 into adobe:master Jan 14, 2015

This was referenced Aug 30, 2021

[CLOSED] Enable SVG Code Hints brackets-archive/bracketsIssues#8949

Open

[CLOSED] SVG Code Hints (#10084) brackets-archive/bracketsIssues#9120

Open

SVG Code Hints (#10084) #10294

SVG Code Hints (#10084) #10294

Conversation

sprintr commented Jan 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Jan 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sprintr commented Jan 2, 2015

sprintr commented Jan 3, 2015

sprintr commented Jan 3, 2015

sprintr commented Jan 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Jan 3, 2015

redmunds commented Jan 3, 2015

Choose a reason for hiding this comment

sprintr commented Jan 7, 2015

marcelgerber commented Jan 7, 2015

redmunds commented Jan 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Jan 8, 2015

sprintr commented Jan 8, 2015

sprintr commented Jan 9, 2015

Choose a reason for hiding this comment

redmunds commented Jan 9, 2015

sprintr commented Jan 9, 2015

redmunds commented Jan 9, 2015

sprintr commented Jan 13, 2015

redmunds commented Jan 14, 2015

sprintr commented Jan 14, 2015

redmunds commented Jan 14, 2015

sprintr commented Jan 14, 2015

rawat11 commented Feb 2, 2015

sprintr commented Feb 2, 2015

rawat11 commented Feb 3, 2015

sprintr commented Feb 3, 2015