155389 - More <link> elements & templatization of navigation_links

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Toms Baugis]

I needed a placeholder bug for this

Comment 1

[Toms Baugis]

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

Comment 2

[Toms Baugis]

Attachment: Diff on existing to use site-navigation.html.tmpl

This removes navigation links from cgi.pl and call out site-navigation template
from header

Comment 3

[Toms Baugis]

Attachment: site-navigation.html.tmpl

Template it self, take #1

It has following features:
1. First, Next, Previous, Last links (modified version of one in cgi.pl and
navigation.html.tmpl)
2. More -> Administration -> ...  contains to user accessible tools

Comment 4

[Toms Baugis]

so - let's start discussion about queries - should they also be in site
navigation bar ?

Comment 5

[Jouni Heikniemi]

My only doubt is about dumping loads of stuff into the head section while most
browsers don't support it. But this may not be an issue. I'll see what it looks
like when I have more time and install a Moz trunk build so that I can see the
<link>s in effect.

Adding keywords, reassigning to patch author.

Comment 6

[Toms Baugis]

i just wanted to say, that Mathew has nothing to do with the attachments
uploaded (in the tmpl file there are too much contributors :)  )

Comment 7

[Jouni Heikniemi]

Comment on attachment 89935 [details]
site-navigation.html.tmpl

Please write an interface comment for the new templates.

>  <link REL="Top" href="index.cgi" />

rel instead of REL, please.

>[%# *** First Previous Next  Last *** %]

How about "Bug list navigation" or something a bit more informative?

>  <link rel="Up" href="buglist.cgi?regetlastlist=1" />

No XHTML closing tags, please. We're not X-ish yet. :-)

>  <link rel="Contents" href="buglist.cgi?regetlastlist=1"

What? Where's the closing >?

>  [% " <link REL='Administration' title='Parameters'    href='editparams.cgi' /> \n" IF 
user.groups.tweakparams %]

A small REL here, please. Also use double quotes around the attribute values,
that's how we do it elsewhere in Bugzilla. You can use apostrophes as the TT
string quote, as well.

Remove the \n:s; they're not necessary here anyway. 

>  [% " <link rel='Administration' title='Users'         href='http://10.10.3.109/editusers.cgi?action=list&matchstr=&matchtype=substr' /> \n" IF user.groups.editusers %]

... What is that fixed IP address there?

>  [% " <link rel='Administration' title='Attachments'   href=editattachstatuses.cgi' /> \n" IF user.groups.editcomponents %]

href value has a missing starting quote.

Comment 8

[Jouni Heikniemi]

Comment on attachment 89934 [details] [diff] [review]
Diff on existing to use site-navigation.html.tmpl

>-    header_html = navigation_links()

This is bitrotten because of my patch for bug 155388. Removing the parentheses
should cut it, though. :-)

>     [% END %]
>-    
>+   
>     [% IF style %]

What's this whitespace change? If you're removing one space, kill 'em all. Or
just leave 'em. :-)

Comment 9

[Toms Baugis]

Attachment: site-navigation.html.tmpl [the needs-work fixes]

This fixes problems noted in comment 7.
Only thing - lost breaks after <link> tags, so admin tools go in one row in
final source. (somehow \n:s don't want to work anymore) but - maybe it is not
important

Comment 10

[Toms Baugis]

Attachment: diff to use site-navigation.html.tmpl

minor changes, as suggested in comment 8

Comment 11

[Toms Baugis]

Attachment: site-navigation.html.tmpl

There where wrong cases for first and previous buttons (first item of list has
index 0, not 1)

Also - the line break problem solved

Comment 12

[Toms Baugis]

some ideas - how about following menu, if standing on bug:
more -> view -> dependency tree
             -> dependency graph
                ----------------
             -> bug activity
             -> Print Ready

??

maybe something else?

Comment 13

[Jouni Heikniemi]

Comment on attachment 90937 [details] [diff] [review]
diff to use site-navigation.html.tmpl

>diff -u -r bugzilla-2.16rc2/CGI.pl my_contrib/CGI.pl
>--- bugzilla-2.16rc2/CGI.pl	2002-06-03 05:48:06.000000000 +0300

CGI.pl part of this won't apply, because the diff is against the 2.16 branch
and we're trying to create a fix for the trunk. Please recreate this patch so
that it applies cleanly to the tip of the trunk.

Comment 14

[Jouni Heikniemi]

You should remove the navigation_links() call in bug_form.pl as well when
converting the patch to the trunk; note that bug 155388 changed that, too...

Comment 15

[Jouni Heikniemi]

Comment on attachment 90939 [details]
site-navigation.html.tmpl

>[%# INTERFACE:
>  # bug_list: list. List of bugs of current query (if any). Used for navigation.
>  #%]

"List of bug numbers", maybe? You can remove the "Used for navigation" thing if
you need horizontal space. 

You're also using a variable called "bug". That needs to be mentioned, too.
Maybe even user. :-)

>[%# *** Bug List Navigation *** %]
>    <link rel="Top" href="index.cgi">

This is not bug list navigation -> move the comment two lines lower.

>[% IF bug_list && bug_list.size> 0 %]

Nit: Add a space before >. 

>    <link rel="Up" href="buglist.cgi?regetlastlist=1">
>    <link rel="Contents" href="buglist.cgi?regetlastlist=1">

I think only "Up" should do this, because the bug list isn't certainly "the
contents of bugzilla". I'm not adamant on this, but it feels odd to use two
links for exactly the same thing.

>  [% this_bug_idx = lsearch(bug_list, bug.bug_id) %]

I think "current_bug_idx" might be more understandable. How do you feel?

>    <link rel="First" href="show_bug.cgi?id=[% bug_list.first %]">
>	[% prev_bug = this_bug_idx - 1 %]
>    <link rel="Prev" href="show_bug.cgi?id=[% bug_list.$prev_bug %]">

Why is the middle line indented?

Also, you should print out the nav links in order "Top, (Up), First, Prev,
Next, Last". While Mozilla doesn't care about the order, fe. Lynx does, and the
current order looks quite odd.

>[%# *** Bugzilla Adimistration Tools *** %]

Nit: Administration.

>  [%+ '  <link rel="Administration" title="Parameters"    href="editparams.cgi">' IF 

Lynx doesn't show these at all, probably because it doesn't recognize
Administration relationship. Can you look at some other browsers supporting
<link>'s and try to find a solution that would allow us to give out these
additional links as browser-independently as possible?

>  [%+ '  <link rel="Administration" title="Users"         href="editusers.cgi">' IF 

Also, I'd go for using "Edit xxx" as the title. Some browser might show them
without the rel attribute. In that case plain 'Users' is pretty odd.

I think you could try adding the footer queries to the link elements as well.
It may be that somebody considers that to be bloat, but I'd like to see how it
works...

Comment 16

[Jouni Heikniemi]

Attachment: trunk-tip patch to use site-navigation

We discussed this with tm on IRC; he can't access a trunk tip version, so I'll
attach a candidate for a trunk-tip-patch that makes header use
site-navigation.html.tmpl (we need to change the name by the way).

Comment 17

[Toms Baugis]

Attachment: site-navigation.html.tmpl (v3 - Fix + "Show.." & "Preset Queries")

This also includes:
1. Bug specific tools (dependency tree, votes, aso.)
2. Preset Queries

Comments:
>>    <link rel="First" href="show_bug.cgi?id=[% bug_list.first %]">
>>	[% prev_bug = this_bug_idx - 1 %]
>>    <link rel="Prev" href="show_bug.cgi?id=[% bug_list.$prev_bug %]">
>Why is the middle line indented?

to get index of previous variable in list, so we can properly address it in
next line. is there any other way how to do it?


>>[%# *** Bugzilla Adimistration Tools *** %]
>Nit: Administration.
:)


>Can you look at some other browsers supporting
><link>'s and try to find a solution that would allow us to give out these
>additional links as browser-independently as possible?
well, i don't know any other browser working on win2k and supporting <link>s
(i have no linux (yes, i know, shame on me :)  )

Comment 18

[Gervase Markham [:gerv]]

Toms: I hate to rain on your parade, but I think it's wrong to add so many
<link> elements when so few browsers support them, but everyone has to download
them. If we are to add some more, we should also only be adding elements for
pages which are directly related to the current one - so the "admin tools"
section should go. I'm also not convinced by the others. I think we should just
stick to a straight templatisation of the current code.

Also, you've got some dodgy indentation and spacing; you should probably check
the file over. Look at other templates for examples. Never use tabs, always use
a 2-space indent and line up matching directives.

Lastly, you do not want to unconditionally include the template in
header.html.tmpl. I suggest doing the following in edit.html.tmpl before
including the header template:

[% header_html = BLOCK %]
  Insert <link>-generating template fragment here
[% END %]

Gerv

Comment 19

[Toms Baugis]

gerv - what i understood is - let's just templatize existing features and do not
add additional ones.
personally me - i got very used to document -> more -> administration section,
because no more scrolling is needed, when you need to do some administrative tools.
site-navigation is just an addittion to existing functionality, based on other
templates code; an mozilla-specific feature.

anyway - you are right, but i would like to hear, what Jouni says about it.

i know that you all don't like options (all this y/n stuff in parameter page),
'cause they make only harder to configure, overlook and admin b-zilla, so maybe
admin part can be just commented out with "you can enable this and that by
removing these comments" ??
also - what harm can do this template if it is called out from header template
(if there is no buglist, then it will just skip all section anyway)?

also - other option - maybe a browser check can be done on these link elements?

Comment 20

[Jouni Heikniemi]

We definitely don't want a param for this. If we'll go with some
configurability, we should go with the "comment out this part of template"
approach. 

About the HTML bloat: I'm not sure about this. I greatly encourage the use of
<link> elements to support navigation, and I also think they should be used in
spite of not being supported in all browsers, because browser support will
develop only if there are sufficient uses for the tools. But this is not to say
that I'd use <link>s no matter what the cost. But still, their bandwidth usage
is not tremendous; I'd like to go the futuristic way and add the <link>s.

Gerv sez:

>we should also only be adding elements for pages which are directly 
>related to the current one 

You need to define the "directly related", really. I mean, I can pick up a
ridiculous bug list and have linkage between bugs that have no sensible
connection whatsoever, yet the link tools are useful - they are _navigation
tools_, just like the footer. If there is sufficient need for the admin tools
being <link>ed (and I really think I might use them!), I don't see much sense in
just sticking with "direct relations". But it's not like I had thought this out
thoroughly...

Comment 21

[Gervase Markham [:gerv]]

Grr. :-)

Ok, then. I give in.

Take my suggestion about including in comment #18.
Put the entire thing inside a browser check for Mozilla and iCab.
Param('usedependencies') is only checked in one irrelevant place; it's going
away. So don't use it.
My Bugs should use the Param.
"Votes for this bug" -> "Votes".
"Print-Ready Version of Bug" -> "Printer-friendly Version".
... and your indentation is still dodgy :-) 2-space indent, start at the very
left, move over when entering a block, otherwise stay where you are.

Gerv

Comment 22

[Jouni Heikniemi]

>Take my suggestion about including in comment #18.

If I read you correctly, you'd like to preserve header_html, right? I would like
to waste it totally. :-) I've been turned to agree with Myk et al.'s opinion *
about generic fields sucking. This means that IMO we don't need a "header_html"
field to cover for generic HTML being put into HEAD; if there's a specific need,
we should create a specific field (like "style"; but we have that already, too). 

*) Bug 148179 comment 2 and forwards for further information.

>Put the entire thing inside a browser check for Mozilla and iCab.

How adamant you are about this? Browser checks suck, because they don't check
for browser capabilities but rather brands (or, in case of HTTP_USER_AGENT
checking, obscure presentations of those). If I use a browser you have never
heard of, I'm not getting links if you check for only those who you happen to
know about. To demonstrate this effect in practice, you just forgot to support
Lynx which shows <link>s quite nicely. :-)

