Last Comment Bug 15364 - Add line numbers to View Source
: Add line numbers to View Source
Status: RESOLVED FIXED
[se-radar]
: helpwanted
Product: Core Graveyard
Classification: Graveyard
Component: View Source (show other bugs)
: Trunk
: All All
P3 enhancement with 5 votes (vote)
: Future
Assigned To: Christian Schmidt
: Patty Mac
:
Mentors:
: 70868 88539 140300 140305 170565 188861 208076 237827 (view as bug list)
Depends on:
Blocks: 79518 79612 104383 239211
  Show dependency treegraph
 
Reported: 1999-10-01 11:34 PDT by Neil Marshall
Modified: 2016-01-31 18:00 PST (History)
45 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch that adds line and column numbers to the status bar (5.78 KB, patch)
2003-06-14 08:51 PDT, Christian Schmidt
no flags Details | Diff | Splinter Review
per-window browse with caret (786 bytes, patch)
2003-06-15 23:06 PDT, rbs
no flags Details | Diff | Splinter Review
Updated patch v. 0.2 (9.85 KB, patch)
2003-06-18 14:27 PDT, Christian Schmidt
no flags Details | Diff | Splinter Review
Updated patch v. 0.3 (10.61 KB, patch)
2003-06-19 14:14 PDT, Christian Schmidt
no flags Details | Diff | Splinter Review
Patch v. 0.4 (10.84 KB, patch)
2003-06-21 10:22 PDT, Christian Schmidt
no flags Details | Diff | Splinter Review
[back-end] helper patch to show the caret even with a selection (4.68 KB, patch)
2003-06-23 19:00 PDT, rbs
sfraser_bugs: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Patch v. 0.5 (12.41 KB, patch)
2003-06-30 15:28 PDT, Christian Schmidt
no flags Details | Diff | Splinter Review
supplement to show the status for gotoline as well (2.38 KB, patch)
2003-07-01 04:47 PDT, rbs
no flags Details | Diff | Splinter Review
patch v0.6: christian's patch v0.5 + supplement (14.17 KB, patch)
2003-07-01 16:55 PDT, rbs
neil: review-
Details | Diff | Splinter Review
Patch v. 0.7 that addresses comment 93. (13.09 KB, patch)
2003-07-06 09:31 PDT, Christian Schmidt
neil: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Fix for bug described in comment 104 (592 bytes, patch)
2003-07-23 15:29 PDT, Christian Schmidt
no flags Details | Diff | Splinter Review

