135707 - normalize template filenames

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Template filenames are a mess and should be normalized.  For templates
supporting multiple formats the following convention has been established:

SCRIPTNAME.EXTENSION.tmpl (for the default format)
SCRIPTNAME-FORMATNAME.EXTENSION.tmpl (for other formats)

For other template files we talked about the following convention:

complete documents: NAME.EXTENSION.tmpl
document fragments: NAME.tmpl

As we begin to build more non-HTML templates, however, I think we'll find it
useful to identify the content-type of template fragments, so I suggest we do
the following:

complete documents: NAME.EXTENSION.tmpl
document fragments: NAME.EXTENSION.tmpl

One other note: In discussions we decided to use hyphens to separate multiple
words in template names, i.e. display-something.html.tmpl.

Comment 1

[Gervase Markham [:gerv]]

There are other things which need to be done globally to the template system
after all the templatisations are done, but before we release 2.16. Here's the
ones I can think of at the moment:

- Convert error arms of templatisation calls to ThrowTemplateError. This fixes
bug 1000092.
- Add INTERFACE comments. This is optional, but also low risk, so we should do
it if people have time.
- Make sure they all have license headers and version numbers.
- Rearrange the directory structure for localisation purposes.

Gerv

Comment 2

[Cédric Caron]

I don't know how this is working with linux but under windows the system use 
the file extention to start the corect editor.

naming the templates files NAME.tmpl.EXTENTION allow to still start the corect 
editor

ex:

mytextfile.tmpl.txt
myhtmlfile.tmpl.txt

Comment 3

[Gervase Markham [:gerv]]

Template files with .html.tmpl are not really HTML files. What happens when you
try and view them as HTML in your editor depends a lot on your editor...

It might be sensible to swap them - does anyone else have a view?

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

no, lets not swap them. They are a .tmpl file, really. I'm sure that an editro
could be set up to use a different mode for .html.tmpl than .tmpl on its own

Comment 5

[Dave Miller [:justdave]]

The final extension on a filename should indicate what application will EXECUTE
the file.  Since that's Template Toolkit in this case, that should be .tmpl.

If you want to edit them in a specific type of editor, edit your filetype
mappings to provide "Edit with XXXX" actions in the context menu for that
extension for each type you commonly edit.  This is pretty trivial to do in
Windows 95 and up.  I did that on my Win2K box at work to allow double-clicking
a Perl file (.pl) to still run it, but I can right-click on one and choose Edit
and it opens in Notepad.

From a Windows Explorer window, go to the Folder Options (View options depending
on the version of Windows), click the File Types tab, locate your .tmpl
extension (or create it if it's not there), click Edit.  Then click Add next to
the actions box, and follow the prompts.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: does the job

Here it is (except for what is being done over in 126908.  Review it before it
rots!

Comment 7

[Gervase Markham [:gerv]]

Comment on attachment 78253 [details] [diff] [review]
patch v1: does the job

I'm somewhat confused. The way I imagined this happening was as
follows:

- We create a directory template/en/.
- We re-check-in all the templates into that directory. As we check in
  each one, we make sure it's in the correct subdirectory (and the
  subdirectory names are sane), that the template uses all the correct
  names internally, and its own name is correct.
- Up to this point, Bugzilla continues to work as normal.
- Once we've got all that perfect, we change the CGIs, perhaps in
  batches, to point to the templates in the new subdirectory, and 
  change the template search path to look in the 'en' directory.
- We CVS remove the old copies of the templates.

This has the advantage that we can take a few days to get the layout
in the 'en' directory correct, without having this "gotta review it
before it rots" quickness. It also kills all three birds (changing to
the 'en' directory, fixing the filenames and fixing the subdirectory
names) with one stone.

Gerv

Comment 8

[Zach Lipton [:zach]]

Comment on attachment 78253 [details] [diff] [review]
patch v1: does the job

I agree with Gerv on this one. This is probably better done in several stages.
Perhaps we can move the files around on the repository to keep the cvs history
around or does it matter because the files are so new?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Ah, yes, I forgot about the localization plan.  Never mind. :-)

Comment 10

[Gervase Markham [:gerv]]

Attachment: New template structure

I won't paste the checkin comment, because it's both enormous and trivial, but
I just re-checked in all the templates according to the scheme agreed by
[e-mail] (attached.)

This in itself has no effect on Bugzilla whatsoever; next thing that will
appear here is a massive patch to make the templates internally consistent with
regard to their names. Then, we'll change the search path and start migrating
the CGIs.

Gerv

Comment 11

[Gervase Markham [:gerv]]

Attachment: Patch v.1

OK, here's the "fix the templates" patch. It does the following things:

- Changes all internal template filenames to point to the new names
- Adds version strings and license headers to all those templates without them
- Replaces almost all instances of INCLUDE with PROCESS (see below)
- Backs out two small changes which were part of a patch of mine (oops)

INCLUDE vs. PROCESS - the difference is that INCLUDE clones all the variables
in the namespace, and the INCLUDEd file can't affect them. PROCESS doesn't
bother. This means PROCESS is faster. In almost all cases, we don't change
variables in the relevant included template, and so can use PROCESS. I've
changed INCLUDE to PROCESS in all those cases. Every little helps :-)

Note to reviewers: this code is not live. The best way to find out if I've
messed this up in small ways is not to read this patch, but to change the
template search path and the CGIs and see what breaks - because any breakage
will be obvious. Therefore, I'm looking for a "yes, the approach is right"
review, not a "didn't you miss this filename in foo.html.tmpl" review.

Gerv

Comment 12

[Gervase Markham [:gerv]]

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

Comment 13

[Myk Melez [:myk] [@mykmelez]]

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

Index: template/en/default/index.html.tmpl

>-[% INCLUDE global/header
>+[% PROCESS global/header.html.tmpl

INCLUDE->PROCESS is a good and important change, but it's also
a different bug and should be done separately.


>Index: template/en/default/search/search.html.tmpl

>-
>-[%# The decent help requires Javascript %]
>-<script> <!--
>-   document.write("<p><a href='query.cgi?help=1'>Give me help</a> with this form (reloads page.)</p>");
>-// -->
>-</script>
>-<noscript>
>-  <p>
>-    <a href="queryhelp.cgi">Give me help</a> with this form.
>-  </p>
>-</noscript>

Hunh?



>Index: template/en/default/global/header.html.tmpl

>-    <script type="application/x-javascript">
>-    <!--
>-      [%+ INCLUDE "help/show-hide.js.tmpl" %]
>-    // -->
>-    </script>
>-        

Hunh?

Comment 14

[Gervase Markham [:gerv]]

> INCLUDE->PROCESS is a good and important change, but it's also
> a different bug and should be done separately.

It's a matter of running 
find ./ -name "*.html.tmpl" -print -exec perl -p -i -e "s/INCLUDE global/PROCESS
global/g" {} \;
over the tree (and then checking the INCLUDEs which aren't header and footer.)
It took me less than five minutes to do. Unless you are saying we shouldn't do
it in 2.16, this seems like the _ideal_ time - where I've got complete control
of all the templates and can't break anyone's patches or something like that.

> Hunh?

I said:
- Backs out two small changes which were part of a patch of mine (oops)

Those are them :-) I accidentally checked in my version of the templates instead
of a clean CVS pull. These are the artifacts of that mistake.

Gerv

Comment 15

[Myk Melez [:myk] [@mykmelez]]

>Unless you are saying we shouldn't do
>it in 2.16, this seems like the _ideal_ time - where I've got complete control
>of all the templates and can't break anyone's patches or something like that.

You wouldn't break anyone's patches if you did it right after checking in these
files either.  I'm not saying we shouldn't do this in 2.16 (although it's not
important enough to be a blocker), I'm saying it's a separate bug and should be
fixed in a separate patch.  I want to review the fixes for these two issues (as
in general) separately, and I want separate cvs log entries for them.

Comment 16

[Gervase Markham [:gerv]]

Attachment: Patch v.2

_Should_ be the same but without the PROCESS/INCLUDE stuff. But I haven't got
time to verify that, as I have to go to work :-)

Gerv

Comment 17

[David D. Kilzer (ddk)]

Attachment: Patch to fix template baseline

This patch adjusts the templates in template/en/default to
mirror the current CVS-tip template/default structure.	It
fixes the following templates:

template/en/default/bug/dependency-tree.html.tmpl
template/en/default/global/header.html.tmpl
template/en/default/search/search.html.tmpl

These include the "huhn?" stuff that myk found, plus some
changes to bug/dependency-tree.html.tmpl that were missed.

I think it would be best to establish a proper baseline
for the templates before we start writing patches for
them.

Comment 18

[Gervase Markham [:gerv]]

Comment on attachment 80024 [details] [diff] [review]
Patch to fix template baseline

<sigh> 2xr=gerv, checked in. 

It's easier for me to deal with the merge conflicts that this patch creates
than waste a day arguing that this is pointless, it makes no difference how we
do it, and just causes me merge hassle.

I will shortly post a v.3 patch, which will be indentical to v.2 except missing
the bits which I've just checked in.

Gerv

Comment 19

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Version 3 patch. This is like v.2 but missing the bits in attachment 80024 [details] [diff] [review].

Could this be reviewed quickly, please? :-)

Gerv

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 80037 [details] [diff] [review]
Patch v.3

You missed two INCLUDE->PROCESS transformations
inset-forgotten-password.html.tmpl.  Fix those and check it in 2x r=myk

Comment 21

[Gervase Markham [:gerv]]

Fixed. Excellent. 

Gerv

Comment 22

[Andreas Franke (gone)]

It's probably too late, but I think I should note it here anyway. IMO, the new
template structure has some inconveniences for developers and admins: 

1. The template files are located at several different levels of depth in the
directory tree. This makes it harder for everyone to grep for something in the
templates. Currently, you have to cd to template/en/default and then do a
        grep foo * */* */*/* 
if you don't want to miss any template files.

[1a. What's the story about these files:
        template/en/default/bug/CVS/Base/edit.html.tmpl
        template/en/default/bug/CVS/Base/navigate.html.tmpl
        template/en/default/bug/CVS/Baserev
 I have never seen them before; I accidentally found them by trying out a
        grep foo */*/*/*
]

2. Sometimes there are both template files *and* subdirs in a directory. This
makes it hard to understand what the things in a directory are if you are
looking at an alphabetically sorted directory listing (e.g. a generated with a
normal "ls -l" command): then directories are mixed with the files.

3. Filenames are often identical for completely different things. (They are in
different subdirectories, but you can't tell where they are from the filename
alone.) This seems to be less problematic than I thought at first, because grep
seems to be intelligent enough to show the full path up to the matching file,
even when doing something like
        find . -name '*.tmpl' -exec grep -l 'sel =' {} \;
But this means that when talking about a file, you must always include the path
to the file, or else it may be ambiguous. E.g. if you have a repository of
interesting small patches somewhere, it doesn't make sense anymore to name the
files "create.html.tmpl.diff" because that could mean different things, and
you'd always have to look at the file contents to find out the context. For this
case, you have to either include the subdir name in the name of the patch file,
e.g. like attachstatus_create.html.tmpl.diff, or have your patch repository
mirror the directory structure of the template/en/default directory.
On the other hand (the other extreme), all the template files have the .tmpl
extension even though it doesn't provide any additional information (since all
of them are somewhere in the "template" directory), but is useful to distinguish
them from all the other bugzilla files (e.g. cgis) which makes sense in
particular if you are talking about filenames without any context.

Comment 23

[Gervase Markham [:gerv]]

> 1. Currently, you have to cd to template/en/default and then do a
>         grep foo * */* */*/* 

You need the magic of the "-r" (recursive) option to grep. :-)
                
[1a. What's the story about these files:
        template/en/default/bug/CVS/Base/edit.html.tmpl
        template/en/default/bug/CVS/Base/navigate.html.tmpl
        template/en/default/bug/CVS/Baserev
 I have never seen them before; I accidentally found them by trying out a
        grep foo */*/*/*
]

Urk. I don't get those; but perhaps something odd is going on in that directory,
because I've seen funny things in there too. I got dmose to check out the
server, and all looks fine. So try repulling.

> 2. Sometimes there are both template files *and* subdirs in a directory. This
> makes it hard to understand what the things in a directory are if you are
> looking at an alphabetically sorted directory listing (e.g. a generated with a
> normal "ls -l" command): then directories are mixed with the files.

You need the magic of ls --color. And directories don't have extensions.

> 3. Filenames are often identical for completely different things.

I'd rather have this than the redundancy involved in bug/bug-create.html.tmpl or
the long names which would come from sticking them all in one directory.

> E.g. if you have a repository of
> interesting small patches somewhere, it doesn't make sense anymore to name the
> files "create.html.tmpl.diff" because that could mean different things, and

You need the magic of Patch Maker (http://www.gerv.net/software/patch-maker).
And you need to name patches after the bugs they fix, not the files they contain.

Hope that's helpful :-)

Gerv

Comment 24

[Andreas Franke (gone)]

Yes, it's helpful, thanks. I installed my own fileutils-4.0, and now it works :)
BTW, can you copy template/.cvsignore to template/en/ ?

Comment 25

[Gervase Markham [:gerv]]

Just to check: am I supposed to check .cvsignore in, in template/en?

Gerv

Comment 26

[Dave Miller [:justdave]]

yes.

Comment 27

[Gervase Markham [:gerv]]

Done.

Gerv