Closed Bug 15364 Opened 25 years ago Closed 21 years ago

Add line numbers to View Source

Categories

(Core Graveyard :: View Source, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: marshall, Assigned: bugzilla.mozilla.org-3)

References

Details

(Keywords: helpwanted, Whiteboard: [se-radar])

Attachments

(3 files, 8 obsolete files)

The View Source window could really use line numbers, which matched up with the
JavaScript Debugger output.
Component: Browser-General → XPApps
Naturally (to certain ways of thinking), this ability should be a user-defined
preference.
[Setting component to XPApps, as View Source issues generally are categorized]
I would prefer it as a "View" menu item that defaulted to off.
Assigning this to rickg, since he wrote view-source and everybody therefore
annoys him about it... (but a component change isn't really appropriate).
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → REMIND
I assume you mean you want line number to appear on the left edge of the
display? If so, I'll consider it post beta (with a few other enhancements).
Marking remind.
Status: RESOLVED → VERIFIED
QA Contact: leger → elig
Verified REMIND.
It is post beta, thus reopening. Targetting for feature complete milestone M16.
Status: VERIFIED → REOPENED
Resolution: REMIND → ---
Target Milestone: --- → M16
This is NOT a committed feature. Please don't reopen this again. If I have the 
time to add this, I will, but I wouldn't count on it for 6.0.
Status: REOPENED → RESOLVED
Closed: 25 years ago24 years ago
Resolution: --- → REMIND
Sorry Rick -- I was just reopening it based on your comment "I'll consider it 
post beta (with a few other enhancements)". 

VERIFIED remind.
Status: RESOLVED → VERIFIED
*** Bug 70868 has been marked as a duplicate of this bug. ***
Status: VERIFIED → REOPENED
Keywords: mozilla1.1
OS: Windows NT → All
Hardware: PC → All
Resolution: REMIND → ---
Target Milestone: M16 → ---
Adding keywords, platform to match duplicate bug.
*** Bug 88539 has been marked as a duplicate of this bug. ***
blake/doron, would either of you like to add this your list? since rickg isn't
here anymore...
QA Contact: elig → sairuh
sure, i'll take this, but future.
Assignee: rickg → doronr
Status: REOPENED → NEW
Keywords: mozilla1.1
Target Milestone: --- → Future
Blocks: 92121
Blocks: 79612
No longer blocks: 92121
mass moving open bugs pertaining to view source to pmac@netscape.com as qa contact.

to find all bugspam pertaining to this, set your search string to
"ItsSharkeysNight".
QA Contact: sairuh → pmac
Component: XP Apps → ViewSource
*** Bug 140300 has been marked as a duplicate of this bug. ***
*** Bug 140305 has been marked as a duplicate of this bug. ***
Please make it so there is a choice between line numbers in the status bar and
next to the text. I like the former, others like the latter.
*** Bug 99211 has been marked as a duplicate of this bug. ***
I can't do that because you have not explained how this "line numbers in the
status bar" thing is supposed to work.
I would assume that "line numbers in the status bar" would work like MS Word or
most text editors, where the cursor position is shown in the status bar (below
the editing window).  In text editors, sometimes it is displayed as "734:16" --
line 734, column 16.  In Word, it's displayed as "Lin 17 Col 1".

However, I would vote for the lines to be displayed in a gutter along the left
hand side of the source window.  Lines that are wrapped (view | wrap long lines)
might be denoted with a small wrapping arrow icon.
Since there is no cursor, displaying its nonexistent position would make little
sense...
HA! So then the option of displaying the line numbers to the left of the source
window might be a better option! =)
For displaying on the left column, make that column shaded, and for wrapped
lines, just leave it empty:

1   Line 1  
2   Line 2
3   Line 3
4   Line 4 - This line has been
    wrapped
5   Line 5
etc Line 6

But, I don't like that left-hand column option, because it is aesthetically
unpleasing. We can easily make a dummy cursor that someone could place on the
document. Anyway, that is a good first step towards making the view-source
editable (bug 143409). 
Besides, if its displayed in the status bar, you can see both a line and col
number such as the following:

Line 1/151 : Col 15/22
I think displaying a cursor in the source window and showing
the line number in the status bar has a lot of advantages:

- it doesnt take up so much space
- you dont have to deal with the question of how many digits to display
- it is easier to mark source for copying without the line numbers
- with the cursor present it would probably also be easy to allow for marking
  of arbitrary text portion using the keyboard 
- i would assume it is more easy to implement and a bit more efficient too

This should be augmented by an "View->goto line number" option which makes
that line get displayed in the vertical center of the window and 
the cursor jump to the beginning of that line.
You can already enable the cursor in the view source window (hit f7).
It may make some sense to put the window into "browse with caret" mode by
default, I guess...