I'd go for printing <link>s unconditionally; otherwise we're just promoting the
painful phenomenon of lying the USER_AGENT in order to achieve something. If IE
6.1 starts supporting <link>s, it should get them even before we update our
regexp (or before IE 6 starts imitating Mozilla :-)).

>"Print-Ready Version of Bug" -> "Printer-friendly Version".

Unless there's a good reason, let's just use a small V in version, please :-)

Comment 23

[Gervase Markham [:gerv]]

What's the objection to generic fields? There are a reasonably large number of
things we could add to the head - having one for <link>, another for <meta> etc.
just seems like a waste of variables.

But, I've partly changed my mind about this. The <link>s which are globally
applicable, like the Admin ones, should be directly in header.html.tmpl. The
ones which should only appear on show_bug.cgi should be included via header_html
(or, if you must, a newly-added "link" parameter.)

> heard of, I'm not getting links if you check for only those who you happen to
> know about. To demonstrate this effect in practice, you just forgot to support
> Lynx which shows <link>s quite nicely. :-)

If there were some foolproof way of determining <link> support, we could use it.
But on non-b.m.o. installations, a lot of people use IE, and it needlessly
bloats _every_ page they download.

If you like, we could exclude common browsers known not to support <link>
instead - IEs 4 through 6, basically. 

>"Print-Ready Version of Bug" -> "Printer-friendly Version".

Unless there's a good reason, let's just use a small V in version, please :-)

We seem to be using most-words-capitalised in other links in that menu (Edit
Users, Preset Queries, Bug Activity)...

Gerv

Comment 24

[Jouni Heikniemi]

>What's the objection to generic fields? There are a reasonably large number of
>things we could add to the head - having one for <link>, another for <meta> etc.
>just seems like a waste of variables.

Well, there is the problem of using one variable for several purposes. It easily
gets fairly messy and leads to odd string catenations. Using a separate variable
for each field allows for more structure. 

But I'll CC myk in case he wants to comment something on this; although his
vacation might prevent him from doing so in a timely fashion...

>But, I've partly changed my mind about this. The <link>s which are globally
>applicable, like the Admin ones, should be directly in header.html.tmpl. The
>ones which should only appear on show_bug.cgi should be included via header_html
>(or, if you must, a newly-added "link" parameter.)

Note that some should also appear in buglist.cgi. But yes, I'm fine with this
approach, though I insist on the 'newly-added "link" parameter'. :-) Maybe with
the name of "link_elements", though. "link" is common enough to warrant a
namespace conflict at some point.

>If you like, we could exclude common browsers known not to support <link>
>instead - IEs 4 through 6, basically. 

If we have to have a browser detection (and granted, it sort of makes sense for
now), let's do this. Limited exclusion is better than limited inclusion.

>We seem to be using most-words-capitalised in other links in that menu (Edit
>Users, Preset Queries, Bug Activity)...

True, I didn't notice. It still sucks. Let's change them all! :-)

Comment 25

[Toms Baugis]

So..ummm...
Is the browser check really necessarry (in fact - i don't know how to do it in
templates (mailme))?

I'll do following:
1. Move just bug specific-features to buglist and editbug templates and pass
these links through the new parameter 'link_elements'
2. Will not change capitalization since it's normal to have first letter
capitalized in titles (and menu items are titles)

Comment 26

[Gervase Markham [:gerv]]

I agree on the capitalisation.

The sniffing would work something like this:
[% IF NOT (user_agent.match("MSIE [1-6]") OR user_agent.match("Netscape/4")) %]
  <link stuff>
[% END %]

I don't know the exact form of IE user-agent strings, so you'd have to check that.

Gerv

Comment 27

[Gervase Markham [:gerv]]

toms: are you still working on this? I just re-noticed that it needs doing :-)

Gerv

Comment 28

[Toms Baugis]

Attachment: Just a screenshot of b-zilla with site-navigation enabled

well, i'm really sorry about not noticing you before - my schedule currently is
overloaded (try to imagine lectures from 10:30-18:00 and then after - job?  :) 
 )

so, i would really appreciate, if someone could take this from me and fix
everything that remains and check it in (i'd love to see this in next version,
'cause i'm already fully using this functionality).


i thought, maybe it would be interesting to see how it looks now, so i added
screenshot of my b-zilla.

also - one idea - the "Prefs" link could also be put in site navigation

in hope to jump in the train of b-zilla later,
tm

Comment 29

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Tidied up version of Toms' patch.

Gerv

Comment 30

[Jouni Heikniemi]

Comment on attachment 99868 [details] [diff] [review]
Patch v.1

>+  [%# *** Up, First, Prev, Next, Last *** %]

"Bug list navigation"? This comment is fairly technical and besides, the rest
of the comments don't list the rels either (but rather their semantic
meanings).

>+     [% IF q.linkinfooter %]

Do you think this is a necessary condition? I could be persuaded to agree
either way, but it's not like <link>s had limited space the same way the footer
has. Of course, there's the issue with the bloat...

>+        <link rel="Preset&nbsp;Queries" 

You don't want an &nbsp; here. A normal space should do it, right?

>+              title="[% q.name FILTER html FILTER no_break %]"

We don't have a filter called no_break. But just remove the "FILTER no_break",
it won't matter. You can't reliably control the wrapping of titles anyway.

Comment 31

[Bradley Baetz (:bbaetz)]

You also appear to have lost the [% header_html %] stuff.

The up/down stuff needs to be conditional on [% IF bug %], too. I'm also not
certian that that works for processbug, where we do this in bits. Maybe the link
stuff should be in a separate heading?

Do we really want to duplicate the footer here?

This makes my ugly hack for bug 171493 to handle this stuff go away, which is good.

Comment 32

[Gervase Markham [:gerv]]

> >+     [% IF q.linkinfooter %]
> 
> Do you think this is a necessary condition? 

Yes - for bloat reasons.

> >+        <link rel="Preset&nbsp;Queries" 
> 
> You don't want an &nbsp; here. A normal space should do it, right?

Nope - because then it treats it as two separate rels, and you get two entries
in the More menu on the links toolbar. Sneaky.

> You also appear to have lost the [% header_html %] stuff.

In comment 22, Jouni said (and Myk agrees) that generic fields suck for this,
and we should have specific ones. So I killed header_html . No-one else uses it.

> Do we really want to duplicate the footer here?

Well, I thought we could be a little more restrained with the number of links -
but I was borne along by the enthusiasm of others ;-P.

Gerv

Comment 33

[Bradley Baetz (:bbaetz)]

Yeah, well, I think you got carried away too much :) You definately don't need
the admin ones, and for a rarely-used feature, I think we can skip the preset
queries, too.

Comment 34

[Toms Baugis]

> Yeah, well, I think you got carried away too much :) You definately don't need
> the admin ones, and for a rarely-used feature, I think we can skip the preset
> queries, too.

well, if you are going to delete some of links, maybe just comment them out with
"[%# Administrative tools - comment out if you want to ... %]"
and so on.
i found admin section very usefull, and i believe that many of others will too -
site navigation bar can substitute footer, it is always accessible (at least on
mozilla), no necessity to scroll down and it's more structurized and compact
than footer.

Comment 35

[Gervase Markham [:gerv]]

They only appear for the people who actually make use of them, and they only
appear on browsers which support them (mostly.) It's not like complex processing
is required to display them - just a few IF tests. The total size if you get the
full set must be 1k or so - hardly enormous.

myk, dave: what do you think?

Gerv

Comment 36

[Bradley Baetz (:bbaetz)]

Hmm. Fair enough, I suppose. Does this work from process_bug, though? If not,
then my bug 171493 patch may make it easier

Comment 37

[Bradley Baetz (:bbaetz)]

Actually, we don't currently output link stuff for process_bug, so don't worry
about that. (does someone want to file a bug on that?)

Comment 38

[Gervase Markham [:gerv]]

Attachment: Patch v.2

> Actually, we don't currently output link stuff for process_bug, so don't
worry
> about that.

I _swear_ we used to. But anyway - fixing this bug will fix that.

New patch attached. Is this worthy of review, if I've convinced you that having
them all is worthwhile?

Gerv

Comment 39

[Bradley Baetz (:bbaetz)]

Comment on attachment 101454 [details] [diff] [review]
Patch v.2

I guess that this is OK, if it WFY.

I'm going to do a bit of Bug.pm hacking to make this work, until Bug.pm does
modifications, because of process_bug. I guess I can handle that, though...

Comment 40

[Gervase Markham [:gerv]]

Fixed.

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.182; previous revision: 1.181
done
Checking in bug_form.pl;
/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.106; previous revision: 1.105
done
Checking in ./template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
 edit.html.tmpl
new revision: 1.21; previous revision: 1.20
done
Checking in ./template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v
 <--  header.html.tmpl
new revision: 1.18; previous revision: 1.17
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/site-navigation.html.tmpl,v
done
Checking in template/en/default/global/site-navigation.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/site-navigation.html.tmpl,v
 <--  site-navigation.html.tmpl
initial revision: 1.1
done

Gerv

Comment 41

[Bradley Baetz (:bbaetz)]

+    [% user.login = user.login FILTER url_quote %]
+    [% substs = { userid => user.login } %]  

Bah, missed this before checkin. You can't do this - PROCESS localised
variables, but it doens't do a deep copy, only a shallow copy. So changing
user.login affects other templates, and the footer now hows as 'bbaetz%40localhost'

Use userid intead?

Comment 42

[Bradley Baetz (:bbaetz)]

Oh, and this doesn't change the process_bug behaviour, for the same reason it
didn't work before - bug_form.pl is called way too late for this, because of our
process-in-bits code

Don't worry about that, though - I'll fix it when doing bug 171493.

Comment 43

[Gervase Markham [:gerv]]

bbaetz: I've fixed the issue you raised with accidental escaping.

Gerv