Gervase Markham wrote: > In http://bugzilla.mozilla.org/show_bug.cgi?id=49187 you say: > > "Source coloring can be switched back on by #defining > VIEW_SOURCE_COLORING in nsViewSource.HTML. It is currently off since that > substantially speeds up the display of the source window." > > Will this become a user choice? Vidur asked if someone in the editor team would be willing to do this, so I'll step up to the plate. Any suggestions for the name of the pref?

taking for qa. this would be tres nifty (since it's avail in 4.x). any hope of getting this in for 6.0?

Nominating for nsbeta3, based on Akkana's email estimate of "trivial - less than an hour's work". Also, seeing as we had source highlighting before, this is a regression. Pref: "Syntax highlighting when Viewing Source" for the UI, dunno the namespace for the internal name so can't comment. Gerv

moving to m19, this can wait till after b3 push

*** Bug 52097 has been marked as a duplicate of this bug. ***

If this is trivial to impliment, then why not make it nsbeta3+? I might assign this one to myself. If this isn't trivial, then turn syntax hilighting back on by default. Since when did maximum speed become more important then readability when viewing source?

jce2: If you want to do it, then you do not need nsbeta3+. Unfortunately, Netscape is currently focussing on issues that are considerably more important than a pref to change how pretty View Source is. Pleeeease do it... ;-)

I'm taking this one. I'll land it in two steps. Step one will be the back end stuff, making it so you can add "browser.view_source_syntaxs_hilight=true" to your prefs file manually. Step 2 will be adding the preference to the pref panel. I need UI advice (for example, what pane should it go into?). I'm about to attach a patch for step 1.

Ok, that would be user_pref("browser.view_source_syntax_hilight", true); Same difference. :P

Adding keywords "patch" and "review". I've tested this patch on my build and it exhibits the following behaviour: Preference not in prefs.js = No syntax hilighting. preference set to true = syntax hilighting. preference set to false = No syntax hilighting. So checking this in is very low risk, and won't affect the default behaviour. Please review it and check it in so people who download nightly builds can turn syntax hilighting back on. :)

How about browser.view_source.hilight_syntax ? Of course, I think it should be "highlight", but I'm English :-) Gerv

How about a better pref name: browser.view_source.syntax_hilight or something. I can imagine that we may come up with different view source prefs in future.

Staying on the cc list (thanks for taking it!) and will look at the patch. Me, I'd prefer browser.view_source.syntax_highlight (it's long anyway, why not take the extra 2-char hit and spell it right?) but the important thing is to get it checked in.

The patch looks fine (though I suggest we switch to using NS_LITERAL_STRING as long as we happen to be in that code) and I've sent mail asking for approval to check this in. (We can change the name of the pref, or not, but it doesn't affect the validity of the patch.)

Ok, now the pref is called "browser.view_source.syntax_highlight" Adding "approval" keyword.

I sent comments by mail, echoing akkana's plea for NS_LITERAL_STRING and NS_WITH SERVICE, adding emphasis. I also whined about non-conforming if/brace style. If the substantive items above (NS_LITERAL_STRING, NS_WITH_SERVICE) get fixed, then a=brendan@mozilla.org. /be

I've now got a r=akkana and an a=brendan for the patch for step one. Please check it in. I find it a bit disturbing that I haven't gotten any UI suggestions for part two (as in what preference pane it should go into). Adding relnote3 keyword in case step two doesn't make it in before nsbeta3. The Relnote in that case should be the following: "Syntax highlighting in the view source window is turned off by default for better performance. To turn syntax highlighting on, add the line "user_pref("browser.view_source.syntax_highlight", true)" to your prefs.js file."

*** Bug 52509 has been marked as a duplicate of this bug. ***

Akkana: Now that we have a r= and an a=, can you check this in for me?

Definitely. I haven't been able to today, due to the tree being closed, but I expect I should be able to check it in tomorrow.

The patch is in! Thanks again for the contribution.

The last two attachments are the patch for step two of fixing this bug. This will create a checkbox in the "advanced" preferences pane. I'm asking for another a=, since I (assume I) have a r=jag. Once this has been checked in, and confirmed to work on all platforms, then this bug can be considered fixed.

