The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9.1b1

Status

()

Toolkit
Preferences
--
enhancement
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Simon Bünzli, Assigned: Simon Bünzli)

Tracking

unspecified
mozilla1.9.1b1
Points:
---
Bug Flags:
blocking-firefox2 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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

11 years ago
Created attachment 216682 [details] [diff] [review]
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.
Assignee: nobody → jason_barnabe
Status: NEW → ASSIGNED
Attachment #216682 - Flags: first-review?

Updated

11 years ago
Attachment #216682 - Flags: first-review? → first-review?(mconnor)
(Assignee)

Comment 2

11 years ago
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

11 years ago
(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.
(Assignee)

Comment 4

11 years ago
(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

11 years ago
Created attachment 216804 [details] [diff] [review]
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.
Attachment #216682 - Attachment is obsolete: true
Attachment #216804 - Flags: first-review?
Attachment #216682 - Flags: first-review?(mconnor)

Updated

11 years ago
Attachment #216804 - Flags: first-review? → first-review?(mconnor)

Comment 6

11 years ago
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. 
(Assignee)

Updated

11 years ago
Flags: blocking-firefox2?
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.
Flags: blocking-firefox2? → blocking-firefox2-

Updated

11 years ago
Attachment #216804 - Flags: first-review?(mconnor) → first-review?(gavin.sharp)
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

11 years ago
(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 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).
Attachment #216804 - Flags: first-review?(gavin.sharp) → first-review-
(Assignee)

Comment 11

10 years ago
Created attachment 275436 [details] [diff] [review]
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).
Assignee: jason_barnabe → zeniko
Attachment #216804 - Attachment is obsolete: true
Attachment #275436 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
Attachment #275436 - Flags: review?(gavin.sharp)
(Assignee)

Comment 12

10 years ago
Never mind. Neither blocking nor wanted-1.9+, thus hardly a chance of getting review in time for Firefox 3.0.
Assignee: zeniko → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 13

9 years ago
Created attachment 335748 [details] [diff] [review]
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.
Attachment #275436 - Attachment is obsolete: true
Attachment #335748 - Flags: review?(gavin.sharp)
Attachment #335748 - Flags: review?(gavin.sharp) → review+
Comment on attachment 335748 [details] [diff] [review]
unbitrotted

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

Updated

9 years ago
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Flags: in-litmus?
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/348b52c4e1d6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1

Updated

9 years ago
Depends on: 454297
(Assignee)

Updated

8 years ago
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.