331940 - Should be able to pass a line number to the external viewer/editor

Status: RESOLVED FIXED

(Toolkit :: Preferences, enhancement)

Description

[Simon Bünzli]

It would be nice if the option to invoke an external viewer/editor as provided by bug 172817 could be extended to make the editor optionally jump to a specified line number. This might come in handy when the JavaScript console also uses viewSourceUtils to open the source viewer (which IMO should happen for consistency's sake) and would be nice to have for Console² which already respects the pref view_source.editor.external.

I suppose that adding one further pref (view_source.editor.line_arg) which defaults to "" but could be set to something like "/line:%s" and would (if set) be inserted as an argument before the filename should be sufficient.

Jason: Can you take a look at this?

Comment 1

[Jason Barnabe (np)]

Attachment: patch v1

This patch implements view_source.editor.line_number_arg. It also makes the JavaScript Console respect the internal/external preference (it's also the only consumer of the new functionality).

Unrelatedly, it adds some dump() statements for when the external editor fails and falls back on the internal viewer, which currently fails silently.

Comment 2

[Simon Bünzli]

Thanks for the patch. Some details:

> function ViewSourceOfURL(aURL, aPageDescriptor, aDocument)

Why not optionally include a line number argument here also? Like that, this function could serve extensions as part of the internal API for extensions. Otherwise, it probably wouldn't even be necessary (since it just passes on the arguments).

>+  viewSource: function(aURL, aPageDescriptor, aDocument, lineNumber)
nit: aLineNumber (also for openInInternalViewer and openInInternalViewer)

+  openInExternalEditor: function(aURL, aPageDescriptor, aDocument, aCallBack, lineNumber)
Why not lineNumber before aCallBack? This would keep the arguments to openInInternalViewer and openInExternalEditor the same - except for the additional callback for the latter.

>+          var lineNumberArg, goToLine;
Why not default goToLine to false here instead of setting it in the else branch below?

>+            goToLine = !!lineNumberArg;
goToLine = lineNumberArg.length > 0 or even = lineNumberArg.indexOf("%s") > -1 would rather describe what you test for.

Comment 3

[Jason Barnabe (np)]

(In reply to comment #2)
> > function ViewSourceOfURL(aURL, aPageDescriptor, aDocument)
> 
> Why not optionally include a line number argument here also? Like that, this
> function could serve extensions as part of the internal API for extensions.
I'm intending on having gViewSourceUtils.viewSource be the internal API to call the appropriate View Source. I'm not removing ViewSourceOfURL in case something already calls it. I'm not giving it a line number argument because nothing in browser opens View Source to a line number.

> +  openInExternalEditor: function(aURL, aPageDescriptor, aDocument, aCallBack,
> lineNumber)
> Why not lineNumber before aCallBack?
Just to keep current users of openInExternalEditor working. I'm not sure if that's necessary, but I'll leave it as is and see what the reviewer has to say about it.

Comment 4

[Simon Bünzli]

(In reply to comment #3)
> I'm not giving it a line number argument because nothing in
> browser opens View Source to a line number.
My point was that an extension might want to do so and if gViewSourceUtils.viewSource is supposed to be internal, ViewSourceOfURL would be the place for the additional argument.

> Just to keep current users of openInExternalEditor working.
Since viewSourceUtils.js hasn't shipped with anything else but nightlies, I doubt that backwards compatibility will be an issue here (ignoring the fact that compatibility has already been broken between releases for as little as one capitalized letter). I'd try to stick to the cleaner solution here - but of course that's just my opinion.

Comment 5

[Jason Barnabe (np)]

Attachment: patch v2

Updated to comments. Just to reiterate, I want view source consumers to use gViewSourceUtils.viewSource. ViewSourceOfURL in browser.js is just for backwards compatibility.

Comment 6

[venessa]

How come no one posts SIMPLE ANSWERS to this problem? I have spent hours already trying to fix this! The only solution may be to go back to IE!!!! which displays the word YAHOO on the web page at least.  Mozilla doesn't, amoungst other things that it no longer displays! I am so annoyed ....It drops the label according to JS Console. 

Comment 7

[Mike Connor [:mconnor]]

An enhancement to a behaviour governed by a hidden pref isn't a blocker, but I'll try to get to the review in time to take the patch.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 216804 [details] [diff] [review]
patch v2

For what it's worth, posting a short comment describing the changes in the patch makes reviewing patches like this (where I'm not entirely familiar with the code) much easier.

1) For the script changes to console.xul: does viewSourceUtils depend on contentAreaUtils? For what?
2) For the consoleBindings.xml click handler: it looks to me that since it always used to launch viewSource.xul, the external editor pref didn't work for those links, and now you're fixing it so that it does, correct?
3) Components.util.reportError() should be preferred over dump(), generally
4) Seems to me that if we're going to add this ability to pass a parameter to the external editor, we should probably consider making it more generic ("view_source.editor.args"?), and using something like "%LINE%" for the line number, in case we want to add additional arguments to the editor later on.

Comment 9

[Jason Barnabe (np)]

(In reply to comment #8)
> 1) For the script changes to console.xul: does viewSourceUtils depend on
> contentAreaUtils? For what?

Yes, for the functions used in getTemporaryFile.

> 2) For the consoleBindings.xml click handler: it looks to me that since it
> always used to launch viewSource.xul, the external editor pref didn't work for
> those links, and now you're fixing it so that it does, correct?

Yes, per comment 1. Since at the time it didn't go to the correct line number, I didn't change that when external editors was implemented.

> 4) Seems to me that if we're going to add this ability to pass a parameter to
> the external editor, we should probably consider making it more generic
> ("view_source.editor.args"?), and using something like "%LINE%" for the line
> number, in case we want to add additional arguments to the editor later on.

nsIProcess accepts an array of arguments, not a string.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 216804 [details] [diff] [review]
patch v2

>Index: browser/app/profile/firefox.js

> pref("view_source.editor.path", "");
> pref("view_source.editor.external", false);
>+pref("view_source.editor.line_number_arg", "");

Please add a brief comment with an example of how it should be used (mention "%s", etc)?

>Index: browser/base/content/browser.js

> function ViewSourceOfURL(aURL, aPageDescriptor, aDocument)
> {
>-  if (getBoolPref("view_source.editor.external", false)) {
>-    gViewSourceUtils.openInExternalEditor(aURL, aPageDescriptor, aDocument);
>-  }
>-  else {
>-    gViewSourceUtils.openInInternalViewer(aURL, aPageDescriptor, aDocument);
>-  }
>+  gViewSourceUtils.viewSource(aURL, aPageDescriptor, aDocument);
> }

I'm inclined to say that we should remove ViewSourceOfURL and change it's one caller to call gViewSourceUtils.viewSource directly. I guess you left it in case extensions are using it?

>Index: toolkit/components/console/content/console.xul

>+  <script type="application/x-javascript" src="chrome://global/content/contentAreaUtils.js"/>
>+  <script type="application/x-javascript" src="chrome://global/content/viewSourceUtils.js"/>

It would be nice to try and avoid having to include contentAreaUtils, if possible, by removing viewSourceUtils.js's dependency on it. That can be a new bug, if you agree.

>Index: toolkit/components/viewsource/content/viewSourceUtils.js

>       // we failed loading it with the external editor.
>+      dump(ex + "\n");

Please use Components.utils.reportError() instead (same with the other occurrence in this patch).

>+          // include line number in the call to the editor if we've been passed 
>+          // a line number and the line number arg pref is set
>+          var lineNumberArg;
>+          var goToLine = false;
>+          if (this.data.lineNumber) {
>+            var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                                  .getService(Components.interfaces.nsIPrefBranch);
>+            lineNumberArg = prefs.getCharPref("view_source.editor.line_number_arg");
>+            goToLine = lineNumberArg.indexOf("%s") > -1;
>+          }
>+          if (goToLine) {
>+            this.editor.run(false, [lineNumberArg.replace("%s", this.data.lineNumber), this.file.path], 2);
>+          } else {
>+            this.editor.run(false, [this.file.path], 1);
>+          }

Get rid of the unnecessary brackets (and wrap at 80 chars, I'd say, but this file already goes over).

Comment 11

[Simon Bünzli]

Attachment: unbitrotted and nits fixed

Trying to get this issue fixed for Firefox 3, if you don't mind, Jason...

(In reply to comment #10)
> I'm inclined to say that we should remove ViewSourceOfURL

Agreed and done. Should any extension have relied upon ViewSourceOfURL, it'll so note the new feature(s) we now offer.

> It would be nice to try and avoid having to include contentAreaUtils

Unfortunately we need major parts of that file for one method. Still, we now include it ourselves where we need it (without polluting the global namespace).

(In reply to comment #8)
> we should probably consider making it more generic

Done. The only potential issue is that we now have to always pass a line number, even when we've got none. Passing 0 for the time being which hopefully means "beginning of the file" or "invalid/default value" for all editors (passing 1 could cause more sophisticated editors to highlight the first line when we don't necessarily want that to happen).

Comment 12

[Simon Bünzli]

Never mind. Neither blocking nor wanted-1.9+, thus hardly a chance of getting review in time for Firefox 3.0.

Comment 13

[Simon Bünzli]

Attachment: unbitrotted

Giving this one another try for Firefox 3.1... The patch hasn't really changed (except for some unbitrotting), so comment #11 still applies.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 335748 [details] [diff] [review]
unbitrotted

Sorry for the delay, bug 454022 was preventing my testing.

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/348b52c4e1d6