519131 - Initialize Add Watch Expression dialog with current source selection

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: prefill dialog

Currently there is no easy way to set up a watch expression based on the current selection in the source view. I can imagine multiple alternatives (drag & drop, shortcut, context menu) but I think only the latter is really needed so that's what I implemented.

With the patch, any invocation through a context menu, be it from the watches or source view, brings up the prompt with the text selection of the currently selected source tab filled in (preselected, so hitting Del is enough to clear it). For that reason, there is no need to change the context menu item label (which has an ellipsis at the end). If there is no selection or no source tab, the prompt is empty as before.

The current text selection is passed without any checks or cleanups which should be OK since you can enter anything at the prompt anyway. There's only one minor glitch in that getSelection().toString() adds line numbers in between multi-line selections but I think we can live with that.

Comment 1

[:Gijs (he/him)]

Comment on attachment 403140 [details] [diff] [review]
prefill dialog

>diff --git a/resources/content/venkman-views.js b/resources/content/venkman-views.js
>--- a/resources/content/venkman-views.js
>+++ b/resources/content/venkman-views.js
>@@ -2262,16 +2262,17 @@ function ss_init ()
>     console.menuSpecs["context:source2"] = {
>         getContext: this.getContext,
>         items:
>         [
>          ["close-source-tab"],
>          ["reload-source-tab"],
>          ["save-source-tab", {enabledif: "console.views.source2.canSave()"}],
>          ["find-string"],
>+         ["watch-expr"],

This should only be enabled if there is a source view with a selection. You'd probably want to write a subroutine to check for that (like the canSave one used a bit above this addition).

>@@ -4365,24 +4366,36 @@ function cmdWatchExpr (e)
>             {
>                 display (getMsg(MSN_FMT_WATCH_ITEM,
>                                 [i, watchData[i].displayName,
>                                  watchData[i].displayValue]));
>             }
>             return null;
>         }
> 
>+        var watchExpr = "";
>+        var source2View = console.views.source2;
>+        if (source2View.tabs)

If I have text selected in the source view, I don't necessarily want the context menu anywhere else to use it... I'm not sure if the event object (e) knows which context menu invoked the command, you'd have to check. Otherwise, you may need to write a mini new command (with no flags, so no CMD_CONSOLE) that calls this one with the relevant e.watchExpression set (use |dispatch("watch-expr", ...)|, there should be other uses of it in the code).

>+        {
>+            var index = source2View.tabs.selectedIndex;
>+            if (index in source2View.sourceTabList)

Is selectedIndex likely to be useless? -1 for no selection, I guess? I'd prefer to check for that, then, rather than use the "in" operator, which confuses me, at least :-)

>+            {
>+                var browser = source2View.sourceTabList[index].iframe;
>+                watchExpr = browser.contentWindow.getSelection().toString();

I'm betting you can filter out the line numbers by dealing with the bit of DOM you get back in a clever way. Maybe you could use filter() and concat the toString()'s of the resulting array? I'd expect I'd want to use
this for long method calls, and still have it work, for instance. :-)

>+            }
>+        }
>+
>         var parent;
>             
>         if (watches.currentContent)
>             parent = watches.currentContent.ownerWindow;
>         else
>             parent = window;
>             
>-        e.watchExpression = prompt(MSG_ENTER_WATCH, "", parent);
>+        e.watchExpression = prompt(MSG_ENTER_WATCH, watchExpr, parent);

In which case, after filtering out the line numbers and only using the selection from the source view context menu, I'd expect you'd want to not have the prompt if the context menu was used. I'd be careful about selecting the right bit, and if inadvertently I still got it wrong, I could edit it afterward...

r- mostly because of the enabledif and having the text be used from every context menu, but I'd also like to see the other things addressed if you agree / they are possible. :-)