Description User image Neil Marshall 1999-10-01 11:34:02 PDT
The View Source window could really use line numbers, which matched up with the
JavaScript Debugger output.
Comment 1 User image Crysgem 1999-10-11 13:26:59 PDT
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]
Comment 2 User image Matthew Tuck [:CodeMachine] 1999-10-20 22:25:59 PDT
I would prefer it as a "View" menu item that defaulted to off.
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 1999-10-29 14:43:59 PDT
Assigning this to rickg, since he wrote view-source and everybody therefore
annoys him about it... (but a component change isn't really appropriate).
Comment 4 User image rickg 1999-11-02 01:15:59 PST
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.
Comment 5 User image Eli Goldberg 1999-11-19 13:31:59 PST
Verified REMIND.
Comment 6 User image Hixie (not reading bugmail) 2000-04-06 08:54:55 PDT
It is post beta, thus reopening. Targetting for feature complete milestone M16.
Comment 7 User image rickg 2000-04-06 15:13:21 PDT
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.
Comment 8 User image Hixie (not reading bugmail) 2000-04-07 07:50:16 PDT
Sorry Rick -- I was just reopening it based on your comment "I'll consider it 
post beta (with a few other enhancements)". 

VERIFIED remind.
Comment 9 User image Jesse Ruderman 2001-05-04 13:45:53 PDT
*** Bug 70868 has been marked as a duplicate of this bug. ***
Comment 10 User image Tim Powell 2001-05-05 10:13:45 PDT
Adding keywords, platform to match duplicate bug.
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2001-06-30 18:46:09 PDT
*** Bug 88539 has been marked as a duplicate of this bug. ***
Comment 12 User image sairuh (rarely reading bugmail) 2001-06-30 23:07:13 PDT
blake/doron, would either of you like to add this your list? since rickg isn't
here anymore...
Comment 13 User image Doron Rosenberg (IBM) 2001-07-01 00:03:13 PDT
sure, i'll take this, but future.
Comment 14 User image sairuh (rarely reading bugmail) 2002-01-24 14:48:04 PST
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".
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2002-04-26 07:16:29 PDT
*** Bug 140300 has been marked as a duplicate of this bug. ***
Comment 16 User image R.K.Aa. 2002-04-26 07:35:56 PDT
*** Bug 140305 has been marked as a duplicate of this bug. ***
Comment 17 User image Brian 'netdragon' Bober 2002-05-15 15:23:09 PDT
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.
Comment 18 User image HJ 2002-05-15 15:28:35 PDT
*** Bug 99211 has been marked as a duplicate of this bug. ***
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2002-05-15 20:10:20 PDT
I can't do that because you have not explained how this "line numbers in the
status bar" thing is supposed to work.
Comment 20 User image Mike Brittain 2002-05-18 15:33:10 PDT
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.
Comment 21 User image Boris Zbarsky [:bz] (still a bit busy) 2002-05-18 15:40:52 PDT
Since there is no cursor, displaying its nonexistent position would make little
sense...
Comment 22 User image Mike Brittain 2002-05-18 15:46:20 PDT
HA! So then the option of displaying the line numbers to the left of the source
window might be a better option! =)
Comment 23 User image Brian 'netdragon' Bober 2002-05-20 03:50:36 PDT
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). 
Comment 24 User image Brian 'netdragon' Bober 2002-05-20 03:52:35 PDT
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
Comment 25 User image johann.petrak@gmail.com 2002-05-20 05:19:49 PDT
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.
Comment 26 User image Hixie (not reading bugmail) 2002-05-20 08:52:05 PDT
You can already enable the cursor in the view source window (hit f7).
Comment 27 User image Boris Zbarsky [:bz] (still a bit busy) 2002-05-20 09:19:29 PDT
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.
Comment 28 User image Brian 'netdragon' Bober 2002-05-20 18:14:30 PDT
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.
Comment 29 User image Dario Alejandro Guzik 2002-09-13 09:19:11 PDT
I would be very helpfull while developing.
The perfect companion for the javasript debugger
Comment 30 User image timeless 2002-09-24 13:16:19 PDT
*** Bug 170565 has been marked as a duplicate of this bug. ***
Comment 31 User image Dan Allen 2002-10-16 11:35:44 PDT
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)
Comment 32 User image Boris Zbarsky [:bz] (still a bit busy) 2002-10-16 13:46:14 PDT
That's bug 104383.  Feel free to fix it.  There is a patch there, but it has a
serious issue.
Comment 33 User image Boris Zbarsky [:bz] (still a bit busy) 2003-01-13 00:37:33 PST
*** Bug 188861 has been marked as a duplicate of this bug. ***
Comment 34 User image Cristiano Duarte 2003-01-16 11:31:09 PST
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 !!!
Comment 35 User image Boris Zbarsky [:bz] (still a bit busy) 2003-01-16 11:35:03 PST
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?
Comment 36 User image nate parsons 2003-01-16 11:38:32 PST
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...
Comment 37 User image Stig Nygaard 2003-01-16 11:58:49 PST
Maybe line numbers could be on only when word wrap is off?
Comment 38 User image Doron Rosenberg (IBM) 2003-01-16 12:12:44 PST
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 :)
Comment 39 User image Boris Zbarsky [:bz] (still a bit busy) 2003-01-16 12:16:02 PST
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).
Comment 40 User image Doron Rosenberg (IBM) 2003-01-16 12:18:40 PST
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?
Comment 41 User image Gerhard Hoogterp 2003-01-16 12:32:05 PST
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..  
Comment 42 User image Boris Zbarsky [:bz] (still a bit busy) 2003-01-16 12:55:09 PST
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.
Comment 43 User image Brian 'netdragon' Bober 2003-01-20 16:33:22 PST
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?

