Closed Bug 246620 Opened 20 years ago Closed 12 years ago

Add line numbers to View Source

Categories

(Toolkit :: View Source, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: sugar.waffle, Assigned: darktrojan)

References

(Depends on 4 open bugs, Blocks 1 open bug)

Details

Attachments

(6 files, 8 obsolete files)

18.39 KB, image/png
Details
1.03 KB, application/x-xpinstall
Details
133 bytes, text/html
Details
1.99 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
3.34 KB, patch
ehsan.akhgari
: review+
philor
: checkin+
Details | Diff | Splinter Review
13.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
The status line is shown by view source by correction of bug15364 at mozilla.
I want the same function also at Firefox.

There is a problem of bug 194380 in present mozilla view sourcede.
When adding a function by Firefox, cautions are required also for this problem.

Windows XP
Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.7) Gecko/20040613
Firefox/0.8.0+
Firstly, the status bar is very useful, mainly for displaying the Column
number. Go to Line, while useful functionality, is not as useful as just
scrolling through the file with line numbers displayed in parallel.

When tracking down validation errors oftentimes all the context you have to go
on is the line# and column# the validator gives back. If your site uses
includes or any sort of programming, these numbers will not correspond to the
numbers in your own local source file, so you need to be able to find the line
in the online source (do a view source on the generated page) and then find the
corresponding text in the local copy.

The Mozilla suite's implementation of this is not as good as it could be. There
should be a column running down the left side of the window listing the line
numbers. This makes word-wrap line breaks obvious. I'm attaching an image of
how this is represented in Homesite 4.5.

The column can either be fixed width with a gutter as above, or can be sized
dynamically based on the length of the source file. I'm not a Mozilla hacker,
but to get the required width of the line# column you can get the number of
lines of source code, which will be, say, 350. Then you know it needs to be 3
digits wide.
This looks like a DUPE of bug 207656 
That bug looks like something to put the current line number in the status bar.
That is a useless feature when looking for a particular line.
Having current cursor position (column and line) is very usefull when validating
an html page !
So when a validator says there is an error on line 394, how fast can you find
that by scrolling, clicking, and looking at the status bar? How many times do
you have to do that to find the right line?
This is an extension by Chris Neale (http://cdn.mozdev.org/) that shows the
line and column number of where the cursor is in the view source window in the
status bar of the window and also  adds the edit->goto line menu item that lets
you jump to a certain line number directly. Maybe it is helpful for those that
are desperate, want to improve on it or want to use it as a basis for a patch
for this bug.
All that is really needed is something that says something like:

foreach(line) {
  $i++
  print i . ". $";
  }

Forgive the pseudo-Perl ... it's my native tongue.
I agree with comment 5; displaying the character in the status bar is not the
ultimate solution (and it's coverted by bug 207656 anyway). I think the solution
is to have line numbers to display in the left margin and ensure that
copy-pasting the source won't also copy-paste those numbers. To do this, I
believe the following would work:

a) Change the code here
http://lxr.mozilla.org/seamonkey/source/parser/htmlparser/src/nsViewSourceHTML.cpp#1194
to output a new pre element at every line break in the source (I don't know C++)
b) Put in a CSS counter to show a number at the start of every pre element.
I would suggest something different (additional) to what others are saying. I
wound really like to have an option in the View menu (checkbox) to Show Line
Numbers. If checked, this would add a vertical bar along the left side of the
screen with line numbers in it. Similar to VC or pretty much any other decent
text/code editor. Of course, a Goto Line... feature would also be great.
here here - line numbers next to the lines !
This bug is absolutely critical to resolve.  Firefox view source is like a complete joke without view line numbers.
I am using the excellent HTML Validator plugin to view HTML errors and warnings, and my biggest complaint is the lack of line numbers. The HTML Validator plugin author says that this is something mozilla/Firefox core needs to fix.
I know this bug has been neglected, but does anyone know anyone who can either A) fix this or B) mark it as a 2.0-blocker?
Can only nominate.
Flags: blocking-firefox2?
Assignee: bugs → nobody
We're not blocking the release of Firefox 2 for this. It sounds like a worthwhile addition, though.
Flags: blocking-firefox2? → blocking-firefox2-
Yes, would be great to have it in :)
The newest beta of the HTML Validator plugin has support for line numbers in view source, although it's a little buggy at the moment.
http://users.skynet.be/mgueury/mozilla/
Is this a dupe of bug 239221? (Or maybe dupe the other once since the last comment was at the end of 2004?) I'm not sure what the original reporter is talking about but it looks like there is a status bar for view source for the trunk build.

