Closed Bug 52154 Opened 23 years ago Closed 23 years ago

Make gui for view source coloring pref (was make a pref...)

Categories

(SeaMonkey :: UI Design, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9

People

(Reporter: akkzilla, Assigned: gerv)

References

Details

(Whiteboard: [se-radar])

Attachments

(12 files)

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?
Status: NEW → ASSIGNED
taking for qa. this would be tres nifty (since it's avail in 4.x). any hope of
getting this in for 6.0?
Component: Browser-General → XP Apps
Keywords: 4xp
QA Contact: doronr → sairuh
Whiteboard: se-radar
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
Keywords: nsbeta3, regression
moving to m19, this can wait till after b3 push
Whiteboard: se-radar → [nsbeta3-]se-radar
Target Milestone: --- → M19
*** 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.
Assignee: akkana → jce2
Status: ASSIGNED → NEW
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. :)
Status: NEW → ASSIGNED
Keywords: patch, review
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.
Keywords: approval
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."
Keywords: relnote3
*** 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.
Keywords: 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.
OS: Linux → All
Hardware: PC → All
>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.
Whiteboard: [nsbeta3-]se-radar → [nsbeta3-]se-radar[rtm-]
Whiteboard: [nsbeta3-]se-radar[rtm-] → [nsbeta3-]se-radar[rtm-] relnote-user
Mozilla 0.9, also very easily finished.
Keywords: mozilla0.9
Target Milestone: M19 → mozilla0.9
*** 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"?
Summary: Make a pref for view source coloring → Make gui for view source coloring pref (was make a pref...)
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?
Whiteboard: [nsbeta3-]se-radar[rtm-] relnote-user → se-radar
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!
Keywords: nsbeta3, rtm
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.
Whiteboard: se-radar → There is significant disagreement on what should be done.
jce: make it so. if someone later wants to move it out of debug they can.
Keywords: relnote3
Whiteboard: There is significant disagreement on what should be done. → [se-radar] There is significant disagreement on what should be done.
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
Assignee: jce2 → gervase.markham
Status: ASSIGNED → NEW
Keywords: regressionmozilla1.0
Whiteboard: [se-radar] There is significant disagreement on what should be done. → [se-radar]
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.
Blocks: 74486
Fix landed.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
<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...
Status: RESOLVED → VERIFIED
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).
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.