As a note, making view-source editable is a waste of everyone's time, as I
already said in bug 143409.  We should just use an editor as the source viewer
if you want that.
I don't agree with that until composer doesn't mangle pages without a workaround
and loads as fast as view-source (which notepad can't even beat).

view-source looks a lot better than composer's plaintext mode, and all the other
stuff composer comes with isn't necessary if you want to change a word.

I've also mentioned that no editor that comes standard on windows has line
numbers which i consider a requirement for proper text editing.
I would be very helpfull while developing.
The perfect companion for the javasript debugger
*** Bug 170565 has been marked as a duplicate of this bug. ***
How about just adding a GOTO LINE for now...it is very annoying to have the
javascript console mention a line and then not being able to get to that line in
the view source without saving the page and opening it with a decent text editor
(such as vim/emacs)
That's bug 104383.  Feel free to fix it.  There is a patch there, but it has a
serious issue.
*** Bug 188861 has been marked as a duplicate of this bug. ***
Is it so hard to implement line numbering ?
I think it is an important feature since JAVASCRIPT reports the line number but
you can't locate it with mozilla !!!
Is it so hard to read this bug and to realize that YES IT IS HARD TO IMPLEMENT
LINE NUMBERING WHEN LINES WRAP IN VIEW SOURCE?
It is hard to believe that a feature request made in 1999 as basic and useful to
scripting developers as this still hasn't been created in 2003.  I mean maybe
its not trivial but this is a basic feature for most browsers...
Maybe line numbers could be on only when word wrap is off?
Keywords: helpwanted
bz: you had a patch that added <a name="">'s for jumping to a specifict line
number, could we not use that for line numbers as well?  I know its a perf hit,
but who cares :)
Hey, if you can retrofit it to display line numbers, go for it.  ;)

Note that the line numbers will not match those in the JS console in some cases
(I filed a but on that at some point many moons ago; the console gets the line
numbers wrong).
I was thinking of ways to speed up highlighting to cover this, and was
wondering: would switching from <span class=""> to <ownmarkup> be any faster? 
It would cause a bit less dom building right?
Just an idea, but a setting on which program to start for viewing the code could also be 
a simple shortcut. Personaly I wouldn't mind if the source was shown in 
kate/quanta/med/ultraedit as long as it's not notepad..  And those all have regexp for 
searching, make cut and past easy, store versions to compare later AND have 
linenumbers..  
The javascript error could start the requested program with the linenumber somewhere 
on the commandline.. and there you go..  
Yeah, we could do an external program.  I've been tempted.

Doron, I doubt using self-hacked markup would be any faster...  You could try if
you want, though.  It should be straightforward to do.
Boris:

>Is it so hard to read this bug and to realize that YES IT IS HARD TO IMPLEMENT
>LINE NUMBERING WHEN LINES WRAP IN VIEW SOURCE?

Can you explain why this is? Does it have to do with the XUL item that contains
the text not allowing you to know when you have soft line wraps?

Why don't we do this the correct way and modify the XUL item to allow it to be 
able to be queried in the manner we need? What item is it btw?
If you can figure out a way to do that, feel free.  I can't figure out a way to
do it without hurting layout performance in general.

The "item" is a <browser/>, as you could tell yourself by looking at
viewSource.xul for oh, 5 seconds.
I know, but my previous comment #44 was intended to recieve a more detailed 
information (at least the element name) than "Yes" and I was disappointed with 
the lack of detail in the response. Since you had already looked at it, and I 
am not planning on working on this at this time, I was hoping you could 
provide that information for someone else who might be interested. Since I 
wasn't planning on working on it, there is no incentive for me to duplicate 
your efforts.
Anyone who would be interested would find out what they needed to know in those
same 5 seconds.  Please stop wasting people's time, Brian.
Whiteboard: [se-radar]
What you refer to as wasting people's time is called DOCUMENTATION. Hmmm... 
Interesting concept. Just because you can find it in 5 minutes doesn't mean 
someone who has only fixed one bug and wants to give this a stab can. I am too 
busy, as you probably are to fix this, so the whole point is making it so 
someone who has the interest and the time can.
Me saying something in a bug is not "documentation".

Anyone who actually tried to give this bug a shot would see what widget it is
immediately.  It's not only I who can read XUL.

I'm not "too busy" to fix this.  I _tried_ to fix this, and spent a fairly large
amount of time on that, as you may have noticed if you looked at the dep tree of
this bug or read anything that's been said here.  I've failed. I do not believe
there is a reasonable way to fix it in the current view-source architecture.

Doesn't matter anyway, since I hope to remove our internal view-source entirely
one of these days....
I will give $9.00 US to whomever adds this functionality to Mozilla. (Must have
a paypal account to receive funds.)
Make that $50 US and you have a deal. ;-)
I'll add $10, also via Paypal with the exemption of Mr. Zbarsky (no offense, 
just don't like your attitude & hey, you've tried already :)
C'mon guys, a feature like "Open with" would be much easier, since there will be
no development efford and the external editor is a user's choice.
Maybe you could setup the external editor at Edit/Preferences...
>no development efford

that feature would have to be implemented, which is a development effort, and
PLEASE TAKE THAT TO THE APPROPRIATE BUG, or even better to /dev/null, unless you
provide a patch.
I don't think that an external editor is an appropriate substitute for a line
numbered viewsource.  

Most of the time, I already have the source open in an editor.  I don't need
another instance of the editor opening with yet another copy of the source nor
do I want my current instance of the editor to prompt me with "This document is
already open.  Do you wish to revert to the saved original?"  Nine times out of
ten, all I want is to see a particular error in context.  A line numbered
viewsource would help immensely.  Better yet, I love to see links in every error
console that opened viewsource with the line in question highlighted.

Being able to view source in an external editor is one of IE's better features
but having no choice but to view source in an external editor is one of IE's
worst.  That said, I rather suffer the latter to enjoy the former.

Any coders out there want to name a price??
When you are developing dynamic pages, the source is not the same as the browser
page source. So, you can have an editor for the source and another for the
browser page source. I think an "Open With" would be much easier (sorry for the
"NO DEVELOPMENT EFFORT") than developing line numbering. It does not mean that
it is the only solution. Why can't we have both ?
Line numbering is is urgent, so I think "Open With" would be faster to develop.
I agree with #57. We can have both. I also feel that the status of view source
issues is going to change significantly with the move to a fully GRE-based
Mozilla. View Source, I imagine, is most likely going to become a separate
application.
Finaly bug 104383 landed. I took first look into this one, and compared it with
functions we have in 104383 patch.
Correct me if I'm wrong, but view-source is totally messed. 

view-source should be, primary, a way to show each line of source code of page.
So we should take a downloaded code of a web page and show it in XUL by creating
xul label for each line. If we'll put it into grid, we can easly make column for
line-number.

Second thing (in rewritting view-source) should be higlight syntax, but i would
use nested labels inside line-label with CSS classes instead of PRE/SPAN.

If i'm right after this we should have something similar to our actual
view-source, but with easy access to GoToLine and line numbering, and fully
scriptable, no XPCOM needed. 

If there are no big mistakes in my thinking, than it should be easy. But it
looks too beautifull for me. Anybody could correct me?
*** Bug 208076 has been marked as a duplicate of this bug. ***
*** Bug 208076 has been marked as a duplicate of this bug. ***
Additionally to the vertical ruler showing line numbers, it will be usefull to
have horizontal one on the top of a source window, and as long as viewsource use
monespaced font we should possibly separate a source into columns of 10 symbols
(for example) with shaded lines.
Comment #59:
> show it in XUL by creating xul label for each line. If we'll put it into grid,
> we can easly make column for line-number.

It will slow view-source even more. (The line numbering is not that much an
issue in venkman because JS scripts are usually short.)

Going back to the earlier suggestion in comment 20, sounds like a compromise now
that the caret shows up in view-source following bug 104383. But it only shows
up after a first use of GotoLine. It can be changed so that the caret always
shows up, and with that, tracking the caret and displaying e.g. "Line 17 Column
1" doesn't seem that bad and would cater for wrapped lines as well.
This patch adds the current line and column number to the status bar of the
view-souce window as described in comment 20.

When the window is opened, no caret is visible and the status bar is empty. If
you click somewhere in the document, the caret becomes visible and the line and
column number are displayed in the status bar. If you are in caret browsing
mode and you move the caret using the keyboard, the status bar is updated.

Currently the caret can only me moved around in caret browsing mode, though it
may be visible even if caret browsing is off. Perhaps caret browsing should
always be on in view-source as suggested in comment 27 (I don't know how to
turn this on for the view-source window only - it seems that caret browsing is
determined by an application-wide pref).

To know when the caret has been moved I listen for the onmouseup and keypress
events. Is there a better way like e.g. a selectionchange event?

There are some oddities with empty lines in caret browsing mode. Steps to
reproduce:
1. View source of this very Bugzilla page. Here lines 1 and 3 are non-empty and
line 2 is empty.
2. Press F7 to enable caret browsing 
3. Press home to move the caret to line 1 col 1; selection.focusOffset is 0.
4. Press down arrow; the caret moves to line 3 col 1 and selection.focusOffset
is 27.
5. Press the left arrow; the caret moves to the end of line 1 - but the status
bar says "line 2 col 1", and selection.focusOffset is 26.
6. Press left arrow.
7. Press right arrow - the status bar now says "line 1 col 25" and
selection.focusOffset is 25.

In step 5 I would expect selection.focusOffset to be 25 (not 26) and the status
bar to display "line 1 col 26". I assume this is a bug/weirdness in caret
browsing - right?
> Currently the caret can only me moved around in caret browsing mode, though
it
> may be visible even if caret browsing is off. Perhaps caret browsing should
> always be on in view-source as suggested in comment 27 (I don't know how to
> turn this on for the view-source window only - it seems that caret browsing
is
> determined by an application-wide pref).

Technically, there can be a per-window caret browsing. FYI, I have attached a
simple patch which shows how it can be done. The patch causes the "browse with
caret movements" on a window for which the caret is enabled.

Another way is to re-implement those movements. See the body of
|DoCommandBrowseWithCaretOn()| in the code affected by the patch. See also
other implementations of |nsIControllerCommand| (e.g., ComposerCommands.js,
typeaheadfind). But it is getting out of hands... My feeling is that if there
can't be a simple patch, then it is probably not worth pursuing.

> To know when the caret has been moved I listen for the onmouseup and keypress

> events. Is there a better way like e.g. a selectionchange event?

There is. But then you will have to implement the corresponding API. It is
unlikely to be two-liners as you have now... So, why bother.

> There are some oddities with empty lines in caret browsing mode. Steps to
> reproduce:

Could be some oddities with your patch too. E.g., is 'keypress' fired after or
before? Could be that things are out of sync if the order isn't what you
expect.

re: your patch in attachment 125640 [details] [diff] [review]
1) looks like there are redundant code fragments. could be factored? e.g., 
gotoPosition(line, selection)? where selection can be null, meaning to find-it
and gotoline?
2) any reason to turn the status to '' in gotoline?
> Technically, there can be a per-window caret browsing. FYI, I have attached a
> simple patch which shows how it can be done. The patch causes the "browse with
> caret movements" on a window for which the caret is enabled.

So what are you saying - is that patch enough, or is it just an example (on the
face of it it sounds reasonable that whenever there is a caret, it should be
possible to move it using the keyboard, but I have not given it much thought)? I
do not code C++, so unless I can do it in XUL, somebody else needs to step in.


> Another way is to re-implement those movements. See the body of
> |DoCommandBrowseWithCaretOn()| in the code affected by the patch. See also
> other implementations of |nsIControllerCommand| (e.g., ComposerCommands.js,
> typeaheadfind). But it is getting out of hands...

I agree.


> Could be some oddities with your patch too. E.g., is 'keypress' fired after or
> before? Could be that things are out of sync if the order isn't what you
> expect.

The behaviour is the same, even if I do not use events but instead call
updateStatusBar() using window.setInterval().

I does not seem right that focusOffset is 26 in step 5, and then after pressing
"left arrow, right arrow" focusOffset is 25 in step 7. Usually I would expect
left arrow and right arrow to "cancel" each other (unless the caret is at the
beginning or end of the document).


> re: your patch in attachment 125640 [details] [diff] [review]
> 1) looks like there are redundant code fragments. could be factored? e.g., 
> gotoPosition(line, selection)? where selection can be null, meaning to find-it
> and gotoline?

Done.


> 2) any reason to turn the status to '' in gotoline?

Currently the status bar is blank when there is text selected. In this state the
caret is not visible, so I think it makes little sense to display of just one of
the end-points of the selection. One could argue then that both end-points
should be displayed in the status bar, but I do not know if that is useful to
anybody.
> Currently the status bar is blank when there is text selected. In this state the
> caret is not visible, so I think it makes little sense to display of just one of
> the end-points of the selection.

Agreed.

> One could argue then that both end-points should be displayed in the status
> bar, but I do not know if that is useful to anybody.

The end points: maybe not.
But a line count or character count of the selection would be quite useful.
-> re-assigning to christian since he is working on it.

> So what are you saying - is that patch enough, or is it just an example (on the
> face of it it sounds reasonable that whenever there is a caret, it should be
> possible to move it using the keyboard, but I have not given it much thought)? I
> do not code C++, so unless I can do it in XUL, somebody else needs to step in.

It was just a demo. Such a change of behavior would need other eyes (especially
from the editor folks). Suggesting to leave this extra aside and file another
bug for per-window caret browsing. If that bug is fixed, then viewsource will
pickup. Otherwise what you have now is okay IMO, and eager developers can use F7
in the meantime.

> I does not seem right that focusOffset is 26 in step 5, and then after pressing
> "left arrow, right arrow" focusOffset is 25 in step 7. Usually I would expect
> left arrow and right arrow to "cancel" each other (unless the caret is at the
> beginning or end of the document).

I suspected you was querying the old selection state (the
'before-selection-changed') -- due to 'keypress'. I applied your patch and
changed the listener to 'keyup' (which calls back in the
'after-selection-changed'). With that, the oddities went away.

> Currently the status bar is blank when there is text selected. In this state the
> caret is not visible, so I think it makes little sense to display of just one of
> the end-points of the selection. One could argue then that both end-points
> should be displayed in the status bar, but I do not know if that is useful to
> anybody.

OK, I see. Up to you to experiment and see whether to show the line number or
leave it blank if you want.
Assignee: doron → bugzilla
if the parser tells the viewsource window homany lines there are, we could impl
it in nice looking zool
> OK, I see. Up to you to experiment and see whether to show the line number or
> leave it blank if you want.

Hmm, it just occured to me that showing the line number is an ambiguous issue
for a (manual) selection across multiple lines... Hence, I will simply go for a
blank status as you did.
just noted a bug with your patch (which is now applied in my tree):
1) make a (manual) selection in the viewsource window
2) click anywhere _in_ the selected area
   -> actual results: caret is shown & selection is cleared,
                      but blank status, line & col don't show up.

      expected results: line & col of the caret position should show up on
                        the status bar.
Attached patch Updated patch v. 0.2 (obsolete) — Splinter Review
The line counting code has been moved to a new function, countLines().

When there is a non-empty selection, the number of characters selected is
displayed as suggested in comment 67. I use
selection.getRangeAt(0).toString().length, because selection.toString().length
seems to ignore blanks (bug 164448 and bug 183496).

I don't think that the number of characters selected should be displayed when
the selection is made by goToLine(), so I made this clear the status bar.
However, when you invoke goToLine() and use Enter to submit the line number
prompt, the keyup event for the Enter keypress is sent to the view-source
window (I think this is bug 112298) and thus invokes updateStatusBar(), and the
number of characters selected is displayed anyway. This bug is cosmetical, so I
think it is reasonable to accept it until bug 112298 has been solved.

With this patch I still see the oddities with empty lines, even if I call
updateStatusBar() from onkeyup or use window.setInterval() instead of events.
However, I think this is problem with caret browsing and not this patch. I will
investigate it further and possibly file a seperate bug.

The bug described in comment 71 has been fixed. I now listen for keypress,
keyup, click and mouseup. I think this catches everything.
Attachment #125640 - Attachment is obsolete: true
Don't know what has changed in the tree w.r.t. security checks, but I had to do
the following change to try your patch:

- var treewalker = document.createTreeWalker(pre, NodeFilter.SHOW_TEXT, null,
false);
+ var treewalker = 
window._content.document.createTreeWalker(pre, NodeFilter.SHOW_TEXT, null, false);

With that out of the way, still an oddity:
1) visit mozilla.org & view-source
2) activate caret browsing with F7, and press Home 
   -> status: line 1, col 1
3) press down arrow 3 times
   -> status: line 5, col 1
4a) press up arrow
   -> bug... status: line 4, col 1
      expected: status: line 3, col 7

Looks like the "interline" again. 

4b) Interesting note: the status is correct if you instead use the 'up arrow' in
 step 4a, no matter the _column_ where the caret is on line 5.
> 4a) press up arrow
shoud read "left arrow"
When did the treewalker stuff stop working?  Could it maybe be related to bug
209701?
Attached patch Updated patch v. 0.3 (obsolete) — Splinter Review
> Don't know what has changed in the tree w.r.t. security checks, but I had to
> do the following change to try your patch:

>- var treewalker = document.createTreeWalker( [...]
>+ var treewalker = window._content.document.createTreeWalker( [...]

It should have been like that in the first place, so the reason for the
regression in your tree is of pure academic interest to this bug :)

The cause of the oddity is that the cursor may be displayed in the same visible
position for two different character offsets. I do not understand why this is
the case or why nsISelectionPrivate.interlinePosition is even needed, but now I
made a "workaround". With that I do not see any more oddities.
Attachment #125952 - Attachment is obsolete: true
there is a remaining trouble with the value of the column:
In 4a) press left arrow
   -> bug... status: line 3, col 3
      expected: status: line 3, col 7

Attached patch Patch v. 0.4 (obsolete) — Splinter Review
I am afraid that I did not test the last patch very thoroughly. Whether syntax
highlighting is on or off has a big impact on the code path when calculating
the column number.

I think I have now got a better understanding of the interline issue. I now no
longer try to detect empty lines; instead I use
nsISelectionPrivate.interlinePosition to detect when the caret is in the tricky
position after the \n of a line.

I hope this irons out the remaining oddities.
Attachment #126056 - Attachment is obsolete: true
Good job christian, big tick from me. [It reminds me of layout, where it is such
inevitable edge cases that add up and turn the code into a complex work of art.]

Still another (visual) oddity, though...
1) visit mozilla.org & view-source
2a) select the line with <html> entirely, by moving the mouse downward
    -> status: 7 characters selected
2b) select the line with <head> entirely, by moving the mouse downward
    -> status: 8 characters selected

bug: '<html>' and '<head>' (should) have the same length... of 6...
The bug doesn't appear if the selection is "suitably" made rightward. Seeems the
difference is just that the "\n" are included sometimes. 

Possible fixes:
1. explicitly remove "\n" before getting the length
2. or, use other representations such as "Line x, Col a - Line y, Col b",
   "Line x, Col a - b" for the status of a selection.

#1 is easiest to implement, while #2 is applicable to gotoline but will be too
much work not worth the hassle (might perhaps be a RFE if it comes back as a
feedback when the code goes in the field).

A nit in your patch: |countLines| doesn't seem a good name as it gives the
impression that it returns a counter. What about findLocation()?

> Good job christian, big tick from me.

Thanks :-)


> bug: '<html>' and '<head>' (should) have the same length... of 6...

I disagree. In the described case, the length of the selection is indeed 7 and 8
respectively. This can be verified by copying the selection and pasting it into
a text editor. I do not think that we should display 6 if CTRL+C copies more
than 6 characters to the clipboard.

Also, I think it makes sense to make a distinction between a selection with and
without the trailing \n. A user can currently choose whether he wants to copy
the trailing \n or not, depending on how he makes the selection (like in a text
editor).

It is a problem, though, that the selection of the trailing \n is not visible,
but that is bug 156175.

However, we could implement your suggestion #2 in addition to the character
count, e.g. "Line x, Col a - Line y, Col b (n characters)". I think this will
give consistent and understandable results. In this case I think we should
always display the first part of the selection first (i.e. y > x || y == x && 
b > a) instead of displaying offsetNode first and focusNode second.


> |countLines| doesn't seem a good name as it gives the impression that it
> returns a counter. What about findLocation()?

Yes, that is probably better.
Status: NEW → ASSIGNED
> Also, I think it makes sense to make a distinction between a selection with
and
> without the trailing \n. A user can currently choose whether he wants to copy

> the trailing \n or not, depending on how he makes the selection (like in a
text
> editor).

That's right. But the 6 vs 7 vs 8 is going to be odd... It doesn't matter that
we agree or not... Wearing my end-user hat, it already seemed odd to me even
though I understood the "\n" issue. It is going to be even more confusing to a
clueless end-user. But showing th cursor might give some clues to the user (see
below).

> It is a problem, though, that the selection of the trailing \n is not
visible,
> but that is bug 156175.

It goes back to that, but that won't solve the problem. Assuming the full blank
line is selected (as 156175 suggests), there still won't be a differentiation
between genuine whitespaces " " and "\n". I am attaching a patch that might
help a little bit here, re:the visibility of the caret position.

> However, we could implement your suggestion #2 in addition to the character
> count, e.g. "Line x, Col a - Line y, Col b (n characters)". I think this will

> give consistent and understandable results. In this case I think we should
> always display the first part of the selection first (i.e. y > x || y == x &&

> b > a) instead of displaying offsetNode first and focusNode second.

Since you are keen with this route (which amounts to two calls of your function
with the selection.anchorNode & selection.focusNode), see the little extra
patch for the back-end will show the caret even when there is a selection. It
makes things look nicer. For this work, you just have to add:
  selCon.setCaretEnabled(true);
+ selCon.setCaretVisibilityDuringSelection(true);

You also need to move the controller stuff on top of updateStatusBar.

Another improvement is to swap the anchorNode and focusNode in gotoline so that
the caret (that is now shown) blinks at the start of the line (i.e., as if the
selection was made backward -- in which case you might have to sync
scrollSelectionIntoView).
Comment on attachment 126333 [details] [diff] [review]
[back-end] helper patch to show the caret even with a selection

asking r/sr for this patch which just exposes an existing useful API to
scripts. This is actually the default behavior in Emacs or ultraedit.

With the one-liner in nsXPLookAndFeel.cpp, people can set the pref
"ui.caretVisibleWithSelection", to 1, which adds a nice combination to the F7
browse with caret mode.

[off-topic: looks like about:config might do well to dig and expose those
obscure nsILookAndFeel prefs too. It is missing a lot of prefs and could also
try looking prefs based on callbacks that have been registered.]
Attachment #126333 - Flags: superreview?(bzbarsky)
Attachment #126333 - Flags: review?(bzbarsky)
Comment on attachment 126333 [details] [diff] [review]
[back-end] helper patch to show the caret even with a selection

Why not just QI the presshell to nsISelectionController and call
setCaretVisibilityDuringSelection on it in
nsTextInputSelectionImpl::SetCaretVisibilityDuringSelection ?

sr=me with that change (or a good reason not to make it), but I don't know this
code well enough to do the r=; Simon may be a better person for that...
Attachment #126333 - Flags: superreview?(bzbarsky)
Attachment #126333 - Flags: superreview+
Attachment #126333 - Flags: review?(sfraser)
Attachment #126333 - Flags: review?(bzbarsky)
> Why not just QI the presshell to nsISelectionController

I wondered the same. But based on the other overloaded methods in
nsTextControlFrame, it appears to differentiate whether it has its own selection.
Ah, ok.  That sounds like a good reason.  ;)
Attachment #126333 - Flags: review?(sfraser) → review+
Comment on attachment 126333 [details] [diff] [review]
[back-end] helper patch to show the caret even with a selection

Checked in. Taking it off the radar.
Attachment #126333 - Attachment is obsolete: true
BTW, since attachment 126333 [details] [diff] [review] now makes the caret to always appear, you can just
keep your default patch with simply "Line x, Col y" -- which is unambiguous with
the blinking caret, and simply forget about the non-essential extras of showing
the number of character that are selected -- which was brought in because the
caret wasn't there, but creates oddities.
Actually, I brought up the selected char count because I find this very useful.
It's just a nice-to-have but no real requirement...
Attached patch Patch v. 0.5 (obsolete) — Splinter Review
With selCon.setCaretVisibilityDuringSelection() I now always display the
position of the caret.

I agree that displaying the number of characters selected and/or the other
endpoint of the selection would be nice, but it is getting pretty complex, so
for now I think we should settle with the simple behaviour.
Attachment #126191 - Attachment is obsolete: true
Care to add this to your tree as well? It is a diff against your version. I
added the status [line,1] for gotoline as well, with the caret blinking at the
beginning of the line in the case. 

There was another peculiar edge case with gotoline 70 on mozilla.org. The
scroll didn't bring line 70 into view (this bug is visible in Nestcape 7.1
BTW). This supplement seems to fix that.

There is another oddity but I am not sure if it is due to some instabilities on
the trunk. After repeated uses of the Ctrl+L #line in succession (about 3-4
times), pressing just the Ctrl key produces this exception on the JS Console:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "aStringsArray has no properties" {file:
"XSt
ringBundle" line: 26}]' when calling method:
[nsIDOMEventListener::handleEvent]"
  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" 
location:
 "<unknown>"  data: yes]
************************************************************
OK, I found the cause of the exception. The problem was that when
|findLocation| is called from |updateStatusBar| (as the Ctrl key is pressed),
it assumes that the selection.focusNode is a _text_ node. If the assumption is
broken, it can't find the location, and things go awry. However, this
assumption doesn't always hold with the supplementary patch where the selection
is tweaked to reposition the focusNode at the beginning of the line. Indeed
this tweak can position it in-between </span>|<span>. To avoid the trouble, I
just added an early return in |updateStatusBar| to not bother:

+  if (selection.focusNode.nodeType != Node.TEXT_NODE) {
+    return;
+  }
Attachment #126780 - Attachment is obsolete: true
Attachment #126816 - Attachment is obsolete: true
Comment on attachment 126868 [details] [diff] [review]
patch v0.6: christian's patch v0.5 + supplement

Patch seems okay to me now. Time to ask the usual culprits (neil & bz) for
r/sr.
Attachment #126868 - Flags: superreview?(bzbarsky)
Attachment #126868 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 126868 [details] [diff] [review]
patch v0.6: christian's patch v0.5 + supplement

