Need a "+patches" view: report lists patches attached to bugs.

Slight (but important) clarification to the #1 use case. "Package maintainers may want to send patches upstream *or include them in ubuntu*". The latter is actually the more common use case for me (and perhaps other ubuntu maintainers).

I agree seeing patches carried by other distros may be useful, but is lower importance. Regarding patches in the upstream tracker, that might be useful if those patches are attached to bug reports that have launchpad bug reports attached to them. Seeing *all* patches in the upstream tracker seems like it could get cluttery.

Bryce, is your mockup at http://www2.bryceharrington.org:8080/X/Reports/patches.html to be considered basically the way you'd want this to work? I.e., is that just a rough idea, or is it actually exactly what you'd like to see?

One thing I'm concerned about is that it's not clear how to handle bugs that have multiple patches attached. Your display is organized by bugtask-per-patch, that is, it shows every bugtask for each bug that has a patch attachment. If a bug had 2 patch attachments, would it display 2 * num_bugtasks lines? You can see how this could get out of hand quickly :-).

I'll attach a screenshot that shows what's running in the branch right now. Note it displays the patch attachment's message as well as the bug summary. Of course, that makes things rather wide; a better solution might be to put the patch message and/or filenmae as a tooltip-style text popup for when one hovers over something that identifies the patch. But currently, there isn't really anything that identifies the patch itself, except the patch age column.

Anyway, screenshot attached. I'll next develop a rough mockup for an interface that is organized by patch, shows all bugtasks, but does not result in the horrible multiplying-lines problem. We should discuss by phone or IRC as soon as we can, though.

Couple minor UI notes on the screenshot while I have a second...

1) Presumably the patch message will link to that patch?

and

2) The message should appear at the end of the row, preserving consistency with existing bugs lists and indicating that this new information is the most relevant for the page.

Cheers,
deryck

Thanks, Deryck. (Yes on both.)

Bryce, attaching a screenshot with proposals for changes (I ended up circling back to your basic design, after trying to figure out a way to make the view solely patch-centric... that didn't work well, because the bugs and bug tasks have important information that the user probably needs to see too).

In the mockup you mention if it's worth having the person's name mentioned in the popup, and Bryce is wondering about the use case with multiple patches per bug. What about having a Patch Column at the end, and each patch is that little bandaid icon, so if a bug has 3 bandaids it has three patches attached to it, and then on hover you get name of submitter and description?

I am guessing that an ubuntu maintainer who has a trust relationship with another developer wouldn't mind seeing who submitted the patch so they can triage it in their mind better. "Oh, this guy always has good patches, let me look at that right away." or something.

On Thu, Jan 14, 2010 at 03:21:03PM -0000, Karl Fogel wrote:
> Bryce, is your mockup at
> http://www2.bryceharrington.org:8080/X/Reports/patches.html to be
> considered basically the way you'd want this to work? I.e., is that
> just a rough idea, or is it actually exactly what you'd like to see?

Pretty much. I would like to have a direct link to the patch itself,
but this is hard to do with launchpadlib.

Maybe linkify Package too.

I've pondered ideas on filtering the data down a bit. E.g. excluding
fix committed, incomplete w/out response, patches posted by the package
maintainer. But each of these ideas has an issue I'm not 100% sure
about.

> One thing I'm concerned about is that it's not clear how to handle bugs
> that have multiple patches attached. Your display is organized by
> bugtask-per-patch, that is, it shows every bugtask for each bug that has
> a patch attachment. If a bug had 2 patch attachments, would it display
> 2 * num_bugtasks lines? You can see how this could get out of hand
> quickly :-).

Agreed about the multiple bugtask issue, which I run into with all my
cross-package reports. It's a hard problem since if you decide to just
do one bug per line (rather than bugtask), then it breaks sorting by
packagename.

I handwave it by saying if a bugtask is filed against multiple of my
packages it's either wrong (and should be fixed), or deservedly that
much more important. (As an example, after seeing how bug #258038 was
marked against 6 of my packages, I put it on the todo list for UDS and
we are now undertaking the hard work of implementing it in Lucid.)

