216707 - Patch Viewer needs user documentation

Status: RESOLVED FIXED

(Bugzilla :: Documentation, defect, P1)

Description

[John Keiser (jkeiser)]

Patch Viewer has recently landed and needs user-level documentation.  An HTML
version of them can be found at
http://www.johnkeiser.com/mozilla/Patch_Viewer.html .  They need to be converted
to the XML format used in Bugzilla.

Comment 1

[John Keiser (jkeiser)]

Attachment: Docs

OK, here they are.

Comment 2

[Jacob Steenhagen]

Comment on attachment 130096 [details] [diff] [review]
Docs

>Index: xml/using.xml
>===================================================================
>+      integrationg with Bonsai, LXR and CVS></para>

You have a > instead of a .

>+      <section id="seeing_difference_between_two_patches">
>+        <title>Seeing the Difference Between Two Patches</title>
>+        <para>To see the difference between two patches, you first view the
>+        newer patch in Patch Viewer, and then select the older patch from the

Gramatically speaking, I don't think there's supposed to be a comma after
"Viewer." I would recommend breaking this up into two different sentences, but
it is up to you.

>+        dropdown at the top of the page ("Differences between [dropdown] and
>+        this patch") and click the "Diff" button. This will show you what

Is there a reason for having parenthsis and quote marks there?

>+      <section id="getting_more_context_in_a_patch">
>+        <title>Getting More Context in a Patch</title>
>+        <para>To get more context in a patch, you put a number in the textbox at
>+        the top of Patch Viewer ("Patch / File / [textbox]) and hit enter.

There's a mismatched quote.

>+      <section id="collapsing_and_expanding_sections_of_a_patch">
>+        <title>Collapsing and Expanding Sections of a Patch</title>
>+        <para>To view only a certain set of files in a patch (for example, if a
>+        patch is absolutely huge and you want to only review part of it at a
>+        time), you can click the "(+)" and "(-)" links next to each file (to
>+        expand it or collapse it). If you want to collapse all files or expand
>+        all files, you can click the Collapse All and Expand All links at the

For conisitancy sake, you should either put quotes around Collapse and Expand
All or not around the +/-.

I would also choose shorter section id's. My recommendation would be something
along the lines of (I prepend  "patchviewer_" to help guarntee uniqueness):
patchviewer_context, patchviewer_collapsing, etc.

These are mostly minor, but I think I would like to see them at least addressed
before granting review :)

(Again, sorry for the long timespan)

Comment 3

[John Keiser (jkeiser)]

Attachment: Docs v1.1

This fixes all comments except "why do you have that sentence both
parenthesized and quoted?"  It is parenthesized because it is an aside, and it
is quoted because it represents text on the screen--without the quotes it can
be confused as instructions rather than text to look for.

Comment 4

[John Keiser (jkeiser)]

Comment on attachment 134502 [details] [diff] [review]
Docs v1.1

Asking kiko so this might make the next release.

Comment 5

[Christian Reis]

Comment on attachment 134502 [details] [diff] [review]
Docs v1.1

I would have written the opening paragraph starting with a positive comment,
but the text is fine as it is :-)

Comment 6

[Christian Reis]

Comment on attachment 134502 [details] [diff] [review]
Docs v1.1

Comments on my wishlist, you may integrate before checking in if you like. 
They don't block review at all.

>+      <para>There has been some difficulty viewing and reviewing patches in
>+      Bugzilla due to lack of context and complexity of reading the raw
>+      patches. Patch Viewer is an enhancement to Bugzilla designed to
>+      fix that by offering increased context, linking to sections, and
>+      integrationg with Bonsai, LXR and CVS.</para>

I would rewrite the opening statement as:

  Viewing and reviewing patches in Bugzilla is often difficult due to 
  lack of context, improper format and the inherent readability issues 
  that raw patches present.

>+      <simplelist>
>+        <member>View patches in color, with side-by-side view rather than trying
>+        to interpret the contents of the patch</member>
>+        <member>See the difference between two patches</member>
>+        <member>Get more context in a patch</member>
>+        <member>Collapse and expand sections of a patch for easy
>+        reading</member>
>+        <member>Link to a particular section of a patch for discussion or
>+        review</member>
>+        <member>Go to Bonsai or LXR to see more context, blame, and
>+        cross-references for the part of the patch you are looking at</member>
>+        <member>Create a rawtext unified format diff out of any patch, no
>+        matter what format it came from</member>
>+      </simplelist>

I would add periods to the end of the items here.

>+      <section id="patchviewer_view">
>+        <title>Viewing Patches in Patch Viewer</title>
>+        <para>The main way to view a patch in patch viewer is to click the
>+        "Diff" link next to a patch in the Attachments list on a bug. You may

Perhaps 

  click *on* the "Diff" link

>+      <section id="patchviewer_context">
>+        <title>Getting More Context in a Patch</title>
>+        <para>To get more context in a patch, you put a number in the textbox at

Perhaps

  you may put / you should put

to match the other paragraphs.

>+        the top of Patch Viewer ("Patch / File / [textbox]") and hit enter.

(Why doesn't this have "Context:" or "C:" labelled in the UI? :-)

>+        This will give you that many lines of context before and after each
>+        change. Alternatively, you can click the "File" link there and it

Perhaps

  click on the "File" link

>+      <section id="patchviewer_link">
>+        <title>Linking to a Section of a Patch</title>
>+        <para>To link to a section of a patch, for example if you want to be
>+        able to give someone a URL to show them which part you are talking
>+        about, you simply click the "Link Here" link on the section header. The

You used a parenthesis to denote your example in the last section; maybe it
makes sense to use the same notation here. This is perhaps my greatest
überübernit of all times <wink>

>+      <section id="patchviewer_bonsai_lxr">
>+        <title>Going to Bonsai and LXR</title>
>+        <para>To go to Bonsai to get blame for the lines you are interested in,
>+        you can click the "Lines XX-YY" link on the section header you are
>+        interested in. This works even if the patch is against an old
>+        version of the file since Bonsai stores all versions of the file.</para>

I would add a comma before Bonsai there.

Comment 7

[John Keiser (jkeiser)]

Attachment: Patch v1.2

This is the patch I just committed.

Comment 8

[John Keiser (jkeiser)]

Checked in.