As for implementing line numbers in the left column, it looks likes nsViewSourceHTML.cpp already adds a <pre id="line%"> tag at intervals defined by NS_VIEWSOURCE_TOKENS_PER_BLOCK in order to fix bug 86355.

Can't I just output the tag for every line instead of intervals and just add a CSS before counter the pre tag at viewsource.css? (As suggested in a previous comment) I can then add a View -> Line Numbers menu option to enable or disable the css counter similar to the wrap lines implemention.

It would also simply the go to line feature without all the treewalker business.

Any reasons why the pre tag can't be outputted for every line?
Sorry, I meant bug 239211 not 239221.
No, bug 239211 is the SeaMonkey equivalent of this bug.
Product: Firefox → Toolkit
Depends on: 469762
Why is this still not implemented? We are now at 3.1!
It would be really useful for any web developer to have ability to easily determine what exact line is line that Error Console referring to. "Go to Line" feature is not the same. Very not the same.
5 1/2 years? Is it sooo hard to fix?
Just notice for developers: even IE8 already shows line numbers in View Source.
Attached patch patch v1 (obsolete) — Splinter Review
This does the donkey work of actually adding numbers into the HTML (each line number in a <span class="linenumber">). There's still some issues with how best to use the span - the current CSS is a bit of a hack, but it works.

(This patch also fixes an earlier mistake where I assigned a PRInt32 to a PRBool. Oops.)
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #432985 - Flags: feedback?(mrbkap)
Depends on: 166235
No longer depends on: 469762
Depends on: 469762
Attachment #432985 - Flags: feedback?(cab)
(In reply to comment #28)
> Created an attachment (id=432985) [details]
> patch v1
> 
> This does the donkey work of actually adding numbers into the HTML (each line
> number in a <span class="linenumber">). There's still some issues with how best
> to use the span - the current CSS is a bit of a hack, but it works.
> 
> (This patch also fixes an earlier mistake where I assigned a PRInt32 to a
> PRBool. Oops.)

Note that you can use CSS counters to generate the linenumbers for you, for example:

  body {
    counter-reset: line; 
  }
  .linenumber:before {
    content: counter(line) ": ";
    counter-increment: line;
  }

I haven't looked at the C++ code in detail yet, but I see that with your patch there is both a "mLineNumber" and a "myLineNumber".

(Also, mrbkap's feedback is likely to be more useful than mine since he can actually approve your patch for landing.)
Attached patch patch v2 (obsolete) — Splinter Review
I *think* the only remaining piece is to make the space for line numbers smaller or larger as appropriate. Is there a way to know the total number of lines when I'm in BuildModel (ie. before we go through and count them all, so I can set an attribute on the body tag)?
Attachment #432985 - Attachment is obsolete: true
Attachment #432985 - Flags: feedback?(mrbkap)
Attachment #432985 - Flags: feedback?(cab)
Attachment #440990 - Flags: feedback?(mrbkap)
Attachment #440990 - Flags: feedback?(bzbarsky)
(In reply to comment #30)
> Created an attachment (id=440990) [details]
> patch v2
> 
> I *think* the only remaining piece is to make the space for line numbers
> smaller or larger as appropriate. Is there a way to know the total number of
> lines when I'm in BuildModel (ie. before we go through and count them all, so I
> can set an attribute on the body tag)?

I personally think just assigning a fixed width like you're currently doing is fine.
Geoff: I discovered a bug with how the line numbers are displayed in the presence of a multi-line HTML comment.  This is with the v2 patch.
Attached patch patch v3 (obsolete) — Splinter Review
Good point. And fixed.
Attachment #440990 - Attachment is obsolete: true
Attachment #441458 - Flags: feedback?(mrbkap)
Attachment #441458 - Flags: feedback?(bzbarsky)
Attachment #440990 - Flags: feedback?(mrbkap)
Attachment #440990 - Flags: feedback?(bzbarsky)
I didn't look at the code too carefully yet (though there are various formatting and string usage issues there as far as I can tell).  General questions:

1) Have you measured the memory consumption of view-source on large files with
   and without this patch?  How does this patch affect it?
2) Have you measured the performance of view-source on large files with and
   without this patch?  How does this patch affect it?