(PS: sorry I didn't get round to checking in that other patch - seems like Ian took care of it though!)

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

(In reply to comment #1)
> >+        {
> >+            var index = source2View.tabs.selectedIndex;
> >+            if (index in source2View.sourceTabList)
> 
> Is selectedIndex likely to be useless? -1 for no selection, I guess? I'd prefer
> to check for that, then, rather than use the "in" operator, which confuses me,
> at least :-)

No, selectedIndex is still 0 if there are no tabs anymore... As for the in operator: It's used just like that multiple times in other places of the file...

> >+            {
> >+                var browser = source2View.sourceTabList[index].iframe;
> >+                watchExpr = browser.contentWindow.getSelection().toString();
> 
> I'm betting you can filter out the line numbers by dealing with the bit of DOM
> you get back in a clever way. Maybe you could use filter() and concat the
> toString()'s of the resulting array? I'd expect I'd want to use
> this for long method calls, and still have it work, for instance. :-)

Unfortunately I found nothing helpful there. getSelection() returns a single range object for a multi-line selection and I found nothing to identify the line numbers appearing there, neither in Selection nor range (cf. MDC). I agree it would be nice to have but at this point I have to admit I cannot do it.

Anyway, the most prominent use case is single-line selections so the loss is tolerable.

> In which case, after filtering out the line numbers and only using the
> selection from the source view context menu, I'd expect you'd want to not have
> the prompt if the context menu was used. I'd be careful about selecting the
> right bit, and if inadvertently I still got it wrong, I could edit it
> afterward...

You cannot edit a watch expression after the fact. ;-)

Even if that was not the case I'd still like to keep it, for multiple reasons:
1. you can check and edit it before creating it (which is especially important since I cannot fix the line number issue)
2. you can replace it by something completely different (press Del or just type)
3. we can reuse the menu item label (ellipsis -> dialog)

> (PS: sorry I didn't get round to checking in that other patch - seems like Ian
> took care of it though!)

I asked on IRC. :-)

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #2)
> > I'm betting you can filter out the line numbers by dealing with the bit of DOM
> > you get back in a clever way. Maybe you could use filter() and concat the
> > toString()'s of the resulting array? I'd expect I'd want to use
> > this for long method calls, and still have it work, for instance. :-)
> 
> Unfortunately I found nothing helpful there. getSelection() returns a single
> range object for a multi-line selection and I found nothing to identify the
> line numbers appearing there, neither in Selection nor range (cf. MDC). I agree
> it would be nice to have but at this point I have to admit I cannot do it.

... unless you want to get nasty:

var sel = browser.contentWindow.getSelection();
for (var i = 0; i < sel.rangeCount; ++i)
    watchExpr += sel.getRangeAt(i).toString().replace(/\n [- ]  \d+\s*/g, "");

works (also removes whitespace at the start of such lines). :-)

Your decision.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3

(In reply to comment #3)
> var sel = browser.contentWindow.getSelection();
> for (var i = 0; i < sel.rangeCount; ++i)
>     watchExpr += sel.getRangeAt(i).toString().replace(/\n [- ]  \d+\s*/g, "");
> 
> works (also removes whitespace at the start of such lines). :-)

While changing [- ] to [-BF ] would have been enough to make this complete I chose to follow the sane path as suggested on IRC. I hope you like it. :-)

Just in case you wonder why I went through the hassle of making this so generic with optional trimming: Just select something and copy it to the clipboard...

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #4)
> Created an attachment (id=403328) [details]
> patch v3

On second thought I think it'd be better to make trim/trimFn a function parameter filterFn (to be set to the identity function if null or not a function) to make it even more generic. That way one could not only trim but also do other things like only skipping line breaks (filterFn = function (text) = {return text=="\n" : "" : text;}) which happen to be stored in one text node each (on fragment.childNodes level).

Comment 6

[:Gijs (he/him)]

Comment on attachment 403328 [details] [diff] [review]
patch v3

>diff --git a/resources/content/venkman-views.js b/resources/content/venkman-views.js
>--- a/resources/content/venkman-views.js
>+++ b/resources/content/venkman-views.js
>@@ -2468,16 +2469,30 @@ function cmdToggleColoring (e)
> }
> 
> console.views.source2.canSave =
> function s2v_cansave ()
> {
>     return this.tabs && this.sourceTabList.length > 0;
> }
> 
>+console.views.source2.hasSelection =
>+function s2v_hasselection ()
>+{
>+    if (!this.tabs)
>+        return;
>+
>+    var index = this.tabs.selectedIndex;
>+    if (!(index in this.sourceTabList))
>+        return;
>+
>+    var browser = this.sourceTabList[index].iframe;
>+    return browser.contentWindow.getSelection().isCollapsed == false;

Hmm. Ideally, if I right click in the source view without a selection, I think the expected behaviour would be to add a watch for the whitespace-delimited expression? So if I right click between the t and S in the above, for example, I'd want a watch for:
browser.contentWindow.getSelection().isCollapsed

Or perhaps just browser.contentWindow.getSelection(), that may be debatable... Anyway, if you don't feel like doing that feature now, you can leave it like this for a bit and file a followup bug. :-)

>@@ -3529,16 +3544,64 @@ function s2v_hide ()
>     if (this.sourceTabList.length > 0)
>         this.lastSelectedTab = this.tabs.selectedIndex;
>     this.clearOutputDeck();
>     this.deck = null;
>     this.tabs = null;
>     this.heading = null;
> }
> 
>+console.views.source2.getSelectedTextForTab =
>+function s2v_getselectedtextfortab (index, trim)
>+{
>+    if (this.tabs && index == null)
>+        index = this.tabs.selectedIndex;
>+
>+    if (!(index in this.sourceTabList))
>+        return "";
>+
>+    var trimFn;
>+    if (trim)
>+        trimFn = function (text) {return text.trim();}
>+    else
>+        trimFn = function (text) {return text;}

With respect to comment #5 and comment #4, I am not sure if it is useful to make the trimming still more generic. At the moment, we wouldn't be using this alternative code path either, so adding more complexity that we're not using feels like a bit of overkill. 

>+
>+    var text = "";
>+    var browser = this.sourceTabList[index].iframe;
>+    var sel = browser.contentWindow.getSelection();
>+    for (var i = 0; i < sel.rangeCount; ++i)
>+    {
>+        var fragment = sel.getRangeAt(i).cloneContents();
>+        if (fragment.hasChildNodes)

I presume you mean hasChildNodes() ?

>+        {
>+            var node, childNode;
>+            for each (node in fragment.childNodes)
>+            {
>+                if (node.nodeType == Node.TEXT_NODE)
>+                    text += trimFn(node.textContent);
Nit: This should have braces, per file (and Vnk) style, because the else has braces.

>+                else if (node.nodeType == Node.ELEMENT_NODE)
>+                {
>+                    for each (childNode in node.childNodes)
Nit: This loop should have braces, because it has a multiline body.
>+                        // skip markers and line numbers
Maybe "breakpoint markers" rather than just "markers".

>+                        if (childNode.nodeType == Node.TEXT_NODE ||
>+                            (childNode.nodeType == Node.ELEMENT_NODE &&
>+                             (childNode.nodeName != "margin" &&
>+                              childNode.nodeName != "num")))

Nit: You're doing: a && (b && c) -- could just use a && b && c.

>+                            text += trimFn(childNode.textContent);

Nit: This should have braces because of the condition being multiline.

In general, maybe it would be cleaner to do:

>                    for each (childNode in node.childNodes)
>                    {
>                        // skip markers and line numbers
>                        var nodeName = childNode.nodeName;
>                        if (nodeName == "margin" || nodeName == "num")
>                            continue;
>                        text += trimFn(childNode.textContent);
>                    }
Fewer braces and yucky conditions, does the same stuff! (nodeName is "#text" for text nodes)

>+                }
>+            }
>+        }
>+        else
>+        {
>+            text += trimFn(fragment.textContent);
>+        }
>+    }
>+    return text;
>+}
>+
> /*******************************************************************************
>  * Source View
>  *******************************************************************************/
> console.views.source = new BasicOView();
> 
> const VIEW_SOURCE = "source";
> console.views.source.viewId = VIEW_SOURCE;
> 
>@@ -4365,24 +4428,28 @@ function cmdWatchExpr (e)
>             {
>                 display (getMsg(MSN_FMT_WATCH_ITEM,
>                                 [i, watchData[i].displayName,
>                                  watchData[i].displayValue]));
>             }
>             return null;
>         }
> 
>+        var watchExpr = "";

Add a comment here explaining that we check for selection in the source view.

>+        var source2View = console.views.source2;
>+        if (e.contextSource == "context:source2" && source2View.tabs)
>+            watchExpr = source2View.getSelectedTextForTab(null, true);
>+
>         var parent;
>-            
>         if (watches.currentContent)
>             parent = watches.currentContent.ownerWindow;
>         else
>             parent = window;
>             
>-        e.watchExpression = prompt(MSG_ENTER_WATCH, "", parent);
>+        e.watchExpression = prompt(MSG_ENTER_WATCH, watchExpr, parent);

