Neil's Place

October 31, 2005

9:57 PM Bugzilla's nonsensical patch review UI

Let's say you've created your very first Mozilla patch for some bug, for instance making Control+left and Control+right work properly (By the way, if this bug isn't fixed soon, Firefox is doomed to fail). You're fairly new to Mozilla development so you aren't very familiar with the whole process. You do know however that you need to create a patch and attach it to the necessary bug in Bugzilla. So you click on the handy link that says "Create a New Attachment". You fill in the bits about the file path, the description and check the "patch" checkbox indicating that you are submitting a patch. Then you get to a bit with a bunch of flags. It looks something like this:

You've read somewhere in a place you can no longer find that Mozilla code needs to be reviewed by someone before being applied, so you assume by the labels that the flags have something to do with reviewing the bug or patch. There are a bunch of flags, each with a dropdown menu, and two of those have a grey box beside them. You look in the menu of the "review" line and see that your choices are "+", "-" and "?". Confused, you wonder why there are just symbols and no descriptive text. You reload the page just to make sure the page loaded properly, and then, being a curious sort, you look at the source code to see that indeed, yes, those labels are what is intended to be there. Hmmm. Do the labels mean "Addition", "Subtraction" and "Whaaa..?". There's no description of what the flags mean, nor any link to any help.

But you aren't going to give up just yet. You try choosing the items from the list. The only thing of note is that the neighbouring grey box turns white when "?" is selected. Perhaps that's what you should choose, but it's the last item on the menu, so that seems unlikely. You'd imagine that the item you are supposed to choose is first on the list, or better, chosen by default. You choose "?" and notice that you can now type something into the now white box. But what should you enter? There is the label "Requestee" above the box. Since "Requestee" isn't a real word, it doesn't tell you much, although it implies from the form of this "word" that it is referring to some person. Maybe you are supposed to enter your name there. That would be strange though, since Bugzilla should know your name since you already logged in. Perhaps, you are supposed to enter someone else's name in there, such as the person who should look at your patch. Sadly, you're new here, and the only Mozilla person you are familar with is Asa, and you don't think he does anything with code. You aren't sure how to find out who the real Mozilla developers are that might be able to help you or how to contact them.

You look at the other flags but they are all similar in nature. No descriptive text and the same choices in the dropdown. Resigned, you just press Submit and hope somebody knows what to do. Two months later, you get a flurry of messages pertaining to the bug created by someone named Boris Zbarsky; although you don't understand the messages, after a while the bug somehow gets fixed. You wonder what it all meant, but you try downloading a new Firefox build and note with joy that the bug is indeed fixed, and Firefox isn't doomed after all.


In case you're wondering, to request a code review for a patch, you're supposed to choose "?" from the list beside "review", and then enter the email address of the person who you want to do the review in the "Requestee" field. It's usually fairly difficult to determine who you should enter unless you have been working with Mozilla code for a while, usually over a year or so, and it's usually about 2% easier when you have. You can also set the superreview flag in a similar way, and set various approval flags. The difference between a review and a superreview can be difficult to understand -- I certainly don't really know exactly -- but generally a superreview is just a regular review but done by someone wearing a cape. Approvals can only be done by people with cars.

Then there's a bug that I filed that has an entirely different set of flags, in this case 'first-review' instead of 'review' and 'second-review' instead of 'superreview'. I don't know why this is different, nor what it means. The first person to correctly answer this will receive a free box of gummy worms.

The + and - values for a review flag mean code reviewed and accepted versus rejected respectively. Now, it really doesn't make sense to be able to mark a patch as reviewed by the person submitting the patch, yet the UI still provides this option. The other thing the UI lets the submitter do is decide whether a patch should be reviewed or not, which would only be of value to an experienced developer -- these people sometimes submit works in progress as patches.

The additional more complex concern is what to enter in the "Requestee" field. There's no indication of who might be allowed to do such a review. There's actually a finite number of people from a list that can do a superreview, so the list might just be presented there for someone to choose from. For review, it could just select from a set of reviewers associated with the component the bug is filed in. Naturally, that won't work if the bug is misfiled, but the bug should actually be recategorized anyway. For instance, a bug in View Source would allow selection from one of the reviewers associated with the View Source component.

To go further, there's no real reason to be selecting a specific person to do a review. Instead, a suitable reviewer should be selected automatically either based on the component, or for the brave, by analyzing the patch to see which files are modified. For most components, this would generally work since there's only one reviewer doing reviews of that code anyway. For other things like layout bugs, the patches could just go into a queue for triage by someone who knows better who should be reviewing it. Of course, I'm not suggesting that the ability to select a specific reviewer be removed, for those people that know what they're doing, they should be able to select a specific person.

When selecting a reviewer, Bugzilla should provide an indication of how active a reviewer is and how many patches are in that person's queue. For instance, if a reviewer has fifty patches and hasn't reviewed any for two months, it probably isn't a good idea to choose that person. In fact, any automated reviewer selection should take this into account and select the reviewer who is most likely to actually do a review.

Thus, the UI for review should just have three options: Request code review from the default reviewer, Request code review from a specific person, and Don't request code review. The superreview could be similar, or for simplicity, only available to be specified if the code review option was set to 'specific person'. The approval flags should either be removed, if it's the case that unreviewed patches can't be approved anyway, or if approving unreviewed patches is allowed, the labels changed to clarify what you're actually requesting. For instance, instead of 'approval1.8rc2', there should be a checkbox that reads "Request approval for Mozilla 1.8 Release Candidate 2" with a link that describes what requesting approval actually means.

If you think these are good ideas, let's discuss and file enhancement bugs to improve Bugzilla's review request UI.

Comments ( 20 )

October 7, 2005

9:55 AM Scales

So I finally got around to finishing up the slider widget, which I called <scale> to make the implementation simpler. Here is an image of what it looks like under various platforms and themes.

No mac support yet, since I don't have access to one. But the other platforms are styled using the native theme. If you'd like to try them out, you need the patches in the bug.

Comments ( 4 )

October 2, 2005

Why I Hate Frameworks
Or, how to build hammers

September 13, 2005

10:22 PM How Templates Work XLIV - Conclusion

We have come to end of the guide on how XUL templates work. I hope people reading it have learned a bit more about templates and how to use them. Expect to find the whole series on the Mozilla developer documentation site soon, or even sooner if (hint, hint), someone offers to help format it.

The template guide was originally supposed to be a short series, perhaps eight to ten parts, but quickly grew into a 44 part monster that took three and a half months to complete. One main reason for doing the template guide through my weblog rather than post it to a tutorial section was because I wanted to try a new method of writing documentation where I would write in short pieces rather than large blocks at a time. Clearly, it was still a lot of work. If I write another guide like this, I'll make sure to keep it shorter.

Anyway, I hope you enjoyed the series 'How Templates Work'. Hopefully, the template changes I've been working on for the last half year will make it into a Mozilla build soon. This will make templates much more useful for the future, and, naturally, will require even more documentation.

Comments ( 8 )

September 12, 2005

5:29 PM How Templates Work XLIII - Additional Template Attributes

All the templates used so far have had the <template> placed inside the root element with the datasources attribute. However, you may use the template attribute to refer to a template located elsewhere within the document. This allows you to share the same template among two different parts of the user interface. Firefox uses this for bookmarks to display the bookmarks menu and the bookmarks toolbar, since the same template may be used to display the menu items in both cases. This is accomplished using a shared template and the clever use of multiple rules. To use this technique, place a template attribute on the root element set to the id of a template.

<listbox datasources="template-guide-photos5.rdf"
            ref="http://www.xulplanet.com/rdf/myphotos"
            template="photoTemplate"/>
...
<template id="photoTemplate">
  ...
</template>

This template will be shared with any other element that references the id "photoTemplate". The listbox here does not have any children, although it may do. If you did add children, they act just like the static content as if the template was present. However, it is possible to use different static content for each usage, even though the template is shared. The datasources and ref attributes also differ for each usage, so it is possible to use a shared template to display the same structure multiple times but with different starting nodes in each case. The generated content is always inserted into the root node, in this example the <listbox>, not inside the template.

Normally, the container and member variables are determined by the template builder automatically. The container or starting node variable is specified in the <content> tag inside a rule's conditions, while the member variable is determined by the value of the uri attribute inside the action body. It is also possible to be explicit about the two variables by using two attributes on the <template> element.

<template container="?first" member="?item">

The container and member attributes can be used to specify the container and member variables. This isn't particularly useful although there is a very slight optimization since the builder does not need to scan the action body looking for the member variable when compiling the rules. You might also use these attributes just to make the code clearer when using very complex rules.

One possible advantage is when using the simple syntax where you don't specify variables; instead you use the special 'rdf:*' syntax for the member variable and the container is implied. By using the container and member attributes, you can use specific variables instead.

<hbox datasources="template-guide-photos5.rdf"
      ref="http://www.xulplanet.com/rdf/myphotos">
  <template container="?start" member="?photo">
    <rule>
      <image uri="?photo" src="?photo"/>
    </rule>
  </template>
</hbox>

In this example, the ?photo variable can be used instead of 'rdf:*' (although you can use either even if you specify the member variable). This may make the code clearer as to its function. We could use the container variable ?start in the action body also. If you are using the simple syntax and want to use the container variable in the content, you must use the container attribute since there is no other way to refer to it. Note also that a <rule> element is needed here, otherwise the builder will think the container and member attributes are conditions to check.

Comments ( 0 )

September 10, 2005

6:00 PM How Templates Work XLII - Content Sorting

For content builders (templates that do not use flags="dont-build-content"), sorting is done using a different means. Instead of sorting on a variable generated during the template generation, sorting for content builders may only be performed using a triple pointing out of the member variable. There is no connection between the generated variables and the resource used for sorting, so it doesn't have to be one used in the template.

You specify the resource used for sorting with the sortResource attribute on the root element, as follows:

<hbox datasources="template-guide-photos5.rdf"
      sortResource="http://purl.org/dc/elements/1.1/title"
      sortDirection="ascending"
      ref="http://www.xulplanet.com/rdf/myphotos">
  <template>
    <vbox class="box-padded" uri="rdf:*">
      <image src="rdf:*"/>
      <label value="rdf:http://purl.org/dc/elements/1.1/title"/>
    </vbox>
  </template>
</hbox>

In this example, the generated items will be sorted by title. The sortDirection attribute specifies the sort direction and may be set to "descending" for a reverse sort. The sortDirection attribute functions similarly to how the tree builder uses it. With a tree builder, each column has its own sort specified using the sort attribute, and the sortActive attribute is used to indicate the column that is currently sorted. For other content, there are no columns and only one sort is applicable, so the sortActive attribute is not necessary.

When the template builder generates a result, the sort service uses the value of the sortResource predicate for the result to determine where in the content the generated output should be inserted. When the RDF datasource changes, and a new result is available, the sort service inserts the new content at the right location.

Unlike with trees, you can sort using a secondary resource with the sortResource2 attribute which is used in the same manner as the sortResource attribute. If the values for the sortResource predicate are the same for two results, the sortResource2 predicate is used to further clarify the order. You can only have one secondary resource, that is, there is no sortResource3 attribute.

To change the sorting for a template's generated contents, you can either change the sort related attributes and rebuild the template, or for listboxes and menus, you can call the sort service's sort method:

var listbox = document.getElementById("aListBox");
var sortService = Components.classes["@mozilla.org/xul/xul-sort-service;1"].
                    getService(Components.interfaces.nsIXULSortService);
sortService.sort(listbox, "http://purl.org/dc/elements/1.1/title", "descending");

This code will sort a listbox by title in a descending order. The arguments to the sort method specify the root node (the listbox), the sort resource and the sort direction.

Comments ( 0 )

September 5, 2005

10:29 PM How Templates Work XLI - Sorting Tree Results

Using a tree builder, you can sort the results in a tree by a column. To do this, place a sort attribute on a <treecol> element referring to the variable to sort by for that column.

<treecol id="name" label="Name" sort="?name" flex="1"/>
<treecol id="date" label="Date" sort="?date" flex="1"/>

In this example, the first column will be sorted by the ?name variable and the second column by the ?date variable. When the sort is ascending, the tree rows will be sorted in alphabetical order. When the sort is descending, the tree rows will be sorted in the reverse order. For natural sorting, the rows will be sorted according to the natural order in the RDF datasource. Only one column applies a sort at a time. If the tree is sorted by name, and the user clicks on the date column header, the sort will change to the date column.

There are two additional attributes used for sorting, which you may set on a column to specify the initial sort. These attributes are modified when the user changes the sort. The sortDirection attribute may be used to specify the initial sort direction for a column. Only one column should have this attribute set, as a tree may only be sorted by one column at a time. The value should be either 'ascending', 'descending' or 'natural'. This last value is the default if the attribute is not specified. The sortActive attribute may be set to true or false and specifies which column the tree is sorted by. Only one column should have the sortActive attribute set to true at a time. The tree will change both attributes as necessary automatically when the column headers are clicked or the tree is sorted by other means.

If you don't want to allow sorting by a certain column, you can leave out the sort attribute. Only specify this attribute on columns that you wish to allow the user to sort by.

Here is a complete example of sorting a tree.

The sort attribute should be set to the variable that holds the values to sort by. Usually, this would be the same variable that is used to generate the label for the cells in that column, however this is not actually necessary. For instance, in the example the second column sorts by date, but if you were to use a different variable such as ?description, assuming a <binding> set it, the tree would sort by the value of the description variable for each row. In almost all situations however, you would normally sort using the same variable used for the label value. However, one situation where this is not desirable is if the displayed values would not generate the correct order as there is a lower representation that is more accurate. For example, the date 'May 15' would appear after 'August 24' when sorted purely alphabetically but before it when sorted chronologically.

Another way to sort by dates is to use the the parseType="Date" construct in the RDF datasource. This marks a literal as being a date value rather than a string. The builder will recognize this and sort chronologically instead. This also has the advantage that the dates will be displayed according to the user's current locale (meaning that the date is formatted so as to be suitable for the user's language). Here is a sample of how to specify this in the RDF/XML datasource:

<rdf:RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
     xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
     xmlns:r="http://www.xulplanet.com/rdf/"
     xmlns:nc="http://home.netscape.com/NC-rdf#">
  <rdf:Description rdf:about="http://www.xulplanet.com/ndeakin/images/t/palace.jpg">
    <r:date nc:parseType="Date">1125966767295<r:date>
  </rdf:Description>
</rdf:RDF>

You can also specify parseType="Integer" for numbers which will allow sorting numerically. By specifing different types for different values, you can sort alphabetically, numerically or by date.

If you are using the simple rule syntax, there are no variables, so you need to specify the full predicate including the rdf: prefix in the sort attribute. For instance:

<treecol id="name" label="Name" sort="rdf:http://purl.org/dc/elements/1.1/title" flex="1"/>

Note that all of this discussion about sorting only applies to tree builders. For other elements or content trees, a different sorting mechanism must be used which will be discussed next.

Comments ( 5 )

August 28, 2005

5:46 PM How Templates Work XL - Sorting Results

The template content builder uses a separate component to insert generated nodes into the content tree. This is done when inserting the nodes when they are first created as well as when a new result is available. This additional component is called the sort service. It is responsible for determining where to insert nodes into the XUL document. Since the component is called the 'sort service' it is also used to sort the generated results. Since an RDF graph doesn't specify any order to results -- unless the items are in an RDF Seq -- the template builder will handle the results in any order. You may have noticed in the examples that results that are not in a Seq are not output in any particular order.