Waterson, please give me an a=? If we don't get this checked in, then people are going to bitch about no syntax highlighting faster then we can tell them to alter their prefs.js file.

Oh shit. Replace: pref="true" preftype="bool" prefstring="view_source.syntax_highlight" with pref="true" preftype="bool" prefstring="browser.view_source.syntax_highlight" in pref-advanced.xul

IMHO the pref doesn't fit under "enable features that help interpret webpages" :-)

Ok, then what panel DOES this belong under? The advanced pane seemed to be the best place to put "misc" stuff. If we aren't getting this pref into nsbeta3, then we should have syntax highlighting enabled by default (add a line to prefs.js setting that preference to true).

How about the `Colors' panel?

bug 26593 (unable to ctrl-C from view source) will probably show up again when this pref is on.

No it won't. Coloured source uses HTML, not XML now.

BTW, please see http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey for checkin rules (you'll need to get module owner review and super review approval if you want to check this in).

Hmm, yep. I suggest putting the pref(s) in the Colors panel, under the bit which talks about whether to allow documents to specify their own colors. Ideally, the same colors as are used for HTML source in the source window should be used when showing un-stylesheeted XML source in the browser itself.

OK, *LAST* call for comments. Should this pref be in the "Colours" panel or not?

the Colors panel is fine with me. (or, Advanced main panel as a runner-up: since viewing the source might be considered a form a advanced usage wrt endusers.)

nominating for rtm.

I really think that this should be in under "advanced" after more thought. Colours is really only talking about the colors of links, and other color selections. If we were allowing the user to choose the syntax highlighting colors, then it would be more appropriate to go under colours, but since it is only an on/off switch, I think that it should go under advanced, where there are a lot of other assorted on/off switches. Besides, the average user would want syntax highlighting on. Only an "advanced" person would really need blazing speed in the syntax highlighting window. Is there someone at Netscape that can make a good final UI decision about this?

Well, let's see. You can cc the UI lead at Netscape, wait a couple weeks, and get a flimsy answer. Or, you can ask Matthew and get a good, solid answer within a matter of days... Your choice.

mpt (I assume that was the "matthew" that blake was talking about), could you explain further why you originally said that the pref should go under "colors" and not "advanced", given my comments that this is basically an "on/off" switch, and should be grouped with a bunch of other on/off switches (which would be the advanced folder).

Actually, I don't think this should be a pref at all. If you're the sort of person who would be affected by any slowness of syntax highlighting (i.e. a Web developer or similar geek), you are going to be the sort of person who has a computer fast enough to handle it, and/or you are going to be the sort of person who farms out HTML source viewing to an external text editor anyway. When I suggested the Colors panel, I hadn't read the bug properly (sorry), and I was thinking of controls for changing the actual colors, hidden behind a disclosure triangle: | Category: Colors & Effects ::::::::::::::::::::::::::: | | +-------------------+ | | |=General===========| [::] _Backgrounds [@@] _Links | | |=Display===========| [##] _Text [OO] _Recent links | | | Languages | [@@] _Headings [**] _Highlighted links | | | Fonts | [OO] _Quoted text | | |::Colors:&:Effects:| St_yle for quoted text: [Italic :^] | | | Styles | | | | Multimedia | [/] Allow documents to use _other colors | | | Filters | [/] _Underline links | | | Scripts | | | | Privacy/Security | > Source code options----------------------- | | | | | | | | | | +-------------------+ :::::::::::::::::::::::::::::::::::::::::::: | | Category: Colors & Effects ::::::::::::::::::::::::::: | | +-------------------+ | | |=General===========| [::] _Backgrounds [@@] _Links | | |=Display===========| [##] _Text [OO] _Recent links | | | Languages | [@@] _Headings [**] _Highlighted links | | | Fonts | [OO] _Quoted text | | |::Colors:&:Effects:| St_yle for quoted text: [Italic :^] | | | Styles | | | | Multimedia | [/] Allow documents to use _other colors | | | Filters | [/] _Underline links | | | Scripts | | | | Privacy/Security | V Source code options----------------------- | | | | [%%] Tag_s [**] _Errors | | | | | | +-------------------+ :::::::::::::::::::::::::::::::::::::::::::: | But for now, if it's just a checkbox, I still think it should go in the Colors panel, not the Advanced panel. Four reasons. 1. Hopefully it will stop being a checkbox and start being a pair of colorpicker widgets (as shown above) pretty soon. Then it would absolutely belong in the Colors panel, and having to remember which prefs category it was in (depending on which version of Mozilla you were using) would be a pain. 2. The source code colors are (or should be) used whenever the user views an XML document which contains even the slightest error (bug 52422), which has no stylesheet, or whose stylesheet is missing. Once XML becomes widespread one might imagine that these situations are going to happen fairly often, which would mean that this pref would hardly be `advanced'. 3. The background, text, and URL colors of source code are (or should be) set by the other controls in the Colors panel. To have some colors set by controls in one panel, and other colors determined by a checkbox in another panel, would be a bit annoying. 4. In the future, the prefs will not have an `Advanced' category at all. So if you put stuff there now, you'll only have to move it later. The argument that since this is a checkbox it should go in a panel with other checkboxes seems ... curious, given that many other prefs panels are mixtures of various kinds of controls. Anyway, the Colors panel already has checkboxes in it. Finally, I don't understand why the pref has a name starting with `browser.', given that it is (or should be) also used when viewing source code of e-mail messages in the mailer app, and source code of documents in the editor app.

if it's changed, how about ui.view_source.syntax_highlight? kinda parallel to the ui.key.accelKey and ui.key.menuAccess pref...

>If you're the sort of person who would be affected by any slowness of syntax >highlighting (i.e. a Web developer or similar geek), you are going to be the >sort of person who has a computer fast enough to handle it, and/or you are >going to be the sort of person who farms out HTML source viewing to an >external text editor anyway. You shouldn't assume that all geeks have fast computers. Also, Mozilla shouldn't discourage curious people with slow computers from playing with HTML. Again, mpt, I think you're being over-zealous in trying to simplify prefs. (btw, there are more than two colors in the view-source window.)

My two cents: $.01 - I like Matthew's UI design. $.02 - I agree that, for now, this pref (though it should exist) should not have a UI (until everything Matthew mentioned is implemented). We shouldn't have a UI for every prefable thing in the world. If we have this as an option in the UI, then users are probably going to wonder why we didn't just put the coloring on, and not make it an option. Then we'll have to explain that putting the colors on makes the window load slower, which makes us look..well...incredibly stupid (`they can't even figure out how to load a window fast if it has some colored text'). But anyways..