Comment on attachment 441458 [details] [diff] [review]
patch v3

pending answers to my questions.
Attachment #441458 - Flags: feedback?(bzbarsky) → feedback-
I don't really have the right tools to test these sorts of things (and I don't have jemalloc), but memory usage seems to be up about 10-15%. 

StartNewPreBlock seems to use a lot of time. Called every 40 lines the overall time was up 50-90% on some pages. :( If it's not called at all the overall time is up less than 10%.
Attachment #441458 - Flags: feedback?(mrbkap)
(In reply to comment #36)
> I don't really have the right tools to test these sorts of things (and I don't
> have jemalloc), but memory usage seems to be up about 10-15%. 

You might look at a simple count of elements in the view source document, both with line numbers and without.  E.g. "javascript:alert(document.body.getElementsByTagName("*").length)".  As I recall, the performance difference I saw for view-source linkification pretty closely correlated to the number of elements created.

> StartNewPreBlock seems to use a lot of time. Called every 40 lines the overall
> time was up 50-90% on some pages. :( If it's not called at all the overall time
> is up less than 10%.

That seems surprising.  (Alas, I have no other insight to offer.)
Attached patch patch v4 (obsolete) — Splinter Review
Fixes multiple-line tags in error and a few other bits and pieces.
Attachment #441458 - Attachment is obsolete: true
Attached patch Simple view-source test (obsolete) — Splinter Review
This is a dirt-simple mochitest for view-source.  It verifies that the source displayed by view-source actually matches the true source.  It doesn't test formatting issues or the display of line numbers in the main patch.  Notably it won't detect the multi-line comment bug in the v3 patch.  It does provide some confidence that changes don't break existing view-source functionality.

Right now the test uses itself as the test source.  It would benefit from better test source files.

Note that the test file can be run directly in the browser with minor modifications (e.g. firefox file://.../view-source.html).
(In reply to comment #39)
> Created an attachment (id=448324) [details]
> Simple view-source test

Very nice. FYI the URL appearing at the beginning is due to the title tag in the view-source page.

>+nsresult CViewSourceHTML::StartLine(void){
>+  //if (mLineNumber % 40 == 0) {
>+  //  StartNewPreBlock();
>+  //}
I think it best if StartNewPreBlock is called every 50 lines, it seems to be a good tradeoff between initial speed and UI responsiveness.

I also think that the line numbers column can be narrowed to 4 characters instead of 5. It's highly unlikely that people would use view source for a 10,000 line page.
Attachment #446641 - Flags: review?(bzbarsky)
Attachment #448324 - Flags: review?(bzbarsky)
(In reply to comment #40)

> I also think that the line numbers column can be narrowed to 4 characters
> instead of 5. It's highly unlikely that people would use view source for a
> 10,000 line page.

Actually I think this is moderately common.  I know a lot of work went in to Firebug to handle long files.  On the other hand, I'm not sure that we need to look pretty when handling really long files.  For example, if the main content gets bumped over by an extra character going from line 9999 to 10000, that may not be such a big deal.
Jeff, it's likely to be at least a week (two is more likely) until I get to this review...
(In reply to comment #42)
> Jeff, it's likely to be at least a week (two is more likely) until I get to
> this review...

Is two weeks up yet Boris? :)
Yes, but people keep dumping emergencies on me.  :(  And my sincere apologies for mis-spelling your name....

Current time estimate is ... sometime.  If no one else dumps 1.5MB "must review now" crap on me, should be within a week.
Attachment #446641 - Flags: approval2.0?
Comment on attachment 446641 [details] [diff] [review]
patch v4

(same as bug 536503 - can't ask for approval until review is completed)
Attachment #446641 - Flags: approval2.0?
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #446641 - Attachment is obsolete: true
Attachment #475811 - Flags: review?(bzbarsky)
Attachment #446641 - Flags: review?(bzbarsky)
Attached patch more detailed tests (obsolete) — Splinter Review
I've added these tests to make sure the patch does as it's supposed to. Kept as a separate patch for clarity's sake.
Attachment #475812 - Flags: review?(bzbarsky)
Ok.  So I really suck at getting to this review, mostly because I keep not finding the time to page this code back in.  :(  Geoff, I'm really sorry about that, and about my complete lack of communication here.

And now Henri is in the process of removing this whole file and rewriting view-source on top of the HTML5 parser...

Henri, would you be willing to effectively roll these patches into your rewrite or do something like this as a followup or something?
(In reply to comment #48)
> Henri, would you be willing to effectively roll these patches into your rewrite
> or do something like this as a followup or something?

As a follow-up would work. The rewrite is now big enough that I have worries about it getting it reviewed and landed, so I don't want new features added before the rewrite has landed.

It seems to me that the patch here regresses bug 86355. Am I missing something?
The patch still does the multiple <pre> block thing.

It also puts each line into a separate block, which breaks bidi stuff even more than bug 86355 did...
Depends on: 482921
Thanks for the update Boris. I'll have another look at this once the rewrite is done.
Comment on attachment 448324 [details] [diff] [review]
Simple view-source test

r=me
Attachment #448324 - Flags: review?(bzbarsky) → review+
Attachment #475812 - Flags: review?(bzbarsky)
Attachment #475811 - Flags: review?(bzbarsky)
Can the reviewed test be landed?
Attached patch patch, v6Splinter Review
This is effectively the same CSS as the previously reviewed patch, the other changes are unnecessary now.
Attachment #475811 - Attachment is obsolete: true
Attachment #571210 - Flags: review?(ehsan)
Attachment #475812 - Attachment is obsolete: true
Attached patch fixed-up test (obsolete) — Splinter Review
I've made a few slight changes to Curtis's test.
Attachment #448324 - Attachment is obsolete: true
Attachment #571213 - Flags: review?(ehsan)
Comment on attachment 571213 [details] [diff] [review]
fixed-up test

Drive-by comment:
>+        return xhr.responseText;

If you are only reading responseText, please set xhr.responseType = "text" to make the test run faster.
Attachment #571210 - Flags: review?(ehsan) → review+
Comment on attachment 571213 [details] [diff] [review]
fixed-up test

The test has two problems:

1. It doesn't use SimpleTest.waitForExplicitFinish/finish, so it effectively doesn't test anything.
2. It loads the same test in an iframe, which could potentially cause _another_ instance of the test to run.  I suggest that you add a test HTML file and test the view-source protocol on that URL instead.

Also, please address comment 56 in the next version of the test.  Thanks!
Attachment #571213 - Flags: review?(ehsan) → review-
Attached patch testsSplinter Review
(In reply to Ehsan Akhgari [:ehsan] from comment #57)
> 2. It loads the same test in an iframe, which could potentially cause
> _another_ instance of the test to run.  I suggest that you add a test HTML
> file and test the view-source protocol on that URL instead.

It's loading the source of the test in the iframe, not the test itself.
Attachment #571213 - Attachment is obsolete: true
Attachment #577870 - Flags: review?(ehsan)
Depends on: 706394
Comment on attachment 577870 [details] [diff] [review]
tests

Review of attachment 577870 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!
Attachment #577870 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4730eb3ec77a
https://hg.mozilla.org/integration/mozilla-inbound/rev/389a18921ea5

Okay, I've pushed this. It doesn't currently work for non-HTML documents, so I'll file a followup and hopefully have it fixed by the end of this cycle.
Target Milestone: --- → mozilla12
Depends on: 713479
Looks like the reference images need to be updated to include the line numbers.
Attachment #584319 - Flags: review?(ehsan)
Comment on attachment 584319 [details] [diff] [review]
fix for broken reftests

Please add a comment why empty id attributes.
And, if possible, remove =""
Attachment #584319 - Flags: review?(ehsan) → review+
Summary: Add line numbers to View Source for Firefox → Add line numbers to View Source
Depends on: 808401
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: