Merge lp:~bac/launchpad/bug-643538-code into lp:launchpad

Screenshots can be seen at: http://people.canonical.com/~bac/code_usage/

I've noticed on the screen where no code has been registered[1] there is a link to +set-branch (Configure code hosting) but no link to +add-branch (seen later as "Register a branch"). This seems like an oversight because using the former you cannot mirror a branch, only set in Launchpad and import.

[1] http://people.canonical.com/~bac/code_usage/0-nocode-owner.png

Cool branch :)

The comments I have are all really minor.

Gavin.

[1]

     def getDistroDevelSeries(self, distribution):
- """Since distribution.currentseries hits the DB every time, cache it."""
+ """distribution.currentseries hits the DB every time so cache it."""
         self._distro_series_map = {}
         try:
             return self._distro_series_map[distribution]
         except KeyError:
             result = distribution.currentseries
             self._distro_series_map[distribution] = result
             return result

I think your change doesn't describe what the code does... but that's
because the code is broken! The following line is the culprit:

         self._distro_series_map = {}

Every time the method is called the cache gets reset. I think this
line can be removed.

[2]

+ self.branch = self.development_focus_branch

development_focus_branch is a cachedproperty, so perhaps branch should
be a property so that development_focus_branch is not computed until
it is needed.

[3]

+ self.assertEqual(None, find_tag_by_id(contents, 'branch-portlet'))
...
+ self.assertNotEqual(None, find_tag_by_id(contents, 'branch-portlet'))

I think this (and others that test against None) should be done with
assertIs(None, ...). Can't remember what difference is makes though :)

[4]

+ def test_portlets_shown_for_EXTERNAL(self):
+ # If the BranchUsage is HOSTED then the portlets are shown.

s/HOSTED/EXTERNAL/

[5]

+ <a tal:attributes="href view/mirror_location"><tal:mirror replace="view/mirror_location"/></a>.

Doesn't really matter, but it might be clearer expressed as:

        <a tal:attributes="href view/mirror_location"
           tal:content="view/mirror_location"/>.

[6]

+ <div tal:condition="not:view/new_branches_are_private" id="privacy-text">
+ <p>
+ New branches you create for <tal:name replace="context/displayname"/>
+ are <strong>public</strong> initially.
+ </p>
+ </div>
+ <div tal:condition="view/new_branches_are_private" id="privacy-text">
+ <p>
+ New branches you create for <tal:name replace="context/displayname"/>
+ are <strong>private</strong> initially.
+ </p>
+ </div>

The tal:condition could go on the subordinate <p> tags, then there's
only one <div>.

[7]

+ <tal:comment condition="nothing">
+ The view/*_count|nothing expressions below are so that this
+ template can be rendered by a view that does not have count
+ information available.
+ </tal:comment>

There aren't any view/*_count|nothing expressions, so this comment can
go.

[8]

+ configured = sum([1 for v in config_statuses.values() if v])

Don't need the list comprehension.

On Sep 23, 2010, at 11:49 , Gavin Panella wrote:

> Review: Approve code
> Cool branch :)
>
> The comments I have are all really minor.

Thanks for the helpful review Gavin.

>
> Gavin.
>
>
> [1]
>
> def getDistroDevelSeries(self, distribution):
> - """Since distribution.currentseries hits the DB every time, cache it."""
> + """distribution.currentseries hits the DB every time so cache it."""
> self._distro_series_map = {}
> try:
> return self._distro_series_map[distribution]
> except KeyError:
> result = distribution.currentseries
> self._distro_series_map[distribution] = result
> return result
>
> I think your change doesn't describe what the code does... but that's
> because the code is broken! The following line is the culprit:
>
> self._distro_series_map = {}
>
> Every time the method is called the cache gets reset. I think this
> line can be removed.

Thanks for looking. My change was just to stop lint from complaining about the comment being too long -- I didn't even see the broken code.

>
>
> [2]
>
> + self.branch = self.development_focus_branch
>
> development_focus_branch is a cachedproperty, so perhaps branch should
> be a property so that development_focus_branch is not computed until
> it is needed.
>

Good catch.

>
> [3]
>
> + self.assertEqual(None, find_tag_by_id(contents, 'branch-portlet'))
> ...
> + self.assertNotEqual(None, find_tag_by_id(contents, 'branch-portlet'))
>
> I think this (and others that test against None) should be done with
> assertIs(None, ...). Can't remember what difference is makes though :)
>

Done

>
> [4]
>
> + def test_portlets_shown_for_EXTERNAL(self):
> + # If the BranchUsage is HOSTED then the portlets are shown.
>
> s/HOSTED/EXTERNAL/
>

Done

>
> [5]
>
> + <a tal:attributes="href view/mirror_location"><tal:mirror replace="view/mirror_location"/></a>.
>
> Doesn't really matter, but it might be clearer expressed as:
>
> <a tal:attributes="href view/mirror_location"
> tal:content="view/mirror_location"/>.
>
>

Yes, much nicer.

> [6]
>
> + <div tal:condition="not:view/new_branches_are_private" id="privacy-text">
> + <p>
> + New branches you create for <tal:name replace="context/displayname"/>
> + are <strong>public</strong> initially.
> + </p>
> + </div>
> + <div tal:condition="view/new_branches_are_private" id="privacy-text">
> + <p>
> + New branches you create for <tal:name replace="context/displayname"/>
> + are <strong>private</strong> initially.
> + </p>
> + </div>
>
> The tal:condition could go on the subordinate <p> tags, then there's
> only one <div>.
>

Done.

>
> [7]
>
> + <tal:comment condition="nothing">
> + The view/*_count|nothing expressions below are so that this
> + template can be rendered by a view that does not have count
> + information available.
> + </tal:comment>
>
> There aren't any view/*_count|nothing expressions, so this comment can
> go.
>
>
c-n-p error

> [8]
>
> ...

Thanks for this work Brad. I have a couple of suggestions:

When no code hosting/mirroring is configured, I think the page would be easier to read if some of the text were split up.

For screen shots 0 and 1, I suggest the following:

  There are no branches of ticktock in Launchpad. You can change this by:

   * activating code hosting directly on Launchpad (read more)
   * asking Launchpad to mirror a Bazaar branch hosted elsewhere (read more)
   * asking Launchpad to import code from Git, Subversion or CVS into a Bazaar branch (read more).

I have removed Mercurial because Tim tells me it is still very much experimental (i.e. works in around 20% of cases).

Other than that, I'm Matthew Revell and I approve this message.

This looks good but I'm not sure the portlets will work nice with real data. For instance, if you see https://code.edge.launchpad.net/launchpad, most branches have long names, which cause most of the horizontal space to be used. How will that look with the portlets?

Also, it looks a bit weird having the two different styles of links (the Register a branch one uses a bigger font and the icon is to its left while the others use a smaller font and have the icon to the right) on the portlet. And by the way, are we moving the action links that were once inlined back into portlets?

Salgado thanks for the review and the questions.

I tried creating some branches with absurdly long names to see what happened. Using a browser that is almost 1400 pixels wide I get http://people.canonical.com/~bac/code_usage/5-code-still-fits.png. I should probably sees what that looks like on a netbook instead of my 27" monitor. :)

Shrinking the browser from there causes bad behavior seen at http://people.canonical.com/~bac/code_usage/6-too-narrow-overlap.png. I'll work with Curtis to see what can be done.

The other application pages have portlets similar to the one I'm proposing. The link styles are patterned after the involvement portlet on a project index page as seen by a project owner so it is on unprecedented. I think the layout needs some tweaking.

The branch urls/names/displayname/what-ever should be using fmt:break-long-words

I think the font size difference in the links is a real problem. They should be the same, particularly since both presentations were added for 3.0. I do not know which is wrong. The problem might be hard to find because one might be using styles from the deprecated style sheet. I cannot judge the scope of this issue. it might be worthy of a separate bug, and it may be in launchpad-web if it is a universal issue.

Hi Curtis, Matthew and Salgado.

I have made the changes as requested by you and they are shown in screenshots 7-9 at http://people.canonical.com/~bac/code_usage/.

* Bulleted list
* Capitalizing the items in the portlets
* Using break-long-words on the branch names in the table allows them to collapse when the window shrinks
* Don't

The requested changes required a number of test changes, especially the change to not show the portlets when code hosting has not yet been configured. Curtis in addition to mentoring the UI could you look at the latest code diff at http://pastebin.ubuntu.com/502288/ ?

The code looks fine. You fixed the sprite and comprehension issues I pointed out in the paste.

I was disappointed by the story additions, the stories did not really tell a story. I was also concerned that the /applets is owned by Foo Bar, so the test for an story about an owner was performed by an admin :(. We looked and did not see an intersection with the unittests in lp/code/browser/tests, so we decided to accept the changes. This branch is large and has a lot of value.

On Tue, 2010-09-28 at 19:50 +0000, Brad Crittenden wrote:
> Hi Curtis, Matthew and Salgado.
>
> I have made the changes as requested by you and they are shown in
> screenshots 7-9 at http://people.canonical.com/~bac/code_usage/.
>
> * Bulleted list
> * Capitalizing the items in the portlets
> * Using break-long-words on the branch names in the table allows them
> to collapse when the window shrinks

That certainly improves things, but my main concern was with the
horizontal space that is now taken by the portlets. Given that the
portlets have virtually no content (just action links and 3 lines of
statistics), we're wasting a significant amount (IMHO) of real state,
which will make the branches list a bit too narrow on a 800px wide
browser window, like I tend to use[1]
(<http://people.canonical.com/~salgado/branches.png>).

You've said that other application pages have portlets, and that's true
for the bugs app page, but in that case they have a significant amount
of content (the tag cloud, pre-defined searches and action links), which
is not the case here. Also, the translations app page, on the other
hand, doesn't have any portlets, so I'm still not convinced the code app
page should have them. Unless there's a policy I'm not aware of?

[1] Even though my monitor's resolution is higher than that, I tend to
use less than 800px wide browser windows because most pages work fine on
those and I think Launchpad should as well.

On Tue, 2010-09-28 at 21:07 +0000, Guilherme Salgado wrote:
> You've said that other application pages have portlets, and that's
> true
> for the bugs app page, but in that case they have a significant amount
> of content (the tag cloud, pre-defined searches and action links),
> which
> is not the case here. Also, the translations app page, on the other
> hand, doesn't have any portlets, so I'm still not convinced the code
> app
> page should have them. Unless there's a policy I'm not aware of?

The intent of the features is to make the root pages for projects to use
the same layouts and conventions. Code is using a 2.0 layout still.
Translations remains a conundrum. The user should trust that global
actions and set searches are in the right portlets.

This feature work is driven by our own strategists complaint that users
see conflicting designs when they work across applications.

{{{

Hello Curtis, Strategons,

Attached are screenshots of each of the six tabs of Launchpad for a
newly created project.

I want you to look at them. Make a note of the fonts, the styling of
the action portlets, the alignment of cells, the busyness of the
pages, the balance of the layouts, the colours used, the amount of
text on each page.

You will notice that there is significant difference between each
page. If you look at the pages with a detached eye, you may well come
to the conclusion that none of them are very nice to look at.

I think that this is a problem. I don't mean to put down anyone on the
team, but I would be embarrassed to present on these pages at a
conference.

...

}}}

This lead to an extension of the bridging-the-gap theme in June.
Launchpad must state where services are hosted so that they can complete
their task. Launchpad must state when the service is unknown, hosted, or
external. The applications must behave the same way when unknown,
external, or hosted so that users know what to expect when they use
multiple applications.
--
__Curtis C. Hovey_________
http://launchpad.net/

Thanks for the explanation, Curtis.

Since it looks like this has been thought through carefully, I think
it's reasonable to trade some screen real estate with better
consistency.

 review approve