Reviewing Patches
One of the new Giggle features I am most proud of is the new patch view, which only shows one chunk at a time and let's you navigate between chunks using the file list, toolbar buttons or hotkeys (Alt+Up, Alt+Down). At the same time it also is the most criticised feature of the new Giggle. So I wonder how to improve that view.
I've introduced that feature because seeing the entire patch at a time distracts me easily. When reviewing patches I want to focus on one change at a time. Clearly not an option (in my opinion) is to introduce a switch to just show the full patch. I'd break that mode far too often cause I'd never use it. That opinion is based on Havoc Pennington's excellent article about "Free software UI". Don't miss the chapter "The Question of Preferences".
So I want to know:
- Why is seeing the entire patch important for you?
- How much information do you need from a patch at a time?
- Which ideas do you have for improving the patch view?
Keeping my own ideas for myself right now, as I am playing the "Ask the Audience" lifeline.
Audience says: use "LANG=C giggle", as we don't want gibberish.
If you mean by "only shows one chunk at a time" that you only show a single changed file from the patch at a time (that's what I see in the video), then I'm all for it. I don't like the gitk view which shows all patches to all files in one long view.
However, to allow both ways, maybe you could turn the file list (on the left) into a treeview: the root element would display the entire patch in gitk-like fashion, and the changed files would be listed as children (and clicking them would only show the changes for the single file).
I find that the patch cases vary for me, so I'd like both options.
Introducing a topmost item into the file list that says "All files" and displays the patch for all files together would be nice.
I see that the patches are generated with "diff -p". Maybe you could view all the consecutive chunks that have the same "function" listed after the @@ at the same time.
Also, I would like some more graphical representation of how many other chunks there are, and maybe how large each chunk is. Something similar to the scrollbar where the size of the bar represents the amount of the viewed chunk that is visible. Just listing "2 of 7" above the patch is kind of easy to miss.
Also, to be actually useful for reviewing patches I would like to be able to type in annotations at specific places in the displayed patch.
I quite like the tree view idea oliver suggested. That would actually allow you to step down in chunk sizes from whole patch (root element for those like Vadim) through files (then possibly functions as suggested by Alexander) and down to the chunks themselves.
You might also want a combo box somewhere nearby to specify step size for the up/down functions, allowing them to work on a file, function or chunk level.
This would also solve the visual indication issue as people know how to deal with tree views, whereas seeing a list with a single file and getting one short patch chunk could easily hide the magnitude of the changes made in all the other chunks. It certainly confused me the first time I saw it, whereas, I'd probably explore a tree view a bit to see what was going on...
But when using a treeview with subfolder structure and such, it easily gets wide and messy...
I like it this way :)
I wouldn't suggest a sub-folder structure either, that's best left to the browse view. I'd suggest just the three (or four) levels I mentioned in my comment.
I like how Google does it;
http://code.google.com/p/nanny/source...
Why not just use a gtksourceview with folding?
@asd: That's a great idea.
So that you can view 2 (or more) diffs at once.
I think the main issue is that the navigation between chunks is not discoverable -- try adding a wide button beneath the first chunk reading "Go to next change".
Furthermore, displaying the number of chunks next to the filenames on the left would make it easier to glance at how many changes there are to each of those files.
As others have noted, it's bad because the UI inconsistent: on the left side, a selector with 3 alternatives, but on the right side we have 10(?) different things to view. These clearly must match up.. Either subfolder view on the left side, or event better in my opinion, a flat hunk list, no file grouping. Other alternative is to have them collapsable on the other side.
Thinking a bit, I think I'd like the left side flat hunk list. And bonus: If you select two hunks, you view those two on the right side.. Selecting the whole list, and ta-da you have the whole patch!
I quite like Google Code's way. You could leave the next and last chunk buttons as a quick way to close the current one and expand the next chunk.
I both like it and dislike it. Sometimes I wish you could view the whole file at once (e.g. the hunking is not always meaningful). It's also not possible to navigate between two hunks that are not sequential without a lot of up/down, it would be nice if hunks were in the treeview under the file they're in. Maybe a menu option/keyboard shortcut/toolbar toggle for "show all changes to a file/show individual hunks"?
I dislike that the thing you show looks like it's the full patch (with the diff header immediately followed by the hunk), but it really isn't. Adding a single line between the header and the hunk (say, "hunk 1/7") would make that clear.
The arrows and the "Viewing 4 out of 7" text is too far away from the area they control. I'd first guess that they let me move to the next/previous changeset, and when that turns out not to be the case, I'd assume they move to the next/previous file.
I think the situation would be improved if you placed the file list on the right of the diff, so the up/down arrows were situated next to the hunk.
IMO the most usable view is a side-by-side view preferably with intra-regio changes. Besides that being probably out of scope for Giggle such a view consumes too much space (Meld is the only app I'm running maximized for good reasons).
But I like to view all changes for a file at once with a fair amount of context around each change. So as others already said, I'd prefer something like Google Code does, with shortcuts to jump to next and previous hunk/file.
For me there's currently an inconsistency in Giggle as it marks the current file in the list on the left, but I think a list of hunks is missing. "n of m" is less intuitive than a list showing where you are within a diff.
For that I'd prefer a flat list rather than something nested. Maybe it's enough to use the index entries from the diff. A combobox above this list to filter it to a single file (again with shortcut navigation) would make larger changesets accessible...
Thank you for all this useful and insightful responses.
Seems like my diff view doesn't properly visualize what happens. Also seems like I use all navigation options available with GtkTreeView. Have to play with the file list a bit.
Regarding a Google-like diff view: This would take quite some effort to implement, especially as folding support has not reaching GtkSourceView trunk yet. It's also to consider that this layout, doesn't utilize horizontal screen space as good as the current layout.
Alex: Annotations? Definitely a must have. See http://bugzilla.gnome.org/show_bug.cg...