>   openDialog("chrome://navigator/content/viewSource.xul",
>              "_blank",
>-             "scrollbars,resizable,chrome,dialog=no",
>+             "scrollbars,resizable,chrome,status,dialog=no",
Might be worth changing this to all,dialog=no (both times)

>+  getBrowser().addEventListener("click", updateStatusBar, true);
>+  getBrowser().addEventListener("mouseup", updateStatusBar, true);
>+  getBrowser().addEventListener("keypress", updateStatusBar, true);
>+  getBrowser().addEventListener("keyup", updateStatusBar, true);
var browser = getBrowser();? Do we even need to capture these events? Then you
could use inline event handlers. I see in utilityOverlay.xul there's a
commandupdater for the select event, but it only fires when a selection is
created or destroyed :-( Perhaps someone could write a caret event, but it
would probably be inefficient :-/

>+      selection.extend(node, offset);
Thus undoing the collapse earlier? Seems inefficient.

>+      node = node.nextSibling ? node.nextSibling : node.parentNode.nextSibling;
>+      selection.extend(node, 0);
Wouldn't it be better to tweak the range before selecting it?

>+      .QueryInterface(Components.interfaces.nsISelectionPrivate).interlinePosition;
Consider const nsISelectionPrivate =

>+    for (pre = node; pre.tagName != 'PRE'; pre = pre.parentNode);
Text nodes don't have a tagName. This generates a strict JavaScript warning;
you should enable them.

>+          // the very last line in the file (this is the only line that does 
No lines ending in spaces, please.

+	     result = new Array(curLine - 1, prevCol);
+	     result = new Array(curLine, curCol);
Please use result = [curline - 1, prevCol]; but see below.

+	   result = range;
Not sure what this is for given the code below.

>+  if (range && !result) {
>+    result = range;
>   }
>+  
>+  return result;
Could return result || range; here. Mind you it's a bit naughty returning
different types of objects - can you make result a parameter, pass in an empty
array, and set the elements as necessary (effectively making result an out
parameter) instead?
Attachment #126868 - Flags: review?(neil.parkwaycc.co.uk) → review-
>>+      selection.extend(node, offset);
> 
> Thus undoing the collapse earlier? Seems inefficient.
> 
>>+      node = node.nextSibling ? node.nextSibling : node.parentNode.nextSibling;
>>+      selection.extend(node, 0);
> 
> Wouldn't it be better to tweak the range before selecting it?

Nope (to both questions). The crux of these tweaks to emulate a _backwards_
selection with the effect of putting the focus (& caret) at the start of the
line. The range spec (and Gecko's implementation) explicitly enforces that "the
start position of a Range is guaranteed to never be after the end position
(...)". Meaning that to achieve the backwards selection, it is simpler to just
do it the way I did.
Attachment #126868 - Flags: superreview?(bzbarsky)
> Perhaps someone could write a caret event, but it would probably be
> inefficient :-/

I don't know, but it could is useful in many ways, including in
application-like web pages. IE has it - called "selectionchange".
Attachment #126868 - Attachment is obsolete: true
Comment on attachment 127117 [details] [diff] [review]
Patch v. 0.7 that addresses comment 93.

Requesting r/sr again, now that the issues in comment 93 (except the caret
event question) have been addressed.
Attachment #127117 - Flags: superreview?(bzbarsky)
Attachment #127117 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 127117 [details] [diff] [review]
Patch v. 0.7 that addresses comment 93.

Couple of nits:
node.nextSibling ? node.nextSibling : node.parentNode.nextSibling
could be
node.nextSibling || node.parentNode.nextSibling
in which case you could possibly inline it into the next line.
Isn't content preferred over _content? (I don't know why they both work).
I like your new out parameter.
I think node.localName will always work even for text nodes (apparently the
result is "#text").
onXXX="updateStatusBar()" should be "updateStatusBar();"
And I found out how to watch the selection: having QI'd to nsISelectionPrivate
you can addSelectionListener with a JS object with a method named
notifySelectionChanged(doc, sel, reason). You get a heck of alot of change
events while you're dragging, though, too many perhaps (they seem to be
duplicated?)
Attachment #127117 - Flags: review?(neil.parkwaycc.co.uk) → review+
>> To know when the caret has been moved I listen for the onmouseup and keypress
>> events. Is there a better way like e.g. a selectionchange event?
>
>There is. But then you will have to implement the corresponding API. It is
>unlikely to be two-liners as you have now... So, why bother.

Actually the big problem is not getting too many events. This is something I
hacked together (of course you remove the browser's event handlers too):

  content.getSelection().QueryInterface(nsISelectionPrivate)
         .addSelectionListener(selectionListener);
}

var selectionListener = {
  timeout: 0,
  notifySelectionChanged: function notifySelectionChanged(doc, sel, reason) {
    if (!this.timeout)
      this.timeout = setTimeout(updateStatusBar, 100);
  }
}

function updateStatusBar() {
  selectionListener.timeout = 0;
Comment on attachment 127117 [details] [diff] [review]
Patch v. 0.7 that addresses comment 93.

Looks good.
Attachment #127117 - Flags: superreview?(bzbarsky) → superreview+
Checked-in on behalf of Christian, with the switch to a selection listener as
suggested by the reviewer.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago21 years ago
Resolution: --- → FIXED
In Firebird, this only works in the Mozilla version of View Source entered via
the javascript console (see bug 207406). It doesn't work with the normal View
Source.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5a) Gecko/20030717 Mozilla
Firebird/0.6
Feel free to file a bug on firebird, since they saw fit to fork this file.
Bug 207656 cares about Firebird (goto line # and show line #). The guys are
working on a patch already.
Christian, apprently |prevPos| is set nowhere in your patch:

Error: prevPos is not defined
Source File: chrome://navigator/content/viewsource.js
Line: 468
Hmm, appearently something went wrong between versions 0.6 and 0.7.  This
little patch fixes it.
Comment on attachment 128369 [details] [diff] [review]
Fix for bug described in comment 104

Checked in.
*** Bug 237827 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
*** Bug 278436 has been marked as a duplicate of this bug. ***
Blocks: 239211
Product: SeaMonkey → Core Graveyard
It appears that line numbers are added for HTML view-source URLS, but not for javascript URLS. E.g.

view-source:https://www.mozilla.org/media/js/common-bundle.e03b092546f6.js

(That file is compressed, of course, so should show just a single line number. Just a demo, but the principle holds.)

FWIW Chrome does line numbers on JS files. They can be awfully useful, especially considering that logs and errors in the console both link out directly to view-source URLS. Please advise if I should open a new bug, or this is the place to discuss.
You need to log in before you can comment on or make changes to this bug.