Comment 44 User image Boris Zbarsky [:bz] (still a bit busy) 2003-01-21 05:41:56 PST
Yes.
Comment 45 User image Brian 'netdragon' Bober 2003-01-21 11:09:56 PST
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?
Comment 46 User image Boris Zbarsky [:bz] (still a bit busy) 2003-01-21 11:20:27 PST
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.
Comment 47 User image Brian 'netdragon' Bober 2003-01-21 11:51:48 PST
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.
Comment 48 User image Boris Zbarsky [:bz] (still a bit busy) 2003-01-21 11:58:26 PST
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.
Comment 49 User image Brian 'netdragon' Bober 2003-01-21 15:29:21 PST
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.
Comment 50 User image Boris Zbarsky [:bz] (still a bit busy) 2003-01-21 15:38:56 PST
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....
Comment 51 User image nate parsons 2003-02-25 13:28:44 PST
I will give $9.00 US to whomever adds this functionality to Mozilla. (Must have
a paypal account to receive funds.)
Comment 52 User image Brian 'netdragon' Bober 2003-02-25 15:19:10 PST
Make that $50 US and you have a deal. ;-)
Comment 53 User image wick 2003-02-26 08:03:56 PST
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 :)
Comment 54 User image Cristiano Duarte 2003-02-26 13:21:17 PST
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...
Comment 55 User image Christian :Biesinger (don't email me, ping me on IRC) 2003-02-26 13:41:20 PST
>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.
Comment 56 User image Ian Pottinger 2003-04-08 19:08:32 PDT
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??
Comment 57 User image Cristiano Duarte 2003-04-09 05:09:55 PDT
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.
Comment 58 User image Brian 'netdragon' Bober 2003-04-15 16:02:40 PDT
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.
Comment 59 User image Zibi Braniecki [:gandalf][:zibi] 2003-05-30 08:15:49 PDT
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?
Comment 60 User image Benedikt Kantus 2003-06-02 23:48:01 PDT
*** Bug 208076 has been marked as a duplicate of this bug. ***
Comment 61 User image Evgeniy Karyakin 2003-06-02 23:55:15 PDT
*** Bug 208076 has been marked as a duplicate of this bug. ***
Comment 62 User image Evgeniy Karyakin 2003-06-03 00:06:59 PDT
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 63 User image rbs 2003-06-13 13:07:55 PDT
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.
Comment 64 User image Christian Schmidt 2003-06-14 08:51:33 PDT
Created attachment 125640 [details] [diff] [review]
Patch that adds line and column numbers to the status bar

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?
Comment 65 User image rbs 2003-06-15 23:06:45 PDT
Created attachment 125730 [details] [diff] [review]
per-window browse with caret

> 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?
Comment 66 User image Christian Schmidt 2003-06-16 15:15:51 PDT
> 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.
Comment 67 User image Karsten Düsterloh 2003-06-16 15:50:12 PDT
> 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.
Comment 68 User image rbs 2003-06-16 16:56:58 PDT
-> 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.
Comment 69 User image Doron Rosenberg (IBM) 2003-06-16 16:59:55 PDT
if the parser tells the viewsource window homany lines there are, we could impl
it in nice looking zool
Comment 70 User image rbs 2003-06-16 18:07:56 PDT
> 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.
Comment 71 User image rbs 2003-06-16 18:14:09 PDT
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.
Comment 72 User image Christian Schmidt 2003-06-18 14:27:30 PDT
Created attachment 125952 [details] [diff] [review]
Updated patch v. 0.2

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.
Comment 73 User image rbs 2003-06-18 22:51:06 PDT
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.
Comment 74 User image rbs 2003-06-18 22:56:53 PDT
> 4a) press up arrow
shoud read "left arrow"
Comment 75 User image Christopher Aillon (sabbatical, not receiving bugmail) 2003-06-18 23:13:14 PDT
When did the treewalker stuff stop working?  Could it maybe be related to bug
209701?
Comment 76 User image Christian Schmidt 2003-06-19 14:14:39 PDT
Created attachment 126056 [details] [diff] [review]
Updated patch v. 0.3

> 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.
Comment 77 User image rbs 2003-06-19 15:03:34 PDT
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

Comment 78 User image Christian Schmidt 2003-06-21 10:22:09 PDT
Created attachment 126191 [details] [diff] [review]
Patch v. 0.4

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.
Comment 79 User image rbs 2003-06-22 21:15:19 PDT
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()?

Comment 80 User image Christian Schmidt 2003-06-23 03:36:46 PDT
> 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.
Comment 81 User image rbs 2003-06-23 19:00:39 PDT
Created attachment 126333 [details] [diff] [review]
[back-end] helper patch to show the caret even with a selection

> 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 82 User image rbs 2003-06-23 19:24:18 PDT
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.]
Comment 83 User image Boris Zbarsky [:bz] (still a bit busy) 2003-06-23 20:29:34 PDT
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...
Comment 84 User image rbs 2003-06-23 20:52:19 PDT
> 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.
Comment 85 User image Boris Zbarsky [:bz] (still a bit busy) 2003-06-23 21:21:19 PDT
Ah, ok.  That sounds like a good reason.  ;)
Comment 86 User image rbs 2003-06-24 18:33:07 PDT
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.
Comment 87 User image rbs 2003-06-27 10:48:57 PDT
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.
Comment 88 User image Karsten Düsterloh 2003-06-27 11:16:20 PDT
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...
Comment 89 User image Christian Schmidt 2003-06-30 15:28:28 PDT
Created attachment 126780 [details] [diff] [review]
Patch v. 0.5

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.
Comment 90 User image rbs 2003-07-01 04:47:43 PDT
Created attachment 126816 [details] [diff] [review]
supplement to show the status for gotoline as well

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]
************************************************************
Comment 91 User image rbs 2003-07-01 16:55:23 PDT
Created attachment 126868 [details] [diff] [review]
patch v0.6: christian's patch v0.5 + supplement

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;
+  }
Comment 92 User image rbs 2003-07-01 16:59:59 PDT
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.
Comment 93 User image neil@parkwaycc.co.uk 2003-07-02 03:44:48 PDT
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?
Comment 94 User image rbs 2003-07-02 10:53:04 PDT
>>+      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.
Comment 95 User image Christian Schmidt 2003-07-06 09:31:15 PDT
Created attachment 127117 [details] [diff] [review]
Patch v. 0.7 that addresses comment 93.

> 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".
Comment 96 User image Christian Schmidt 2003-07-09 07:55:48 PDT
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.
Comment 97 User image neil@parkwaycc.co.uk 2003-07-09 08:57:57 PDT
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?)
Comment 98 User image neil@parkwaycc.co.uk 2003-07-09 09:14:21 PDT
>> 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 99 User image Boris Zbarsky [:bz] (still a bit busy) 2003-07-09 12:20:13 PDT
Comment on attachment 127117 [details] [diff] [review]
Patch v. 0.7 that addresses comment 93.

Looks good.
Comment 100 User image rbs 2003-07-16 00:03:28 PDT
Checked-in on behalf of Christian, with the switch to a selection listener as
suggested by the reviewer.
Comment 101 User image Steffen Wilberg 2003-07-17 15:42:54 PDT
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
Comment 102 User image Boris Zbarsky [:bz] (still a bit busy) 2003-07-17 15:51:03 PDT
Feel free to file a bug on firebird, since they saw fit to fork this file.
Comment 103 User image Steffen Wilberg 2003-07-18 04:12:45 PDT
Bug 207656 cares about Firebird (goto line # and show line #). The guys are
working on a patch already.
Comment 104 User image rbs 2003-07-22 21:23:55 PDT
Christian, apprently |prevPos| is set nowhere in your patch:

Error: prevPos is not defined
Source File: chrome://navigator/content/viewsource.js
Line: 468
Comment 105 User image Christian Schmidt 2003-07-23 15:29:20 PDT
Created attachment 128369 [details] [diff] [review]
Fix for bug described in comment 104

Hmm, appearently something went wrong between versions 0.6 and 0.7.  This
little patch fixes it.
Comment 106 User image rbs 2003-07-23 16:27:16 PDT
Comment on attachment 128369 [details] [diff] [review]
Fix for bug described in comment 104

Checked in.
Comment 107 User image Boris Zbarsky [:bz] (still a bit busy) 2004-03-17 13:56:20 PST
*** Bug 237827 has been marked as a duplicate of this bug. ***
Comment 108 User image Kevin Brosnan 2005-01-14 13:07:53 PST
*** Bug 278436 has been marked as a duplicate of this bug. ***
Comment 109 User image Alex Bell 2016-01-31 18:00:00 PST
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.

Note You need to log in before you can comment on or make changes to this bug.