Yeah, I guess we need an edit function... for now, you can leave it like that. :-(

>         if (!e.watchExpression)
>             return null;
>     }
>     
>     var refresher;
>     if (e.command.name == "watch-expr")
>     {
>         if (!("currentEvalObject" in console))


r- because of the fact that the current code might stop working if you use a correct check for hasChildNodes - but please fix the (other) nits too :-)

PS: I considered your use of trim here - there is a utility function (stringTrim), but bug 472395 is related, kind of. I guess we will now move HG trunk development to work well for the latest release(s) of SeaMonkey and Firefox/Thunderbird/Sunbird only, rather than keeping loads of backwards compat, in which case using "foo".trim() throughout is probably good. If you feel like it, you can do the rather trivial patch for bug 472395 (if not, I'll do it, but I'll need someone else to review it... ;-) ).

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v4

(In reply to comment #6)
> Hmm. Ideally, if I right click in the source view without a selection, I think
> the expected behaviour would be to add a watch for the whitespace-delimited
> expression?

Yes, ideally. ;-)

> Anyway, if you don't feel like doing that feature now, you can leave it like
> this for a bit and file a followup bug. :-)

I'll take that path it'll probably not be as clean/simple as what I have right now.

> With respect to comment #5 and comment #4, I am not sure if it is useful to
> make the trimming still more generic. At the moment, we wouldn't be using this
> alternative code path either, so adding more complexity that we're not using
> feels like a bit of overkill. 

I think it has more benefit than overhead/complexity, that's why I implemented what I suggested. The idea is this: trim also removes spaces after keywords (if the source highlights them) but that's OK for Add Watch Expression. Instead of using trim you could easily just remove newlines and as a result have a function that gives you the source as-is in one line. Finally, using the identity function (= using null as second function parameter) we can in the future override the default copy command (cmd_copy) which is currently broken. And all that without having to modify the existing function. :-)

> In general, maybe it would be cleaner to do:
> 
> >                    for each (childNode in node.childNodes)
> >                    {
> >                        // skip markers and line numbers
> >                        var nodeName = childNode.nodeName;
> >                        if (nodeName == "margin" || nodeName == "num")
> >                            continue;
> >                        text += trimFn(childNode.textContent);
> >                    }
> Fewer braces and yucky conditions, does the same stuff! (nodeName is "#text"
> for text nodes)

Yes, that's even better.

> PS: I considered your use of trim here - there is a utility function
> (stringTrim), but bug 472395 is related, kind of. I guess we will now move HG
> trunk development to work well for the latest release(s) of SeaMonkey and
> Firefox/Thunderbird/Sunbird only, rather than keeping loads of backwards
> compat, in which case using "foo".trim() throughout is probably good. If you
> feel like it, you can do the rather trivial patch for bug 472395 (if not, I'll
> do it, but I'll need someone else to review it... ;-) ).

I'll have a look at it after this went in. Can you ask Robert to unassign first? Or is it OK to just change the assignee to myself?

Comment 8

[:Gijs (he/him)]

Comment on attachment 403860 [details] [diff] [review]
patch v4

r=me

(In reply to comment #7)
> > Anyway, if you don't feel like doing that feature now, you can leave it like
> > this for a bit and file a followup bug. :-)
> 
> I'll take that path it'll probably not be as clean/simple as what I have right
> now.

OK, got a bug filed already? :-)

> I'll have a look at it after this went in. Can you ask Robert to unassign
> first? Or is it OK to just change the assignee to myself?

In general, yes, it is OK to just change the assignee. I did it for you just now ;-)

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #8)
> (From update of attachment 403860 [details] [diff] [review])
> r=me
> 
> (In reply to comment #7)
> > > Anyway, if you don't feel like doing that feature now, you can leave it like
> > > this for a bit and file a followup bug. :-)
> > 
> > I'll take that path it'll probably not be as clean/simple as what I have right
> > now.
> 
> OK, got a bug filed already? :-)

Filed bug 519992 (I wanted to have r+ here first).

Comment 10

[:Gijs (he/him)]

http://hg.mozilla.org/venkman/rev/d2faf77446e4

Fixed, thanks Jens!