Closed Bug 699978 Opened 10 years ago Closed 10 years ago

Tools should notify the highlighter when they've modified a node

Categories

(DevTools :: General, defect)

10 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: dcamp, Assigned: dcamp)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 2 obsolete files)

Attached patch WIP 1 (obsolete) — Splinter Review
... to let the highlighter and other tools know to update themselves after a change.
Attachment #572135 - Flags: feedback?(rcampbell)
Comment on attachment 572135 [details] [diff] [review]
WIP 1

looks good. We'll need to add the onUpdate references and methods to each of the registration objects and tools (I think the only one that'll really need it is the style inspector).
Attachment #572135 - Flags: feedback?(rcampbell) → feedback+
* Style inspector needs it (and should be easy, it already has refresh code)
* Rule View needs it (and will take a bit of code, in progress.  Will be needed if someone edits a style attribute in html view)
* HTML view needs it (when someone edits a style attribute in rule view).

Was planning to do each of these as followups, but we could do them all here.
your call, depending on the workload. Style inspector should be easy. just a matter of telling it to reselect the node.

Ruleview is baby, so you should know what needs doing.

HTML view is the trickiest of the bunch because of the ridiculous way the tree panel is constructed. Talked a bit about this in IRC, am willing to leave this as a followup.
Status: NEW → ASSIGNED
Attached patch final v1 (obsolete) — Splinter Review
OK, this version makes sure the highlighter rectangle and the style inspector are updated on changes from the html panel or the rule view.
Assignee: nobody → dcamp
Attachment #572135 - Attachment is obsolete: true
Attachment #572242 - Flags: review?(rcampbell)
Comment on attachment 572242 [details] [diff] [review]
final v1

   /**
+   * Notify register tools of changes to the highlighted element.
+   * @param object aUpdater

probably should read "notify registered tools". That is a pretty nitty comment.

CssRuleView.jsm

+  /**
    * Creates editor UI for each of the rules in _elementStyle.
    */
   _createEditors: function CssRuleView_createEditors()
@@ -1003,10 +1034,12 @@ TextPropertyEditor.prototype = {
    */
   _parseValue: function TextPropertyEditor_parseValue(aValue)

should probably document aValue and @return

ok!
Attachment #572242 - Flags: review?(rcampbell) → review+
(In reply to Rob Campbell [:rc] (robcee) from comment #5)
> Comment on attachment 572242 [details] [diff] [review] [diff] [details] [review]
> final v1
> 
>    /**
> +   * Notify register tools of changes to the highlighted element.
> +   * @param object aUpdater
> 
> probably should read "notify registered tools". That is a pretty nitty
> comment.
> 

Oops, updated.

>     */
>    _parseValue: function TextPropertyEditor_parseValue(aValue)
> 
> should probably document aValue and @return

Bad context, those are documented before the diff cutoff.
Attached patch v2Splinter Review
Fixed review comments and a minor test mistake.
Attachment #572242 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/815234795765
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/815234795765
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.