Merge pull request #5541 from adobe/nj/update-rule-label

[Initial review] Update rule label when selector is edited

Author: dangoor

src/document/TextRange.js

@@ -40,8 +40,10 @@ define(function (require, exports, module) {
      * Important: you must dispose() a TextRange when you're done with it. Because TextRange addRef()s
      * the Document (in order to listen to it), you will leak Documents otherwise.
      *
-     * TextRange dispatches two events:
-     *  - change -- When the range changes (due to a Document change)
+     * TextRange dispatches these events:
+     *  - change -- When the range boundary line numbers change (due to a Document change)
+     *  - contentChange -- When the actual content of the range changes. This might or might not
+     *          be accompanied by a change in the boundary line numbers.
      *  - lostSync -- When the backing Document changes in such a way that the range can no longer
      *          accurately be maintained. Generally, occurs whenever an edit spans a range boundary.
      *          After this, startLine & endLine will be unusable (set to null).
@@ -86,7 +88,10 @@ define(function (require, exports, module) {
 
     /**
      * Applies a single Document change object (out of the linked list of multiple such objects)
-     * to this range. Returns true if the range was changed as a result.
+     * to this range.
+     * @param {Object} change The CodeMirror change record.
+     * @return {{hasChanged: boolean, hasContentChanged: boolean}} Whether the range boundary
+     *     and/or content has changed.
      */
     TextRange.prototype._applySingleChangeToRange = function (change) {
         // console.log(this + " applying change to (" +
@@ -97,7 +102,7 @@ define(function (require, exports, module) {
         if (!change.from || !change.to) {
             this.startLine = null;
             this.endLine = null;
-            return true;
+            return {hasChanged: true, hasContentChanged: true};
 
         // Special case: certain changes around the edges of the range are problematic, because
         // if they're undone, we'll be unable to determine how to fix up the range to include the
@@ -116,29 +121,36 @@ define(function (require, exports, module) {
                    (change.from.line <= this.endLine && change.to.line > this.endLine)) {
             this.startLine = null;
             this.endLine = null;
-            return true;
+            return {hasChanged: true, hasContentChanged: true};
 
         // Normal case: update the range end points if any content was added before them. Note that
         // we don't rely on line handles for this since we want to gracefully handle cases where the
         // start or end line was deleted during a change.
         } else {
             var numAdded = change.text.length - (change.to.line - change.from.line + 1);
-            var hasChanged = false;
+            var result = {hasChanged: false, hasContentChanged: false};
 
             // This logic is so simple because we've already excluded all cases where the change
             // crosses the range boundaries
-            if (change.to.line < this.startLine) {
-                this.startLine += numAdded;
-                hasChanged = true;
+            if (numAdded !== 0) {
+                if (change.to.line < this.startLine) {
+                    this.startLine += numAdded;
+                    result.hasChanged = true;
+                }
+                if (change.to.line <= this.endLine) {
+                    this.endLine += numAdded;
+                    result.hasChanged = true;
+                }
             }
-            if (change.to.line <= this.endLine) {
-                this.endLine += numAdded;
-                hasChanged = true;
+            if (change.from.line >= this.startLine && change.from.line <= this.endLine) {
+                // Since we know the change doesn't cross the range boundary, as long as the
+                // start of the change is within the range, we know the content changed.
+                result.hasContentChanged = true;
             }
 
             // console.log("Now " + this);
 
-            return hasChanged;
+            return result;
         }
     };
 
@@ -148,12 +160,13 @@ define(function (require, exports, module) {
      * range can no longer be accurately maintained.
      */
     TextRange.prototype._applyChangesToRange = function (changeList) {
-        var hasChanged = false;
+        var hasChanged = false, hasContentChanged = false;
         var change;
         for (change = changeList; change; change = change.next) {
             // Apply this step of the change list
             var result = this._applySingleChangeToRange(change);
-            hasChanged = hasChanged || result;
+            hasChanged = hasChanged || result.hasChanged;
+            hasContentChanged = hasContentChanged || result.hasContentChanged;
 
             // If we lost sync with the range, just bail now
             if (this.startLine === null || this.endLine === null) {
@@ -165,6 +178,9 @@ define(function (require, exports, module) {
         if (hasChanged) {
             $(this).triggerHandler("change");
         }
+        if (hasContentChanged) {
+            $(this).triggerHandler("contentChange");
+        }
     };
 
     TextRange.prototype._handleDocumentChange = function (event, doc, changeList) {

src/editor/CSSInlineEditor.js

@@ -229,10 +229,10 @@ define(function (require, exports, module) {
             });
             return result;
         }
-
+        
         CSSUtils.findMatchingRules(selectorName, hostEditor.document)
             .done(function (rules) {
-                cssInlineEditor = new MultiRangeInlineEditor(rules || [], _getNoRulesMsg);
+                cssInlineEditor = new MultiRangeInlineEditor(CSSUtils.consolidateRules(rules) || [], _getNoRulesMsg, CSSUtils.getRangeSelectors);
                 cssInlineEditor.load(hostEditor);
 
                 var $header = $(".inline-editor-header", cssInlineEditor.$htmlContent);

src/editor/MultiRangeInlineEditor.js

@@ -74,7 +74,10 @@ define(function (require, exports, module) {
     SearchResultItem.prototype.textRange = null;
     SearchResultItem.prototype.$listItem = null;
 
-    function _updateRangeLabel(listItem, range) {
+    function _updateRangeLabel(listItem, range, labelCB) {
+        if (labelCB) {
+            range.name = labelCB(range.textRange);
+        }
         var text = StringUtils.htmlEscape(range.name) + " <span class='related-file'>— " + StringUtils.htmlEscape(range.textRange.document.file.name) + " : " + (range.textRange.startLine + 1) + "</span>";
         listItem.html(text);
         listItem.attr("title", listItem.text());
@@ -85,9 +88,11 @@ define(function (require, exports, module) {
      * @param {Array.<{name:String,document:Document,lineStart:number,lineEnd:number}>} ranges The text ranges to display.
      * @param {function(): $.Promise} messageCB An optional callback that returns a promise that will be resolved with a message to show
      *      when no matches are available.
+     * @param {function(range): string} labelCB An optional callback that returns an updated label string for the given range. Called
+     *      when we detect that the content of one of the ranges has changed.
      * @extends {InlineTextEditor}
      */
-    function MultiRangeInlineEditor(ranges, messageCB) {
+    function MultiRangeInlineEditor(ranges, messageCB, labelCB) {
         InlineTextEditor.call(this);
 
         // Store the results to show in the range list. This creates TextRanges bound to the Document,
@@ -96,6 +101,7 @@ define(function (require, exports, module) {
             return new SearchResultItem(rangeResult);
         });
         this._messageCB = messageCB;
+        this._labelCB = labelCB;
 
         this._selectedRangeIndex = -1;
     }
@@ -113,6 +119,7 @@ define(function (require, exports, module) {
     MultiRangeInlineEditor.prototype._ranges = null;
     MultiRangeInlineEditor.prototype._selectedRangeIndex = null;
     MultiRangeInlineEditor.prototype._messageCB = null;
+    MultiRangeInlineEditor.prototype._labelCB = null;
 
     /**
      * @private
@@ -133,6 +140,8 @@ define(function (require, exports, module) {
         // Update list item as TextRange changes
         $(range.textRange).on("change", function () {
             _updateRangeLabel($rangeItem, range);
+        }).on("contentChange", function () {
+            _updateRangeLabel($rangeItem, range, self._labelCB);
         });
 
         // If TextRange lost sync, remove it from the list (and close the widget if no other ranges are left)

src/language/CSSUtils.js

@@ -504,6 +504,8 @@ define(function (require, exports, module) {
                                    starts that this selector (e.g. .baz) is part of. Particularly relevant for
                                    groups that are on multiple lines.
          selectorGroupStartChar:   column in line where the selector group starts.
+         selectorGroup:            the entire selector group containing this selector, or undefined if there 
+                                   is only one selector in the rule.
          declListStartLine:        line where the declaration list for the rule starts
          declListStartChar:        column in line where the declaration list for the rule starts
          declListEndLine:          line where the declaration list for the rule ends
@@ -690,6 +692,25 @@ define(function (require, exports, module) {
             var j;
             declListStartLine = line;
             declListStartChar = stream.start;
+            
+            // Extract the entire selector group we just saw.
+            var selectorGroup, sgLine;
+            if (selectorGroupStartLine !== -1) {
+                selectorGroup = "";
+                for (sgLine = selectorGroupStartLine; sgLine <= declListStartLine; sgLine++) {
+                    var startChar = 0, endChar = lines[sgLine].length;
+                    if (sgLine === selectorGroupStartLine) {
+                        startChar = selectorGroupStartChar;
+                    } else {
+                        selectorGroup += " "; // replace the newline with a single space
+                    }
+                    if (sgLine === declListStartLine) {
+                        endChar = declListStartChar;
+                    }
+                    selectorGroup += lines[sgLine].substring(startChar, endChar);
+                }
+                selectorGroup = selectorGroup.trim();
+            }
 
             // Since we're now in a declaration list, that means we also finished
             // parsing the whole selector group. Therefore, reset selectorGroupStartLine
@@ -706,7 +727,7 @@ define(function (require, exports, module) {
                 }
             }
 
-            // assign this declaration list position to every selector on the stack
+            // assign this declaration list position and selector group to every selector on the stack
             // that doesn't have a declaration list start and end line
             for (j = selectors.length - 1; j >= 0; j--) {
                 if (selectors[j].declListEndLine !== -1) {
@@ -716,6 +737,9 @@ define(function (require, exports, module) {
                     selectors[j].declListStartChar = declListStartChar;
                     selectors[j].declListEndLine = line;
                     selectors[j].declListEndChar = stream.pos - 1; // stream.pos actually points to the char after the }
+                    if (selectorGroup) {
+                        selectors[j].selectorGroup = selectorGroup;
+                    }
                 }
             }
         }
@@ -915,7 +939,8 @@ define(function (require, exports, module) {
                 name: selectorInfo.selector,
                 document: sourceDoc,
                 lineStart: selectorInfo.ruleStartLine + lineOffset,
-                lineEnd: selectorInfo.declListEndLine + lineOffset
+                lineEnd: selectorInfo.declListEndLine + lineOffset,
+                selectorGroup: selectorInfo.selectorGroup
             });
         });
     }
@@ -1220,13 +1245,66 @@ define(function (require, exports, module) {
         };
     }
 
+    /**
+     * 
+     * In the given rule array (as returned by `findMatchingRules()`), if multiple rules in a row 
+     * refer to the same rule (because there were multiple matching selectors), eliminate the redundant 
+     * rules and use the selectorGroup as the name instead of the original selector.
+     */
+    function consolidateRules(rules) {
+        var newRules = [], lastRule;
+        rules.forEach(function (rule) {
+            if (lastRule &&
+                    rule.document === lastRule.document &&
+                    rule.lineStart === lastRule.lineStart &&
+                    rule.lineEnd === lastRule.lineEnd &&
+                    rule.selectorGroup === lastRule.selectorGroup) {
+                lastRule.name = lastRule.selectorGroup;
+            } else {
+                newRules.push(rule);
+            }
+            lastRule = rule;
+        });
+        return newRules;
+    }
+    
+    /**
+     * Given a TextRange, extracts the selector(s) for the rule in the range and returns it.
+     * Assumes the range only contains one rule; if there's more than one, it will return the
+     * selector(s) for the first rule.
+     * @param {TextRange} range The range to extract the selector(s) from.
+     * @return {string} The selector(s) for the rule in the range.
+     */
+    function getRangeSelectors(range) {
+        // There's currently no immediate way to access a given line in a Document, because it's just
+        // stored as a string. Eventually, we should have Documents cache the lines in the document
+        // as well, or make them use CodeMirror documents which do the same thing.
+        var i, startIndex = 0, endIndex, text = range.document.getText();
+        for (i = 0; i < range.startLine; i++) {
+            startIndex = text.indexOf("\n", startIndex) + 1;
+        }
+        endIndex = startIndex;
+        // Go one line past the end line. We'll extract text up to but not including the last newline.
+        for (i = range.startLine + 1; i <= range.endLine + 1; i++) {
+            endIndex = text.indexOf("\n", endIndex) + 1;
+        }
+        var allSelectors = extractAllSelectors(text.substring(startIndex, endIndex));
+        
+        // There should only be one rule in the range, and if there are multiple selectors for
+        // the first rule, they'll all be recorded in the "selectorGroup" for the first selector,
+        // so we only need to look at the first one.
+        return (allSelectors.length ? allSelectors[0].selectorGroup || allSelectors[0].selector : "");
+    }
+        
     exports._findAllMatchingSelectorsInText = _findAllMatchingSelectorsInText; // For testing only
     exports.findMatchingRules = findMatchingRules;
     exports.extractAllSelectors = extractAllSelectors;
     exports.extractAllNamedFlows = extractAllNamedFlows;
     exports.findSelectorAtDocumentPos = findSelectorAtDocumentPos;
     exports.reduceStyleSheetForRegExParsing = reduceStyleSheetForRegExParsing;
     exports.addRuleToDocument = addRuleToDocument;
+    exports.consolidateRules = consolidateRules;
+    exports.getRangeSelectors = getRangeSelectors;
 
     exports.SELECTOR = SELECTOR;
     exports.PROP_NAME = PROP_NAME;

test/UnitTestSuite.js

@@ -64,6 +64,7 @@ define(function (require, exports, module) {
     require("spec/QuickOpen-test");
     require("spec/RemoteFunctions-test");
     require("spec/StringMatch-test");
+    require("spec/TextRange-test");
     require("spec/UpdateNotification-test");
     require("spec/ViewCommandHandlers-test");
     require("spec/ViewUtils-test");