The sort service may be used to order the results in some particular order, generally, ascending or descending based on the value of some predicate pointing out of the result node. The sort service also supports a third sort order, natural order, which is the default. It causes items to appear without any extra sorting in the order they are added. However, if the results are items in a Seq they will appear in the order listed in the Seq. For instance, the photos are listed in the same order in this example as they appear in the Seq in the datasource.

This method of sorting a Seq works best for simple rule conditions since it is obvious how the starting ref relates to the end member results (they are just the children), or for extended syntax rules that follow a similar pattern. For more complex rules, this natural sorting will not work, because the sort service assumes that the starting ref resource is the container and the end results are the children. In this case, the natural order of the results will just be the order that the template builder generates the results.

For ascending or descending sorts, this doesn't matter, since it will ignore whether results are containers and just sort by a value, alphabetically or numerically depending on the type of data.

The sort service only applies to content builders. The tree builder uses a different and much simpler means of sorting since there is no content to insert. It supports the same three types of sorting, natural, ascending or descending. In the latter two sort types, the tree builder sorts by the value in a column. For instance, if the photos were displayed in a two column tree showing the title and description, you could sort by either title or description. The user can change the sort column and direction by clicking the column headers, however, you can programmatically change the sort as well.

Next, we'll look at some examples of tree sorting. We'll examine trees first because they are much simpler to handle, and you will usually want to support sorting on trees.

Comments ( 0 )

August 26, 2005

4:20 PM How Templates Work XXXIX - More on RDF Modifications

Often, a new RDF triple is created in the datasource which would only affect a template rule's bindings. Since the bindings section of a rule specifies conditions that may optionally match, the addition or removal or this RDF data would never be able to add or remove a new result. At the very most, the change would cause a label to be filled in with a value, or cleared when removing an RDF triple.

As described earlier, the builder first scans the conditions part of a rule to see if it would cause a change. After this, the bindings are examined. This is done whether the conditions produced a new result, removed one, or the content was not affected, since a binding could have affected any existing results. It's possible, for instance, for every existing row to be affected by a single triple being added to the datasource. Consider the following binding:

<binding subject="?start"
            predicate="http://www.xulplanet.com/rdf/categoryName"
            object="?name"/>

This binding involves a triple pointing out from the starting variable that has been used in these examples. The value for this binding will be the same for every result, so if the category name changes, every result will need to change. However, the builder can use a much simpler process for recalculating the results. Instead of regenerating the content for a result, the builder just looks for attribute values that involve the ?name variable. Those attributes are just recomputed, substituting the new value for ?name instead. This process is repeated for each result that would be affected.

When a template involves multiple rules, the same process is used for each rule as with one rule. As when generating the results initially, only the highest matching rule needs to be applied. The only extra complication to deal with in the multiple rule case is when a particular result's member resource already matches a rule, yet the new RDF triple would cause an earlier rule to match. Since the earlier rule takes precedence, the builder handles this by removing the old content first and then adding the new content.

Next we will look at how the template builder determines where to insert content.

Comments ( 0 )