Exactly. If you have a performance problem with something as basic as coloring text, then work on speeding up the coloring code -- don't just pref your way out of that responsibility. And the argument that I'm trying to over-simplify prefs `again' doesn't really hold water here. Even Ben Bucksch (my chief detractor in that particular debate) agrees that the `Advanced' category of prefs should go away.

>don't just pref your way out of that responsibility. Agreed, but there isn't time to fix the performance issue, so the next best options are to not color at all or to add a pref. Coloring suffers from a performance issue, so it shouldn't be forced on users. Also, if we decide to leave coloring out of NS6, we can temporarily work around bug 49030 and bug 44186 by not parsing the code that is displayed by view-source.

> there isn't time to fix the performance issue We have *months* to fix the performance issue. If by chance you are referring to the Netscape branch, then you would seem to be under the impression that Web developers would rather use Netscape than use Mozilla, and in that case I have a bridge I'd like to sell you. > so the next best options are to not color at all or to add a pref. You missed one option: to put up with it until the performance issue is fixed (like we're putting up with all the other performance problems in Mozilla right now). > Coloring suffers from a performance issue, so it shouldn't be forced on users. Interpreting HTML in the browser window as anything other than a string of ASCII characters causes a performance hit, yet we `force' such a rendering on all users. Showing headers of an e-mail message as a pretty header panel causes a performance hit, yet we `force' that on all users too. > Also, if we decide to leave coloring out of NS6, Neither you nor I are members of Netscape's PDT, so the use of `we' is rather amusing. Anyway, I would guess the chance of such a patch getting rtm++ed at this stage would be remote. > we can temporarily work around > bug 49030 and bug 44186 by not parsing the code that is displayed by > view-source. Ditto for the patch to implement that workaround.

*** Bug 56264 has been marked as a duplicate of this bug. ***

rtm-, too late for UI changes on the branch and seems like a true P3.

Mozilla 0.9, also very easily finished.

*** Bug 61608 has been marked as a duplicate of this bug. ***

Updating summary to indicate that the pref is there, but there isn't a checkbox in the preferences window yet. Btw, what's the bug for the performance problems, so the pref can eventually be changed to default to "on"?

from what i can tell [after grepping the files in defaults/pref/], the pref for browser.view_source.syntax_highlight isn't in any of the files yet. so, i guess this means one would need (1) know about it and (2) add it in manually [eg, your prefs.js] and set it to true. apologies for my pendantic-ness! but i guess my remaining question is, why doesn't this appear in all.js? thx again!

Yes, we need to get a default value for the pref checked in. (I could swear that I made the review contingent upon that, but don't seem to have said so here; if I forgot that, mea culpa, but it's a necessary part of adding a new pref.)

Clearing out all notations in the Status Whiteboard except "se-radar" because I don't know what that is for. Are we going to expand this bug to cover definition of colours in syntax highlighting, background colours, etc, or do we still only need a single checkbox? And was the default pref ever added to the all.js file?

nope, the pref still isn't in the defaults/pref/all.js file. what needs to get done for that to happen, and could it get done soon? thx!

actually the pref should be set as: pref("browser.view_source.syntax_highlight", false); to match what's in nsViewSourceHTML.cpp, right...?

Well you could use pref("browser.view_source.syntax_hilight", false) in all.js if you actually wanted it to do something ... [second patch does that even though i didn't fix the comment]

would this also enable blinking if tags are not closed appropriately ... as it was a NS 4.X behavior?! This was very useful and some kind of HTML debugging.

Noting that there really isn't a consensus on what should be done, which is why this bug is just sitting here at the moment. Notes: 1) This bug should NOT be related to the request for a UI that specifies the colours used in the syntax hilighting 2) This bug is also not related to making items flash in the syntax highlighting that aren't well formed. Personal preference: I would like to add a checkbox to the "debug" section of the prefs and be done with it.

jce: make it so. if someone later wants to move it out of debug they can.

Is there a bug actually filed on the perf problem rather than band-aiding this with a pref?

*** Bug 71801 has been marked as a duplicate of this bug. ***

Re markush@world-direct.com's comment from December: I find the tag-blinking in NS4 very annoying, since the it makes it nearly impossible to read the part of the html that's blinking.

blinking is just on errors,which is a very convenient feature for finding erros.

With the new cache in the delay in the display of highlighted HTML is not as noticable because previously you had the overhead of fetching the source from the network and highlighting it. On most reasonably sized HTML pages the delay in displaying the source is not as noticable. Some work should be done on making the highlighting more effecient but it's probably fast enough to enable it by default (but have a pref in the ui). We need something in view source to make it better than the IE default of opening in notepad and highlighting is useful.

Right. Enough discussion :-) What I plan to do here (unless Jason wants to take the job off me, and he's quite welcome to) is: a) change the name of the pref to "view_source.syntax_highlight". It's not a browser-only pref and view source is probably big enough and cross-product enough to stand on its own. If there is a spec for pref names, point me to it. b) Add a single checkbox to Preferences | Appearance | Colours for it. This will default to "on". Later, this will be expanded to allow colour selection etc., when anyone gets around to implementing it, according to mpt's UI spec. c) Get a default pref of "true" checked into all.js. I think that if performance is poor, we shouldn't hide it behind having a sucky view-source window. Any anyway, isn't someone rewriting it? I will begin implementing this strategy in 48 hours unless anyone objects before then :-) Gerv

OK, patch attached implementing my plan from before. Looking for r= from akkana? Gerv

I like the plan as stated; the code changes look fine to me; with the default value for the pref, the view source window comes up with syntax highlighting and doesn't take TOO long. This quickie review is unfortunately all I have time for right now (I've been called out of town on a family emergency and I'd planned to leave five minutes ago) but it looks good and I don't want to block it, so r=akkana. If anyone else sees anything objectionable, please speak up.

Looks good to me! Looking forward to using this. sr=hewitt

[Registering interest on coloring w.r.t. MathML where the tag-to-content ratio is large. Usually, single characters are surrounded by a deep hierarchy of tags. Coloring greatly helps to wade through the markups.] The observed perf problem that has been lingering is now being worked out in bug 74486.

Fix landed.

Er... looking at the patch: + <checkbox id="syntaxHighlight" label="&syntaxHighlight.label;" accesskey="&syntaxHighlight.accesskey;" + pref="true" preftype="bool" prefstring="browser.view_source.syntax_highlight" prefattribute="checked"/> And +pref("view_source.syntax_highlight", true); And +thePrefsService->GetBoolPref("view_source.syntax_highlight", &syntaxHighlight); Which of these pref names is not like the other two? Needless to say this does not work. Reopening.

r=blake, r=stephend, I can't believe that was missed in the 1st place. Thanks, Boris. Fix checked in, for real this time.

<shuffles feet> Er... not sure how that happened. Ahem. Gerv

Thanks for finishing this. I had patches that would place the checkbox in debug, but for some inexplict reason, it wasn't working for me! My project at work is getting close to shipping, which is why I keep lacking time to get my bugs fixed. :-(

I want this UI removed. I agree with mpt and blake, this feature should NOT need a UI. A pref as an excuse for slow syntax highlighting covers the issue that the syntax highlighting system is slow. If we must have any sort of UI, could it be accessible to the view source window? (maybe as a context menu item or something). Please, before adding pref *UI* to navigator/xpapps components (in this case, Navigator), please get me or someone like me (e.g. mpt, blake, etc) to comment and review.

This bug has had significant UI input from mpt. However, if you want the UI removed, you're the boss - just back out the XUL changes. I believe that should do the trick, but I can't be certain as my net access is currently via Windows CE 1.0. Turning it on by default was the other main thing - I think we are agreed we need to keep that... Gerv

I believe Doron at some point had a way to turn this on and off as part of the menus he is making for the view source window. Doron, you want to readd that to your menus? Then the UI in prefs can be backed out if Ben decides to do so.

yes, i had a checked/unchecked menuitem which did this. I think I still have the code backed up.

The "Additional Comments From Matthew Thomas (mpt) 2000-10-02 16:40" sum up the weirdness of this checkbox. Since coloring is expected, users will find it weird that there is a pref for it at all. Unfortunately, until recently, the slowness that was at the basis of the pref was on nobody's plate. (Style resolution is expensive. The existing code uses the style='.' attribute which is the slowest way to style a page. The style='.' is _parsed_ and this court-circuits the efforts at speeding the matching style resolution algorithm. Worse: coloring is done by setting this attribute on every tag.) Hopefully the improvements provided by the patch in bug 74486 might help performance-wise so that the pref could be removed/hidden. What is also nice with that patch is that colors are not hard-coded anymore. So while awaiting point 1 in mpt's comments [if this ever gets implemented], users will be able to hand-edit the viewsource.css file to set different colors.

Gerv, mpt later stated that using prefs to attempt to hide a performance problem is not a good thing. I'd prefer to see the patch that rbs refers to evaluated and checked in rather than weighing down an already heavy preferences dialog with options that will make no sense to a majority of people (even advanced users like myself... who often have the attitudes like "fix the speed problem" or "open notepad like IE")

I have checked-in the patch.

rbs' patch is in. We need to remove that checkbox and replace it with a way of changing the View Source colours - or do we think that's so trivial that it doesn't need UI? As long as we choose them carefully in the first place... Gerv

see bug 62678 for color choices and the like for the view source window.

It just occurred to me that something was left out when jce2 initially implemented viewsource in html. I am mentioning it here for the record in case someone wants to fix it. It has to do with error reporting. The XML version outputs <error>...</error> [see CViewSourceHTML::WriteTagWithError()]. In the HTML version, this has to become <span class="error">...</span>.

bug 76412 filed on the error reporting thing.

vrfy'ing a couple of things: * coloring in view source is present [has been for a while]. * the frontend pref is in the Colors pref panel [for better or for worse :)] if anyone wants the the frontend pref pulled out or moved elsewhere, i'd prefer a separate bug rather than reopening this...

I have filed bug 129612 to remove this checkbox from the UI (possibly to have it moved to Debug UI, or somewhere far less conspicuous).