Merge lp:~mvo/software-center/lp1002271 into lp:software-center/5.2

[Michael Vogt]

This fixes a regression in the 5.2 branch when capitalize_first_word() is called with a empty string (e.g. a empty summary for a package).

[Natalia Bidart (nataliabidart)]

I would definitely call capitalize() over "string" without any further checking, since as far as I see that's all is needed.

review: Needs Fixing

[Robert Roth (evfool)]

I agree with Natalia on this one, I'm the one to blame for the 'unnecessary' workaround to capitalize only the first letter, as I have confused capitalize() with title() (which capitalizes all words of a string), that is why I have added the "workaround". The only thing to check is if the string is not None, and if it is not, we can safely use capitalize.

[Michael Vogt (mvo)]

One interessting observation:

In [2]: "MPlayer's Movie Encoder".capitalize()
Out[2]: "Mplayer's movie encoder"

So we may need something custom afterall as we don't want to change the subsequent words.

[Natalia Bidart (nataliabidart)]

Michael, I'd say to either user .title() or .capitalize() consistently among all packages. If the package description uses title-like or a capitalize-like message, that's up to the package maintainer, but I think the Software Center should use a consistent capitalization of these strings.

Does that make sense?

[Michael Vogt (mvo)]

Hi Natalia, thanks for your comment!

The spec https://wiki.ubuntu.com/SoftwareCenter currently says:
"""
The summary should be the application Comment, if there is one; otherwise the package synopsis with its first word capitalized, if the application has a Name but not a Comment; otherwise the package name (because if you’re looking at a non-application package in the first place, you’re fairly likely to be the sort of user who wants to know the package name).
"""

Its not clear to me what s-c should do in the mplayer example:
Original String: "MPlayer's Movie Encoder"
Should this become "MPlayer's Movie Encoder"
or "MPlayer's movie encoder"

I will ask mpt to clarify this a bit more.

[Michael Vogt (mvo)]

Sorry,
"""
 should this become "MPlayer's Movie Encoder"
"""
is misleading, should this stay unchanged is what I mean to ask.

[Matthew Paul Thomas (mpt)]

When I said "with its first word capitalized", I did not mean to suggest that any part of the string should be converted to lower case!

"MPlayer's Movie Encoder" should remain "MPlayer's Movie Encoder", because its first word is already capitalized.

If there's a clearer way I could have worded it, let me know, and I'll change it for both title and summary.

[Michael Vogt (mvo)]

Fwiw, the previous lines in the test:

19 self.assertTrue(
20 capitalized == "MPlayer's Movie Encoder")

should also become:
 self.assertEqual(capitalized, "MPlayer's Movie Encoder")

instead of the current self.assertTrue() because python will give a more meaningful result on test failure
(i.e. a diff of what-it-got and what-it-expected)

[Kiwinote (kiwinote)]

(It is worth noting that we don't always want the first letter capitalised - think of apps that explicitly brand themselves with a lower case first letter, eg gPodder, gThumb, jEdit, etc - it may work to capitalise the first letter of the first word if and only if the second letter of the first word is not capitalised.)

[Michael Vogt (mvo)]

@mpt: Thanks for your clarification, its fine, I just wanted to be sure I understand it correctly and to address the concern that Natalia expressed. The way the spec is currently written indicates that the capitalize_first_word() is the right approach here.

review: Needs Resubmitting

[Natalia Bidart (nataliabidart)]

review: Approve

[Michael Vogt (mvo)]

On Wed, May 23, 2012 at 11:56:19AM -0000, Natalia Bidart wrote:
> Review: Approve

Thanks!

=== modified file 'softwarecenter/utils.py'
--- softwarecenter/utils.py	2012-05-15 07:51:29 +0000
+++ softwarecenter/utils.py	2012-05-21 12:30:23 +0000
@@ -125,7 +125,7 @@
     """ this takes a package synopsis and uppercases the first  word's
         first letter
     """
-    if string[0].isalpha() and not string[0].isupper():
+    if len(string) > 1 and string[0].isalpha() and not string[0].isupper():
         return string[0].capitalize() + string[1:]
     return string
 

=== modified file 'test/test_utils.py'
--- test/test_utils.py	2012-05-15 07:51:29 +0000
+++ test/test_utils.py	2012-05-21 12:30:23 +0000
@@ -173,6 +173,8 @@
         capitalized = capitalize_first_word(test_synopsis)
         self.assertTrue(
             capitalized == "MPlayer's Movie Encoder")
+        # ensure it does not crash for empty strings, LP: #1002271
+        self.assertEqual(capitalize_first_word(""), "")
             
     def test_ensure_file_writable_and_delete_if_not(self):
         from softwarecenter.utils import ensure_file_writable_and_delete_if_not