Closed Bug 15364 Opened 25 years ago Closed 21 years ago

Add line numbers to View Source

Categories

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

Product:
Core Graveyard
Component:
View Source
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Future

People

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

References

Details

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

Attachments

(3 files, 8 obsolete files)

per-window browse with caret
786 bytes, patch
Details | Diff | Splinter Review
Patch v. 0.7 that addresses comment 93.
13.09 KB, patch
neil
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Fix for bug described in comment 104
592 bytes, patch
Details | Diff | Splinter Review
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 ago → 25 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: 25 years ago → 21 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.

Attachment

General

Creator:
Created:
Updated:
Size: