Merge lp:~wallyworld/launchpad/recipe-find-related-branches into lp:launchpad

Hi Ian,

This looks like I nice addition, but I wonder if it's necessary to list the branch URLs or if it'd be enough to have just the branch's name (linking to the branch itself) and the series/package name (also linking to the relevant thing). Something like

v Related branches[1]

 ------------------------------------------------------------------------
 | From source packages | Owner |
 ------------------------------------------------------------------------
 | _firefox-4.0_ from the _We're back in the game_ series | TheOwner |

 -----------------------------------------------------------------
 | From product series | Owner |
 -----------------------------------------------------------------
 | _bzr-2.2_ from the _Bazaar 2.2_ series | TheOwner |

That makes (what seems to me to be) the important information more prominent, avoids the repetition, provides a link to the series/packages themselves and allows us to use similar table headings for both tables (having one table list the owner and the other just the source package seemed a bit weird to me, but maybe there's a reason for doing so?).

Also, do you really need the fixed width on the table? Shouldn't the surrounding boxes of the launchpad-form limit the width of the table? Or is this because you want them to have the exact same width as the recipe text area?

[1] notice the different capitalization, as recommended by https://dev.launchpad.net/UserInterfaceWording

The column width issue is the same one that the milestone and project packages pages had to solve. User really do need the columns to be the same size and aligned to scan the page. But as Salgado notes, the columns do not have the same content. This is a bit like the related packages problem (see https://launchpad.net/~wgrant/+related-software for packages and ppa). This data issue might not be solvable now, but it would be nice to have the same column headings.

I think Salgado has an interesting point about showing the URL
I suspect such a change will frustrate users who copy the branch URL from the listing. But I wonder. Why is the owner missing from the product URLs?

So I think Salgado is asking the right question. I would rephrase it as "Why isn't the data symmetric?"

On Tue, 2011-01-25 at 20:07 +0000, Curtis Hovey wrote:
> The column width issue is the same one that the milestone and project
> packages pages had to solve. User really do need the columns to be the
> same size and aligned to scan the page. But as Salgado notes, the
> columns do not have the same content. This is a bit like the related
> packages problem (see https://launchpad.net/~wgrant/+related-software
> for packages and ppa). This data issue might not be solvable now, but
> it would be nice to have the same column headings.
>
> I think Salgado has an interesting point about showing the URL
> I suspect such a change will frustrate users who copy the branch URL
> from the listing. But I wonder. Why is the owner missing from the
> product URLs?

I can see why people would copy the branch URL if they wanted to use
that in the recipe but I was under the impression the main use case here
was just to show stuff that the user might like to know rather than to
aid them in crafting the recipe. That was my understanding after
reading https://bugs.launchpad.net/launchpad/+bug/670452/comments/2 but
maybe I misunderstood or the main use case is to aid people writing
recipes?

> Hi Ian,
>
> This looks like I nice addition, but I wonder if it's necessary to list the
> branch URLs or if it'd be enough to have just the branch's name (linking to
> the branch itself) and the series/package name (also linking to the relevant
> thing). Something like
>
> v Related branches[1]
>
> ------------------------------------------------------------------------
> | From source packages | Owner |
> ------------------------------------------------------------------------
> | _firefox-4.0_ from the _We're back in the game_ series | TheOwner |
>
> -----------------------------------------------------------------
> | From product series | Owner |
> -----------------------------------------------------------------
> | _bzr-2.2_ from the _Bazaar 2.2_ series | TheOwner |
>
> That makes (what seems to me to be) the important information more prominent,
> avoids the repetition, provides a link to the series/packages themselves and
> allows us to use similar table headings for both tables (having one table list
> the owner and the other just the source package seemed a bit weird to me, but
> maybe there's a reason for doing so?).
>
> Also, do you really need the fixed width on the table? Shouldn't the
> surrounding boxes of the launchpad-form limit the width of the table? Or is
> this because you want them to have the exact same width as the recipe text
> area?
>
> [1] notice the different capitalization, as recommended by
> https://dev.launchpad.net/UserInterfaceWording

Thanks for such a detailed comment - really appreciated. The reason for showing the branch urls is to allow the user to explicitly see what the urls are and so to allow simple copy and paste into the recipe - I believe this to be one of the key drivers for this work; when creating a recipe, it's the most error prone bit and the info is not easily intuited unless it is displayed. The branch name is clear from reading the url hence I didn't see the need to show it.

The reason for showing the owner of the product series branches is that it is not obvious from the url who the owner is. The source package column in the source package branches table is to allow easy navigation to the source package home page. These choice were based on implementation discussions when the work was commenced, however I am more than happy to show whatever information is deemed necessary. If we want to continue showing the urls for the reason mentioned above, it's tricky showing too many columns given the urls are so long and they look terrible when wrapped.

Before I added the fixed table width, the tables expanded out to the right hand edge of the browser window and looked terrible - I'm not sure why the form didn't limit the width. Having the tables a similar width to the other main recipe form widgets (the description text area in particular) seems to look good to me but that's only an opinion.

I'll fix the capitalization.

> The column width issue is the same one that the milestone and project packages
> pages had to solve. User really do need the columns to be the same size and
> aligned to scan the page. But as Salgado notes, the columns do not have the
> same content. This is a bit like the related packages problem (see
> https://launchpad.net/~wgrant/+related-software for packages and ppa). This
> data issue might not be solvable now, but it would be nice to have the same
> column headings.
>

Do we want the same styling to be used here as on the related software page listed above? That would have been my preferred way of doing it - bold table heading text etc. I just used the CCS class "listing" for the table style because I thought that was the correct thing to do. It would be good to change the look of my page to match the one referenced.

> I think Salgado has an interesting point about showing the URL
> I suspect such a change will frustrate users who copy the branch URL from the
> listing. But I wonder. Why is the owner missing from the product URLs?
>
> So I think Salgado is asking the right question. I would rephrase it as "Why
> isn't the data symmetric?"

I agree it would be better to have the same column headings etc. The main reason the two tables have different headings is that the branch urls are so long and I didn't want them to wrap and so was limited in how much space could be devoted to adding other columns. Initial implementation discussion resulting in the key info being chosen for display for each branch type and although I could have displayed owner for source package branches (for example), decided not to in order to limit the required table width.

When I didn't limit the table width, the tables came out very wide - see the screenshot from the original implementation: http://people.canonical.com/~ianb/recipe-related-branches.png
The screenshot lacks the vertical space between the tables, and the columns are each very wide and so the tables could easily accommodate more columns with extra info being added, but I think it the ui looks terrible given the tables are so much wider than the other data entry widgets on the recipe form??

If we are happy to have the tables wider, I could look to make the data in each symmetric....

I think it's good to have the links to the SourcePackage, but we should also have them for the ProductSeries. Maybe that could replace the owner column? Or is the owner really important for some reason?

I thought the text area widgets were constrained by the width of the surrounding elements, but I was wrong; zope's TextAreaWidget uses, by default, a 60-cols width. I'd also prefer if the tables had the same width as the other form elements, but I'm not sure using a fixed width on the table is a good idea because the widget's width might change but most importantly because I don't think the fixed-width table will behave the same way as the textare widget when the window is resized to a smaller size than what's needed to fit the textarea.

> I think it's good to have the links to the SourcePackage, but we should also
> have them for the ProductSeries. Maybe that could replace the owner column?
> Or is the owner really important for some reason?
>

I've added links to the product series instead of owner. I've also made the ui look better by including <h2> headings for each table. These headings and the table headers etc are all styled with the "code" domain css. Hence the headings appear in gold etc. New screenshot: http://people.canonical.com/~ianb/recipe-related-branches2.png

The ordering for the product series branches is based on the sorted_active_series_list property provided by the SortSeriesMixin. This provides only active series with the most recent first and the development focus at the top of the list.

I realised when doing these changes that I was exposing private branches and others the users had no right to see. So I fixed the implementation and added a couple of extra tests.

> I thought the text area widgets were constrained by the width of the
> surrounding elements, but I was wrong; zope's TextAreaWidget uses, by default,
> a 60-cols width. I'd also prefer if the tables had the same width as the
> other form elements, but I'm not sure using a fixed width on the table is a
> good idea because the widget's width might change but most importantly because
> I don't think the fixed-width table will behave the same way as the textare
> widget when the window is resized to a smaller size than what's needed to fit
> the textarea.

Before doing the fixed width tables, I did check the behaviour of the widgets' layouts when the browser window was resized and it all behaved as expected - the text area widget and tables retained their width and a simple horizontal scroll bar was added to the browser window.

What I've done now is to make the <fieldset> element which contains everything for the related branches widget a fixed size and remove the width styling from the tables. The resize behaviour is the same as described above but I think it's a cleaner implementation.

On Thu, 2011-01-27 at 12:57 +0000, Ian Booth wrote:
[...]
> > I thought the text area widgets were constrained by the width of the
> > surrounding elements, but I was wrong; zope's TextAreaWidget uses, by default,
> > a 60-cols width. I'd also prefer if the tables had the same width as the
> > other form elements, but I'm not sure using a fixed width on the table is a
> > good idea because the widget's width might change but most importantly because
> > I don't think the fixed-width table will behave the same way as the textare
> > widget when the window is resized to a smaller size than what's needed to fit
> > the textarea.
>
> Before doing the fixed width tables, I did check the behaviour of the
> widgets' layouts when the browser window was resized and it all
> behaved as expected - the text area widget and tables retained their
> width and a simple horizontal scroll bar was added to the browser
> window.

I thought the number of columns on a textarea wouldn't prevent the
browser from resizing it when the window is resized. That's the
behaviour I see with the comment box (which specifies cols=60) on a
merge-proposal page, but I tested here on a html file I crafted and the
browser didn't resize my textarea. Maybe Curtis will know why we're
seeing a different behaviour on the merge-proposal page.

>
> What I've done now is to make the <fieldset> element which contains
> everything for the related branches widget a fixed size and remove the
> width styling from the tables. The resize behaviour is the same as
> described above but I think it's a cleaner implementation.

Do you really think we should go with the fixed width, even with all its
drawbacks?

>
>>
>> What I've done now is to make the <fieldset> element which contains
>> everything for the related branches widget a fixed size and remove the
>> width styling from the tables. The resize behaviour is the same as
>> described above but I think it's a cleaner implementation.
>
> Do you really think we should go with the fixed width, even with all its
> drawbacks?
>

Without the fixed width, the tables expand right out to the edge of the
browser window, and on a screen with the browser maximised for example,
it looks really bad (to my eye) when all the form elements are around
the same width (approx size of text area widget) and just the related
branches tables are really wide. In my view, it's better visually to try
and maintain a similar width for all the form widgets and a consistent
resize behaviour ie if the text area widget is fixed, the the tables
(via the width styling on the fieldset element) should be too. When the
tables are really wide without the fixed width, the first column expands
way out and there's a lot of empty space and your eye is forced to scan
all the way to the right to see the 2nd table column. Do you agree with
this view? I'm not an css expert so I'd love to hear about any issues
with using the current fixed width approach that outweigh the positive
benefits outlined above.

On Thu, 2011-01-27 at 14:28 +0000, Ian Booth wrote:
> >
> >>
> >> What I've done now is to make the <fieldset> element which contains
> >> everything for the related branches widget a fixed size and remove the
> >> width styling from the tables. The resize behaviour is the same as
> >> described above but I think it's a cleaner implementation.
> >
> > Do you really think we should go with the fixed width, even with all its
> > drawbacks?
> >
>
> Without the fixed width, the tables expand right out to the edge of the
> browser window, and on a screen with the browser maximised for example,
> it looks really bad (to my eye) when all the form elements are around
> the same width (approx size of text area widget) and just the related
> branches tables are really wide. In my view, it's better visually to try
> and maintain a similar width for all the form widgets and a consistent
> resize behaviour ie if the text area widget is fixed, the the tables
> (via the width styling on the fieldset element) should be too. When the
> tables are really wide without the fixed width, the first column expands
> way out and there's a lot of empty space and your eye is forced to scan
> all the way to the right to see the 2nd table column. Do you agree with
> this view? I'm not an css expert so I'd love to hear about any issues
> with using the current fixed width approach that outweigh the positive
> benefits outlined above.

Well, the main issue is that there's no way to make sure the table's
width is kept in sync with the widget's sync. You have a good point
that it will look really bad on wide windows, so I think you're right
that it's better to use the fixed width for now.

The implementation of the methods need to be put into the BranchTarget implementation classes.

Code changes made to move core model functionality to the IBranchTarget implementation. The View calls through to this class to get the required data. Separate tests written for IBranchTarget. The ordering of related branches is such the those for the most recent product series or distro series appear first. The sorting was done using sorted_version_numbers()

The related package branch listing now shows the distro series instead of the source package. I would appreciate a careful review of the algorithm used to load the related branches (especially the related package branches) to ensure everything is correct.

One question - I don't like it how the related branches tables are inside the form but I think it looks weird if they are moved outside the form and hence appear below the Update Recipe button. I'll do whatever is deemed best.

I added functionality to limit the number of related branches displayed to 5 in each category. See screenshot for example of how it looks:

http://people.canonical.com/~ianb/recipe-related-branches3.png

The branches shown are the most recent according to the sorted_version_numbers() function:
- for related series, branches, the ordering is applied to the product series, and the devel focus is always shown
- for package branches, the ordering is applied to the distro series

On Mon, 31 Jan 2011 15:18:53 you wrote:
> I added functionality to limit the number of related branches displayed to
> 5 in each category. See screenshot for example of how it looks:
>
> http://people.canonical.com/~ianb/recipe-related-branches3.png
>
> The branches shown are the most recent according to the
> sorted_version_numbers() function: - for related series, branches, the
> ordering is applied to the product series, and the devel focus is always
> shown - for package branches, the ordering is applied to the distro series

I don't think it is beneficial to have the text: "Top 5 most recent
branches...".

I think that if we limit the results, the user should be informed, lest
they get confused as to why a branch they may have been expecting to be
there is not. But that's just my opinion as a general ui priciple. If
you feel it's better to remove the text, that's no problem. Or would a
rewording of the text be better?

On 31/01/11 12:28, Tim Penhey wrote:
> On Mon, 31 Jan 2011 15:18:53 you wrote:
>> I added functionality to limit the number of related branches displayed to
>> 5 in each category. See screenshot for example of how it looks:
>>
>> http://people.canonical.com/~ianb/recipe-related-branches3.png
>>
>> The branches shown are the most recent according to the
>> sorted_version_numbers() function: - for related series, branches, the
>> ordering is applied to the product series, and the devel focus is always
>> shown - for package branches, the ordering is applied to the distro series
>
> I don't think it is beneficial to have the text: "Top 5 most recent
> branches...".
>

This is good to land.

This looks okay to me, aside from a few blank lines that look to have crept -- however, if make lint disagrees with me, then I'll let that slide.

Why are the imports for lp.testing.factory right under the docstring rather than being ordered with the rest of them?

I'd suggest running utilites/format-imports over the files you changed.

On 02/02/11 10:43, Steve Kowalik wrote:
> Review: Approve code*
> This looks okay to me, aside from a few blank lines that look to have crept -- however, if make lint disagrees with me, then I'll let that slide.
>

Lint is happy - just complains about lines too long which were already
there, related to string literals in some tests.

> Why are the imports for lp.testing.factory right under the docstring rather than being ordered with the rest of them?
>

Cause I need my eyesight checked. Fixed.

> I'd suggest running utilites/format-imports over the files you changed.