Regarding multiple patches per bug, what I've done in this report is
ignore all but the most recently posted. Here's my reasoning. In most
cases where I've seen multiple patches, the prior patches were just
earlier variants of the same patch, and only the latest one was relevant
for review/upload. In cases where there were multiple patches to deal
with, that would be clear from the bug's context and more than one entry
in this table would be unnecessary.

One idea for displaying multiple patches might be to display the patch
icon multiple times on the line, one for each patch.

> I'll attach a screenshot that shows what's running in the branch right
> now. Note it displays the patch attachment's message as well as the bug
> summary. Of course, that makes things rather wide; a better solution
> might be to put the patch message and/or filenmae as a tooltip-style
> text popup for when one hovers over something that identifies the patch.
> But currently, there isn't really anything that identifies the patch
> itself, except the patch age column.

Heh, in an earlier version of the patch report I also included the patch
title. But like you I found it took up way too much space. Also, I
found in practice I really didn't care about what the patch was named,
and I removed it from the list in favor of the bug title. Here's the
reasoning I used: A) As a triager/developer my brain is wired more to
recognize the bug title; B) too often users select names for patches
that aren't very helpful ("patch" or "package-1.2.3-4ubuntu5.debdiff");
C) If an update to the patch is ...

After phone conversation with Jorge:

Propose that when there are multiple patches, we show the youngest patch's age, along with a "..." to indicate that there are other, older patches in that bug. It's really not necessary to show every patch individually, since the first place the user is going to go, once they're interested, is the bug report anyway. See attached screenshot with proposal.

On Thu, Jan 14, 2010 at 07:16:50PM -0000, Jorge O. Castro wrote:
> In the mockup you mention if it's worth having the person's name
> mentioned in the popup, and Bryce is wondering about the use case with
> multiple patches per bug. What about having a Patch Column at the end,
> and each patch is that little bandaid icon, so if a bug has 3 bandaids
> it has three patches attached to it, and then on hover you get name of
> submitter and description?
>
> I am guessing that an ubuntu maintainer who has a trust relationship
> with another developer wouldn't mind seeing who submitted the patch so
> they can triage it in their mind better. "Oh, this guy always has good
> patches, let me look at that right away." or something.

Wow, I should have read your comment before posting mine. :-)
Yes, complete agreement.

kfogel writes:
> Bryce, attaching a screenshot with proposals for changes (I ended up
> circling back to your basic design, after trying to figure out a way to
> make the view solely patch-centric... that didn't work well, because the
> bugs and bug tasks have important information that the user probably
> needs to see too).

I like this. And I agree with all your notations. I include the
summary as a sortable item only due to the generality of the table
sorting javascript I'm using; sorting by summary has no usefulness.
Hover-over to show patch info is sweet.

Regarding showing the patch submitter as a column, I do think it is
useful info but I'm torn as to whether it's worth devoting a column to
it. On the pro side: A) You could sort by submitter, to make it easy to
find all of a particular person's submissions, B) It'd clue you in that
a patch is just from another package maintainer (maybe yourself) so no
need to review it, C) it'd imply a trust measure as jorge mentions. On
the con side is that it'd add another wide column.

Hmm. Would it be possible to display just an icon for the person? If
so, I think that would give the benefits without sacrificing so much
space. If not, then maybe omit showing the patch submitter for now
(just include in the popup) and see if there's a big demand for it at
roll out and decide then?

Bryce

Well, my feeling is if you can't identify the person from the icon, there's no point showing it -- it would add no information, since the viewer already knows the patch was submitted by some person. So I think for now I'll just go with putting it in the popup (maybe along with the person's karma, riffing off an idea from beuno).

Notes from discussion with Martin Albisetti:

- obviously, importance/status columns should use the standard colors (see, e.g., https://bugs.edge.launchpad.net/bzr)
- for patch age, just show youngest and have it link directly to the patch itself, for quick-deep inspection
- we can consider putting number of other (older) patches in parens to the right of the patch age, but that might be confusing
- in package column, link package name to the list of open bugs (or maybe hot bugs) for that package

> - in package column, link package name to the list of open bugs (or maybe hot bugs) for that package

Or perhaps a list of bugs-with-patches for just that package?

And the bugs column should do sort-by-heat, maybe? Just thinking out loud.

Single digit numbers will be hard to click. How about putting the patch icon next to the number?

Martin, just FYI: we're using the "human readable time durations" thingie now, so the patch age is much wider (e.g., "8 days" instead of just "8"). See attached screenshot. Also notice that pages are batching now; in the URL, you can see where we set the batch size to 2, for testing purposes, but of course the default will be 50.

This is great Karl, it's looking super good. Thanks for solving it.

Bryce (and Jorge and Martin, for that matter):

Please see the new mouseover popup contents in the attached screenshot. Let us know if there's any other information you think should be in that popup.

Bah, wrong screenshot above -- try this one, please.

That looks great to me!

On Thu, Jan 21, 2010 at 05:43:13PM -0000, Jorge O. Castro wrote:
> That looks great to me!

I love it, this is really coming along!

Do you think having the date on that popup might be of use? I know that
you can figure it out based on the patch age, but might be useful
particularly for some of the older patches.

Bryce

@Martin: It turns out that having the mouseover region be the entire table cell is important. If we use something smaller (such as just the "8 days ago" link, or a patch band-aid icon), then the user can't move the mouse pointer down into the popup box to click on one of the links in there -- because as they move down into it, it disappears, because they've left the small region in which the popup is kept active.

So IMHO we should just keep the table cell as the active region. It takes a bit of getting used to, but it has the right behavior (more or less) once one is no longer surprised by it.

Abel and I just thought of something even better! We're going to try this:

Make the mouseover the small area (the "8 days ago" text), but the mouseout region will be the larger region -- the table cell. Thus the popup won't appear in a surprising way, but it will behave nicely once it does appear: the user would be able to move the pointer into the popup.

We'll keep you posted.

This didn't make it in by the close of pqm for release. We're pretty much done, so it will land as soon as pqm reopens after rollout and be available from edge by the Ubuntu platform sprint next week.

Filed bug #512498 about displaying a special message if no patches to show, and bug #512500 about sorting on patch age.

Next step: we need to make this work for Product, ProductSeries, DistroSeries, DistributionSourcePackage, Distribution, SourcePackage, and Person/Team.

It seems that IMilestone, IProjectPublic, IBugTarget inherit from IHasBugs, so we should hang our implementation off of IHasBugs in lib/lp/bugs/browser/configure.zcml, instead of IBugTarget. (IMilestone, IProjectPublic, IBugTarget all seem to inherit from IHasBugs, so it should be fine.) The comment formatter is probably going to mess this up, but the relevant block in .zcml is:

        <browser:page
            name="+patches"
            for="lp.bugs.interfaces.bugtarget.IBugTarget"
            class="lp.bugs.browser.bugtarget.BugsPatchesView"
            facet="bugs"
            permission="zope.Public"
            template="../templates/bugtarget-patches.pt"/>

The only assumption we make about our context is that it provides the method searchTasks(), and this method is defined in IHasBugsBase, so there should be no problem with the change of the base class.

We'll need tests for Milestone, Project (in the sense of "project group") and bug targets -- namely Product, ProductSeries, DistroSeries, DistributionSourcePackage, Distribution, and SourcePackage.

Regarding Person / Team:

In another branch, we should add an implementation for person pages, so that people and teams can see the patches for bugs they are "interested in". Person.searchTasks() already exists, so adding a ~somebody/+patches view should be quite simple. We can probably use the existing browser class.

(Above notes partly taken from Abel Deuring's research -- thank you, Abel!)

Comments on the above from Jorge Castro and Bryce Harrington:

  <kfogel> bryce, jcastro: regarding the contexts in which the
            "+patches" view should work, see
            https://bugs.edge.launchpad.net/malone/+bug/506018/comments/26
            and let me know if you have any thoughts on those. (Like,
            is it important for Persons and Teams, for example?)

  <jcastro> it's important for teams

  <jcastro> I would say it's more important for teams than individuals,
            thought ~jorge/+patches or whatever would be useful too

  <jcastro> kfogel: I would say when someone is working on a package
            they would prefer the package view

  <jcastro> but when they get some free time or a new contributor they'd
            want to see all packages their team is responsible for

  <bryce> kfogel, yeah I'd agree with jcastro

  <bryce> jcastro, hey btw I have done something you will find cool

Fixed in db r8961 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/db-devel/revision/8961>