launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05098
[Merge] lp:~sinzui/launchpad/picker-custom-icons into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/picker-custom-icons into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/picker-custom-icons/+merge/77053
Add custom icons to pickers.
Launchpad bug: https://bugs.launchpad.net/bugs/857785
Pre-implementation: jcsackett
Custom icons are missing from target pickers. Actually. They are missing
from all pickers. While Entry.image is defined for custom icons,
no code sets it. This might be because developers assumed that getting
the object's sprite css provided the custom icon...it does not, it only
provides the default display. The css and image are mutually exclusive,
we want the image, but will accept the sprite.
--------------------------------------------------------------------
RULES
* The Entry code is using ObjectImageDisplayAPI, but there is no public
method to get the custom icon url.
* Make the _get_custom_icon_url() public and add a test for it since
since the method appears to be untested.
* Update DefaultPickerEntrySourceAdapter.getPickerEntries() to also
set the image.
* Update DefaultPickerEntrySourceAdapter.getPickerEntries() to only
set the css if the image url is None.
QA
* Visit https://bugs.qastaging.launchpad.net/
* Use the choose link and search for 'launchpad'
* Verify that /launchpad has the Lp icon
* Visit https://bugs.qastaging.launchpad.net/launchpad/+bug/741639
* Choose to reassign the bug
* Search for ~launchpad
* Verify that ~launchpad has the Lp icon
LINT
lib/lp/app/browser/tales.py
lib/lp/app/browser/vocabulary.py
lib/lp/app/browser/tests/test_vocabulary.py
lib/lp/app/tests/test_tales.py
TEST
./bin/test -vvc -t ObjectImageDisplayAPITestCase lp.app.tests.test_tales
./bin/test -vvc -t DefaultPickerEntrySourceAdapterTestCase \
lp.app.browser.tests.test_vocabulary
IMPLEMENTATION
Renamed _get_custom_icon_url to custom_icon_url and added tests to verify
the three value it can return.
lib/lp/app/browser/tales.py
lib/lp/app/browser/tests/test_vocabulary.py
Updated DefaultPickerEntrySourceAdapter.getPickerEntries() to get both
the custom icon and sprite css for the entry and then choose whether to
use the icon or the sprite.
lib/lp/app/browser/vocabulary.py
lib/lp/app/tests/test_tales.py
--
https://code.launchpad.net/~sinzui/launchpad/picker-custom-icons/+merge/77053
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/picker-custom-icons into lp:launchpad.
=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py 2011-08-01 18:51:18 +0000
+++ lib/lp/app/browser/tales.py 2011-09-26 21:45:30 +0000
@@ -792,7 +792,7 @@
return '/@@/meeting-mugshot'
return None
- def _get_custom_icon_url(self):
+ def custom_icon_url(self):
"""Return the URL for this object's icon."""
context = self._context
if IHasIcon.providedBy(context) and context.icon is not None:
@@ -1175,7 +1175,7 @@
def _makeLink(self, view_name, rootsite, text):
person = self._context
url = self.url(view_name, rootsite)
- custom_icon = ObjectImageDisplayAPI(person)._get_custom_icon_url()
+ custom_icon = ObjectImageDisplayAPI(person).custom_icon_url()
if custom_icon is None:
css_class = ObjectImageDisplayAPI(person).sprite_css()
return (u'<a href="%s" class="%s">%s</a>') % (
@@ -1207,7 +1207,7 @@
def icon(self, view_name):
"""Return the URL for the person's icon."""
custom_icon = ObjectImageDisplayAPI(
- self._context)._get_custom_icon_url()
+ self._context).custom_icon_url()
if custom_icon is None:
css_class = ObjectImageDisplayAPI(self._context).sprite_css()
return '<span class="' + css_class + '"></span>'
@@ -1426,7 +1426,7 @@
html = super(PillarFormatterAPI, self).link(view_name)
context = self._context
custom_icon = ObjectImageDisplayAPI(
- context)._get_custom_icon_url()
+ context).custom_icon_url()
url = self.url(view_name, rootsite)
summary = self._make_link_summary()
if custom_icon is None:
=== modified file 'lib/lp/app/browser/tests/test_vocabulary.py'
--- lib/lp/app/browser/tests/test_vocabulary.py 2011-09-20 20:33:47 +0000
+++ lib/lp/app/browser/tests/test_vocabulary.py 2011-09-26 21:45:30 +0000
@@ -26,7 +26,10 @@
IHugeVocabulary,
VocabularyFilter,
)
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+ DatabaseFunctionalLayer,
+ LaunchpadFunctionalLayer,
+ )
from lp.app.browser.vocabulary import (
IPickerEntrySource,
MAX_DESCRIPTION_LENGTH,
@@ -49,6 +52,31 @@
return entry
+class DefaultPickerEntrySourceAdapterTestCase(TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def test_css_image_entry_without_icon(self):
+ product = self.factory.makeProduct()
+ entry = get_picker_entry(product, object())
+ self.assertEqual("sprite product", entry.css)
+ self.assertEqual(None, entry.image)
+
+ def test_css_image_entry_without_icon_or_sprite(self):
+ thing = object()
+ entry = get_picker_entry(thing, object())
+ self.assertEqual('sprite bullet', entry.css)
+ self.assertEqual(None, entry.image)
+
+ def test_css_image_entry_with_icon(self):
+ icon = self.factory.makeLibraryFileAlias(
+ filename='smurf.png', content_type='image/png')
+ product = self.factory.makeProduct(icon=icon)
+ entry = get_picker_entry(product, object())
+ self.assertEqual(None, entry.css)
+ self.assertEqual(icon.getURL(), entry.image)
+
+
class PersonPickerEntrySourceAdapterTestCase(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -224,6 +252,7 @@
distroarchseries=archseries)
self.assertEqual("fnord", self.getPickerEntry(dsp).description)
+
class TestProductPickerEntrySourceAdapter(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -328,6 +357,7 @@
self.assertEqual(
expected_details, entry.details[0])
+
class TestDistributionPickerEntrySourceAdapter(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -372,7 +402,7 @@
def test_distribution_truncates_summary(self):
summary = ("This is a deliberately, overly long summary. It goes on"
"and on and on so as to break things up a good bit.")
- distribution= self.factory.makeDistribution(summary=summary)
+ distribution = self.factory.makeDistribution(summary=summary)
index = summary.rfind(' ', 0, 45)
expected_summary = summary[:index + 1]
expected_details = summary[index:]
@@ -382,6 +412,7 @@
self.assertEqual(
expected_details, entry.details[0])
+
class TestPersonVocabulary:
implements(IHugeVocabulary)
test_persons = []
=== modified file 'lib/lp/app/browser/vocabulary.py'
--- lib/lp/app/browser/vocabulary.py 2011-09-20 20:34:30 +0000
+++ lib/lp/app/browser/vocabulary.py 2011-09-26 21:45:30 +0000
@@ -129,9 +129,12 @@
if hasattr(term_value, 'summary'):
extra.description = term_value.summary
display_api = ObjectImageDisplayAPI(term_value)
- extra.css = display_api.sprite_css()
- if extra.css is None:
- extra.css = 'sprite bullet'
+ image_url = display_api.custom_icon_url() or None
+ css = display_api.sprite_css() or 'sprite bullet'
+ if image_url is not None:
+ extra.image = image_url
+ else:
+ extra.css = css
entries.append(extra)
return entries
@@ -260,7 +263,7 @@
picker_entry.details = []
summary = picker_entry.description
if len(summary) > 45:
- index = summary.rfind(' ', 0, 45)
+ index = summary.rfind(' ', 0, 45)
first_line = summary[0:index + 1]
second_line = summary[index:]
else:
@@ -268,15 +271,15 @@
second_line = ''
if len(second_line) > 90:
- index = second_line.rfind(' ', 0, 90)
- second_line = second_line[0:index+1]
+ index = second_line.rfind(' ', 0, 90)
+ second_line = second_line[0:index + 1]
picker_entry.description = first_line
picker_entry.details.append(second_line)
picker_entry.alt_title = target.name
picker_entry.target_type = self.target_type
maintainer = self.getMaintainer(target)
if maintainer is not None:
- picker_entry.details.append(
+ picker_entry.details.append(
'Maintainer: %s' % self.getMaintainer(target))
return entries
=== modified file 'lib/lp/app/tests/test_tales.py'
--- lib/lp/app/tests/test_tales.py 2011-05-31 22:21:39 +0000
+++ lib/lp/app/tests/test_tales.py 2011-09-26 21:45:30 +0000
@@ -17,9 +17,11 @@
from canonical.testing.layers import (
DatabaseFunctionalLayer,
FunctionalLayer,
+ LaunchpadFunctionalLayer,
)
from lp.app.browser.tales import (
format_link,
+ ObjectImageDisplayAPI,
PersonFormatterAPI,
)
from lp.registry.interfaces.irc import IIrcIDSet
@@ -302,3 +304,25 @@
'<span class="discreet"> on </span>\n'
'<strong><b>irc.canonical.com</b></strong>\n',
expected_html)
+
+
+class ObjectImageDisplayAPITestCase(TestCaseWithFactory):
+ """Tests for ObjectImageDisplayAPI"""
+
+ layer = LaunchpadFunctionalLayer
+
+ def test_custom_icon_url_context_is_None(self):
+ display_api = ObjectImageDisplayAPI(None)
+ self.assertEqual('', display_api.custom_icon_url())
+
+ def test_custom_icon_url_context_has_no_icon(self):
+ product = self.factory.makeProduct()
+ display_api = ObjectImageDisplayAPI(product)
+ self.assertEqual(None, display_api.custom_icon_url())
+
+ def test_custom_icon_url_context_has_an_icon(self):
+ icon = self.factory.makeLibraryFileAlias(
+ filename='smurf.png', content_type='image/png')
+ product = self.factory.makeProduct(icon=icon)
+ display_api = ObjectImageDisplayAPI(product)
+ self.assertEqual(icon.getURL(), display_api.custom_icon_url())
