Merge lp:~salgado/launchpad/real-breadcrumbs into lp:launchpad
- real-breadcrumbs
- Merge into devel
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Approved by: | Aaron Bentley | ||||||||
Approved revision: | no longer in the source branch. | ||||||||
Merged at revision: | not available | ||||||||
Proposed branch: | lp:~salgado/launchpad/real-breadcrumbs | ||||||||
Merge into: | lp:launchpad | ||||||||
Diff against target: | None lines | ||||||||
To merge this branch: | bzr merge lp:~salgado/launchpad/real-breadcrumbs | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Aaron Bentley (community) | Approve | ||
Review via email:
|
Commit message
Description of the change
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Guilherme Salgado (salgado) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Aaron Bentley (abentley) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
status approved
The only think I see that could be improved is in Heirarchy.items, noted
below.
Guilherme Salgado wrote:
> Guilherme Salgado has proposed merging lp:~salgado/launchpad/real-breadcrumbs into lp:launchpad/devel.
>
> Requested reviews:
> Canonical Launchpad Engineering (launchpad)
>
> = Summary =
>
> When I redesigned our breadcrumb infrastructure I made the
> BreadcbrumbBuilder abstraction unnecessary, but my branch was already
> too big so I left it to be removed later.
>
> == Proposed fix ==
>
> Remove BreadcrumbBuilder by directly adapting objects into IBreadcrumbs,
> instead of adapting them into IBreadcrumbBuilder and calling
> make_breadcrumb() on it.
>
> In order to actually complete this I had to change all existing adapters
> so that they inherit from Breadcrumb rather than BreadcrumbBuilder, but
> that brought the diff to 1.4KLOC, so I'll ask for a review of that
> later.
>
> == Pre-implementation notes ==
>
> == Implementation details ==
>
> == Tests ==
>
> ./bin/test -vvt hierarchical-
>
> == Demo and Q/A ==
>
>
> = Launchpad lint =
>
> Checking for conflicts. and issues in doctests and templates.
> Running jslint, xmllint, pyflakes, and pylint.
> Using normal rules.
>
> Linting changed files:
> lib/canonical/
> lib/canonical/
> lib/canonical/
> lib/canonical/
> lib/canonical/
> lib/canonical/
>
>
> == Pyflakes Doctest notices ==
>
> lib/canonical/
> 144: 'canonical_url' imported but unused
>
> This has been removed already.
>
>
> == Pylint notices ==
>
> lib/canonical/
> 103: [F0401] Unable to import 'lazr.uri' (No module named uri)
>
>
> === modified file 'lib/canonical/
> --- lib/canonical/
> +++ lib/canonical/
> @@ -96,7 +96,7 @@
> StandardLaunchp
> stepto)
> from canonical.
> - IBreadcrumbBuilder, ILaunchBag, ILaunchpadRoot, INavigationMenu,
> + IBreadcrumb, ILaunchBag, ILaunchpadRoot, INavigationMenu,
> NotFoundError, POSTToNonCanoni
> from canonical.
> from canonical.
> @@ -226,39 +226,31 @@
> The list starts with the breadcrumb closest to the hierarchy root.
> """
> breadcrumbs = []
> - for builder in self._getBreadc
> - crumb = builder.
> - if crumb is not None:
> - breadcrumbs.
> - return breadcrumbs
> -
> - def _getBreadcrumbB
> - builders = []
> for obj in self.objects:
> - builder = queryAdapter(obj, IBreadcrumbBuilder)
> -...
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Guilherme Salgado (salgado) wrote : | # |
Hi Aaron,
I did the change you suggested, but I need to cast the result of enumerate(
Also, https:/
Preview Diff
1 | === modified file 'lib/canonical/launchpad/browser/launchpad.py' | |||
2 | --- lib/canonical/launchpad/browser/launchpad.py 2009-08-20 05:06:34 +0000 | |||
3 | +++ lib/canonical/launchpad/browser/launchpad.py 2009-08-24 17:47:34 +0000 | |||
4 | @@ -96,7 +96,7 @@ | |||
5 | 96 | StandardLaunchpadFacets, canonical_name, canonical_url, custom_widget, | 96 | StandardLaunchpadFacets, canonical_name, canonical_url, custom_widget, |
6 | 97 | stepto) | 97 | stepto) |
7 | 98 | from canonical.launchpad.webapp.interfaces import ( | 98 | from canonical.launchpad.webapp.interfaces import ( |
9 | 99 | IBreadcrumbBuilder, ILaunchBag, ILaunchpadRoot, INavigationMenu, | 99 | IBreadcrumb, ILaunchBag, ILaunchpadRoot, INavigationMenu, |
10 | 100 | NotFoundError, POSTToNonCanonicalURL) | 100 | NotFoundError, POSTToNonCanonicalURL) |
11 | 101 | from canonical.launchpad.webapp.publisher import RedirectionView | 101 | from canonical.launchpad.webapp.publisher import RedirectionView |
12 | 102 | from canonical.launchpad.webapp.authorization import check_permission | 102 | from canonical.launchpad.webapp.authorization import check_permission |
13 | @@ -226,39 +226,31 @@ | |||
14 | 226 | The list starts with the breadcrumb closest to the hierarchy root. | 226 | The list starts with the breadcrumb closest to the hierarchy root. |
15 | 227 | """ | 227 | """ |
16 | 228 | breadcrumbs = [] | 228 | breadcrumbs = [] |
17 | 229 | for builder in self._getBreadcrumbBuilders(): | ||
18 | 230 | crumb = builder.make_breadcrumb() | ||
19 | 231 | if crumb is not None: | ||
20 | 232 | breadcrumbs.append(crumb) | ||
21 | 233 | return breadcrumbs | ||
22 | 234 | |||
23 | 235 | def _getBreadcrumbBuilders(self): | ||
24 | 236 | builders = [] | ||
25 | 237 | for obj in self.objects: | 229 | for obj in self.objects: |
29 | 238 | builder = queryAdapter(obj, IBreadcrumbBuilder) | 230 | breadcrumb = queryAdapter(obj, IBreadcrumb) |
30 | 239 | if builder is not None: | 231 | if breadcrumb is not None: |
31 | 240 | builders.append(builder) | 232 | breadcrumbs.append(breadcrumb) |
32 | 241 | 233 | ||
33 | 242 | host = URI(self.request.getURL()).host | 234 | host = URI(self.request.getURL()).host |
35 | 243 | if (len(builders) == 0 | 235 | if (len(breadcrumbs) == 0 |
36 | 244 | or host == allvhosts.configs['mainsite'].hostname): | 236 | or host == allvhosts.configs['mainsite'].hostname): |
38 | 245 | return builders | 237 | return breadcrumbs |
39 | 246 | 238 | ||
40 | 247 | # If we got this far it means we have breadcrumbs and we're not on the | 239 | # If we got this far it means we have breadcrumbs and we're not on the |
41 | 248 | # mainsite, so we'll sneak an extra breadcrumb for the vhost we're on. | 240 | # mainsite, so we'll sneak an extra breadcrumb for the vhost we're on. |
42 | 249 | vhost = host.split('.')[0] | 241 | vhost = host.split('.')[0] |
43 | 250 | 242 | ||
45 | 251 | # Iterate over the context of our builders in reverse order and for | 243 | # Iterate over the context of our breadcrumbs in reverse order and for |
46 | 252 | # the first one we find an adapter named after the vhost we're on, | 244 | # the first one we find an adapter named after the vhost we're on, |
47 | 253 | # generate an extra breadcrumb and insert it in our list. | 245 | # generate an extra breadcrumb and insert it in our list. |
54 | 254 | for idx in reversed(xrange(len(builders))): | 246 | for idx in reversed(xrange(len(breadcrumbs))): |
55 | 255 | builder = builders[idx] | 247 | breadcrumb = breadcrumbs[idx] |
56 | 256 | extra_builder = queryAdapter( | 248 | extra_breadcrumb = queryAdapter( |
57 | 257 | builder.context, IBreadcrumbBuilder, name=vhost) | 249 | breadcrumb.context, IBreadcrumb, name=vhost) |
58 | 258 | if extra_builder is not None: | 250 | if extra_breadcrumb is not None: |
59 | 259 | builders.insert(idx + 1, extra_builder) | 251 | breadcrumbs.insert(idx + 1, extra_breadcrumb) |
60 | 260 | break | 252 | break |
62 | 261 | return builders | 253 | return breadcrumbs |
63 | 262 | 254 | ||
64 | 263 | def render(self): | 255 | def render(self): |
65 | 264 | """Render the hierarchy HTML. | 256 | """Render the hierarchy HTML. |
66 | 265 | 257 | ||
67 | === modified file 'lib/canonical/launchpad/browser/personproduct.py' | |||
68 | --- lib/canonical/launchpad/browser/personproduct.py 2009-06-25 05:30:52 +0000 | |||
69 | +++ lib/canonical/launchpad/browser/personproduct.py 2009-08-24 17:47:34 +0000 | |||
70 | @@ -5,7 +5,7 @@ | |||
71 | 5 | 5 | ||
72 | 6 | __metaclass__ = type | 6 | __metaclass__ = type |
73 | 7 | __all__ = [ | 7 | __all__ = [ |
75 | 8 | 'PersonProductBreadcrumbBuilder', | 8 | 'PersonProductBreadcrumb', |
76 | 9 | 'PersonProductFacets', | 9 | 'PersonProductFacets', |
77 | 10 | 'PersonProductNavigation', | 10 | 'PersonProductNavigation', |
78 | 11 | ] | 11 | ] |
79 | @@ -17,7 +17,7 @@ | |||
80 | 17 | from lp.code.interfaces.branchnamespace import ( | 17 | from lp.code.interfaces.branchnamespace import ( |
81 | 18 | get_branch_namespace) | 18 | get_branch_namespace) |
82 | 19 | from canonical.launchpad.interfaces.personproduct import IPersonProduct | 19 | from canonical.launchpad.interfaces.personproduct import IPersonProduct |
84 | 20 | from canonical.launchpad.webapp.breadcrumb import BreadcrumbBuilder | 20 | from canonical.launchpad.webapp.breadcrumb import Breadcrumb |
85 | 21 | from canonical.launchpad.webapp import ( | 21 | from canonical.launchpad.webapp import ( |
86 | 22 | Link, Navigation, StandardLaunchpadFacets) | 22 | Link, Navigation, StandardLaunchpadFacets) |
87 | 23 | from canonical.launchpad.webapp.interfaces import NotFoundError | 23 | from canonical.launchpad.webapp.interfaces import NotFoundError |
88 | @@ -38,8 +38,8 @@ | |||
89 | 38 | return branch | 38 | return branch |
90 | 39 | 39 | ||
91 | 40 | 40 | ||
94 | 41 | class PersonProductBreadcrumbBuilder(BreadcrumbBuilder): | 41 | class PersonProductBreadcrumb(Breadcrumb): |
95 | 42 | """Builds a breadcrumb for an `IPersonProduct`.""" | 42 | """Breadcrumb for an `IPersonProduct`.""" |
96 | 43 | 43 | ||
97 | 44 | @property | 44 | @property |
98 | 45 | def text(self): | 45 | def text(self): |
99 | 46 | 46 | ||
100 | === modified file 'lib/canonical/launchpad/doc/hierarchical-menu.txt' | |||
101 | --- lib/canonical/launchpad/doc/hierarchical-menu.txt 2009-08-17 19:22:06 +0000 | |||
102 | +++ lib/canonical/launchpad/doc/hierarchical-menu.txt 2009-08-24 17:47:34 +0000 | |||
103 | @@ -60,7 +60,7 @@ | |||
104 | 60 | 60 | ||
105 | 61 | The Hierarchy class builds the breadcrumbs by looking at each object in | 61 | The Hierarchy class builds the breadcrumbs by looking at each object in |
106 | 62 | the request.traversed_objects attribute. If a traversed object can be | 62 | the request.traversed_objects attribute. If a traversed object can be |
108 | 63 | adapted to IBreadcrumbBuilder, then it is added to the breadcrumbs list. | 63 | adapted to IBreadcrumb, then it is added to the breadcrumbs list. |
109 | 64 | 64 | ||
110 | 65 | We'll add the objects to the request's list of traversed objects so | 65 | We'll add the objects to the request's list of traversed objects so |
111 | 66 | the hierarchy will discover them. | 66 | the hierarchy will discover them. |
112 | @@ -71,38 +71,35 @@ | |||
113 | 71 | ... [root, cookbook, recipe]) | 71 | ... [root, cookbook, recipe]) |
114 | 72 | 72 | ||
115 | 73 | The hierarchy's list of breadcrumbs is empty since none of the objects | 73 | The hierarchy's list of breadcrumbs is empty since none of the objects |
117 | 74 | have an IBreadcrumbBuilder adapter. | 74 | have an IBreadcrumb adapter. |
118 | 75 | 75 | ||
119 | 76 | >>> hierarchy = getMultiAdapter((recipe, request), name='+hierarchy') | 76 | >>> hierarchy = getMultiAdapter((recipe, request), name='+hierarchy') |
120 | 77 | >>> hierarchy.items() | 77 | >>> hierarchy.items() |
121 | 78 | [] | 78 | [] |
122 | 79 | 79 | ||
127 | 80 | The ICookbook and IRecipe breadcrumb objects show up in the hierarchy | 80 | The ICookbook and IRecipe breadcrumb objects show up in the hierarchy after |
128 | 81 | after IBreadcrumbBuilder adapters are registered for them. The | 81 | IBreadcrumb adapters are registered for them. The hierarchy builds a list of |
129 | 82 | hierarchy builds a list of breadcrumbs starting with the breadcrumb | 82 | breadcrumbs starting with the breadcrumb closest to the hierarchy root. |
126 | 83 | closest to the hierarchy root. | ||
130 | 84 | 83 | ||
132 | 85 | >>> from canonical.launchpad.webapp.breadcrumb import BreadcrumbBuilder | 84 | >>> from canonical.launchpad.webapp.breadcrumb import Breadcrumb |
133 | 86 | 85 | ||
134 | 87 | # Note that the Hierarchy assigns the breadcrumb's URL, but we need to | 86 | # Note that the Hierarchy assigns the breadcrumb's URL, but we need to |
135 | 88 | # give it a valid .text attribute. | 87 | # give it a valid .text attribute. |
137 | 89 | >>> class TextualBreadcrumbBuilder(BreadcrumbBuilder): | 88 | >>> class TextualBreadcrumb(Breadcrumb): |
138 | 90 | ... @property | 89 | ... @property |
139 | 91 | ... def text(self): | 90 | ... def text(self): |
140 | 92 | ... return self.context.name.capitalize().replace('-', ' ') | 91 | ... return self.context.name.capitalize().replace('-', ' ') |
141 | 93 | 92 | ||
143 | 94 | >>> from canonical.launchpad.webapp.interfaces import IBreadcrumbBuilder | 93 | >>> from canonical.launchpad.webapp.interfaces import IBreadcrumb |
144 | 95 | 94 | ||
149 | 96 | >>> provideAdapter( | 95 | >>> provideAdapter(TextualBreadcrumb, [ICookbook], IBreadcrumb) |
150 | 97 | ... TextualBreadcrumbBuilder, [ICookbook], IBreadcrumbBuilder) | 96 | >>> provideAdapter(TextualBreadcrumb, [IRecipe], IBreadcrumb) |
147 | 98 | >>> provideAdapter( | ||
148 | 99 | ... TextualBreadcrumbBuilder, [IRecipe], IBreadcrumbBuilder) | ||
151 | 100 | 97 | ||
152 | 101 | >>> hierarchy.items() | 98 | >>> hierarchy.items() |
154 | 102 | [<Breadcrumb | 99 | [<TextualBreadcrumb |
155 | 103 | url='http://launchpad.dev/joy-of-cooking' | 100 | url='http://launchpad.dev/joy-of-cooking' |
156 | 104 | text='Joy of cooking'>, | 101 | text='Joy of cooking'>, |
158 | 105 | <Breadcrumb | 102 | <TextualBreadcrumb |
159 | 106 | url='http://launchpad.dev/joy-of-cooking/spam' | 103 | url='http://launchpad.dev/joy-of-cooking/spam' |
160 | 107 | text='Spam'>] | 104 | text='Spam'>] |
161 | 108 | 105 | ||
162 | @@ -114,59 +111,45 @@ | |||
163 | 114 | ... 'http://launchpad.dev/+cooker/jamie', | 111 | ... 'http://launchpad.dev/+cooker/jamie', |
164 | 115 | ... [root, cooker]) | 112 | ... [root, cooker]) |
165 | 116 | 113 | ||
168 | 117 | >>> provideAdapter( | 114 | >>> provideAdapter(TextualBreadcrumb, [ICooker], IBreadcrumb) |
167 | 118 | ... TextualBreadcrumbBuilder, [ICooker], IBreadcrumbBuilder) | ||
169 | 119 | 115 | ||
170 | 120 | >>> cooker_hierarchy = getMultiAdapter( | 116 | >>> cooker_hierarchy = getMultiAdapter( |
171 | 121 | ... (cooker, cooker_request), name='+hierarchy') | 117 | ... (cooker, cooker_request), name='+hierarchy') |
172 | 122 | >>> cooker_hierarchy.items() | 118 | >>> cooker_hierarchy.items() |
174 | 123 | [<Breadcrumb url='http://launchpad.dev/+cooker/jamie' text='Jamie'>] | 119 | [<TextualBreadcrumb url='http://launchpad.dev/+cooker/jamie' text='Jamie'>] |
175 | 124 | 120 | ||
176 | 125 | 121 | ||
177 | 126 | == Building IBreadcrumb objects == | 122 | == Building IBreadcrumb objects == |
178 | 127 | 123 | ||
186 | 128 | The construction of breadcrumb objects is handled by an | 124 | The construction of breadcrumb objects is handled by an IBreadcrumb adapter, |
187 | 129 | IBreadcrumbBuilder factory. The factory adapts a context object and | 125 | which adapts a context object and produces an IBreadcrumb object for that |
188 | 130 | produces an IBreadcrumb object for that context. | 126 | context. The default adapter provides the url attribute, but the breadcrumb's |
189 | 131 | 127 | text must be overriden in subclasses. | |
183 | 132 | The builder holds a temporary copy of a breadcrumb until the breadcrumb is in | ||
184 | 133 | a usable state. We can ask the builder for a finished breadcrumb after both | ||
185 | 134 | the text and the URL have been specified. | ||
190 | 135 | 128 | ||
191 | 136 | >>> from canonical.launchpad.webapp.interfaces import IBreadcrumb | 129 | >>> from canonical.launchpad.webapp.interfaces import IBreadcrumb |
192 | 137 | >>> from zope.interface.verify import verifyObject | 130 | >>> from zope.interface.verify import verifyObject |
201 | 138 | 131 | >>> breadcrumb = Breadcrumb(cookbook) | |
194 | 139 | >>> builder = BreadcrumbBuilder(cookbook) | ||
195 | 140 | >>> verifyObject(IBreadcrumbBuilder, builder) | ||
196 | 141 | True | ||
197 | 142 | |||
198 | 143 | >>> builder.text = 'Joy of cooking' | ||
199 | 144 | |||
200 | 145 | >>> breadcrumb = builder.make_breadcrumb() | ||
202 | 146 | >>> verifyObject(IBreadcrumb, breadcrumb) | 132 | >>> verifyObject(IBreadcrumb, breadcrumb) |
203 | 147 | True | 133 | True |
204 | 134 | >>> print breadcrumb.text | ||
205 | 135 | None | ||
206 | 148 | >>> breadcrumb | 136 | >>> breadcrumb |
207 | 149 | <Breadcrumb | 137 | <Breadcrumb |
208 | 150 | url='http://launchpad.dev/joy-of-cooking' | 138 | url='http://launchpad.dev/joy-of-cooking' |
210 | 151 | text='Joy of cooking'> | 139 | text='None'> |
211 | 152 | 140 | ||
214 | 153 | The breadcrumb's attributes can be overridden with subclassing and | 141 | As said above, the breadcrumb's attributes can be overridden with subclassing |
215 | 154 | Python properties. | 142 | and Python properties. |
216 | 155 | 143 | ||
217 | 156 | >>> from canonical.launchpad.webapp.publisher import canonical_url | 144 | >>> from canonical.launchpad.webapp.publisher import canonical_url |
220 | 157 | 145 | >>> class DynamicBreadcrumb(Breadcrumb): | |
219 | 158 | >>> class DynamicBreadcrumbBuilder(BreadcrumbBuilder): | ||
221 | 159 | ... @property | 146 | ... @property |
222 | 160 | ... def text(self): | 147 | ... def text(self): |
223 | 161 | ... return self.context.name.capitalize().replace('-', ' ') | 148 | ... return self.context.name.capitalize().replace('-', ' ') |
224 | 162 | ... | ||
225 | 163 | ... @property | ||
226 | 164 | ... def url(self): | ||
227 | 165 | ... return canonical_url(self.context) | ||
228 | 166 | 149 | ||
232 | 167 | >>> builder = DynamicBreadcrumbBuilder(cookbook) | 150 | >>> breadcrumb = DynamicBreadcrumb(cookbook) |
233 | 168 | >>> builder.make_breadcrumb() | 151 | >>> breadcrumb |
234 | 169 | <Breadcrumb | 152 | <DynamicBreadcrumb |
235 | 170 | url='http://launchpad.dev/joy-of-cooking' | 153 | url='http://launchpad.dev/joy-of-cooking' |
236 | 171 | text='Joy of cooking'> | 154 | text='Joy of cooking'> |
237 | 172 | 155 | ||
238 | @@ -186,9 +169,9 @@ | |||
239 | 186 | >>> provideAdapter( | 169 | >>> provideAdapter( |
240 | 187 | ... RecipeImageDisplayAPI, [IRecipe], IPathAdapter, 'image') | 170 | ... RecipeImageDisplayAPI, [IRecipe], IPathAdapter, 'image') |
241 | 188 | 171 | ||
245 | 189 | >>> builder = DynamicBreadcrumbBuilder(recipe) | 172 | >>> breadcrumb = DynamicBreadcrumb(recipe) |
246 | 190 | >>> builder.make_breadcrumb() | 173 | >>> breadcrumb |
247 | 191 | <Breadcrumb | 174 | <DynamicBreadcrumb |
248 | 192 | url='http://launchpad.dev/joy-of-cooking/spam' | 175 | url='http://launchpad.dev/joy-of-cooking/spam' |
249 | 193 | text='Spam' | 176 | text='Spam' |
250 | 194 | icon='<img src="/@@/recipe"/>'> | 177 | icon='<img src="/@@/recipe"/>'> |
251 | @@ -204,9 +187,9 @@ | |||
252 | 204 | >>> print queryAdapter(cookbook, IPathAdapter, name='image').icon() | 187 | >>> print queryAdapter(cookbook, IPathAdapter, name='image').icon() |
253 | 205 | None | 188 | None |
254 | 206 | 189 | ||
258 | 207 | >>> builder = DynamicBreadcrumbBuilder(cookbook) | 190 | >>> breadcrumb = DynamicBreadcrumb(cookbook) |
259 | 208 | >>> builder.make_breadcrumb() | 191 | >>> breadcrumb |
260 | 209 | <Breadcrumb | 192 | <DynamicBreadcrumb |
261 | 210 | url='http://launchpad.dev/joy-of-cooking' | 193 | url='http://launchpad.dev/joy-of-cooking' |
262 | 211 | text='Joy of cooking'> | 194 | text='Joy of cooking'> |
263 | 212 | 195 | ||
264 | @@ -221,7 +204,6 @@ | |||
265 | 221 | consistency across the site. | 204 | consistency across the site. |
266 | 222 | 205 | ||
267 | 223 | >>> from canonical.launchpad.browser.launchpad import Hierarchy | 206 | >>> from canonical.launchpad.browser.launchpad import Hierarchy |
268 | 224 | |||
269 | 225 | >>> class CustomHierarchy(Hierarchy): | 207 | >>> class CustomHierarchy(Hierarchy): |
270 | 226 | ... @property | 208 | ... @property |
271 | 227 | ... def objects(self): | 209 | ... def objects(self): |
272 | @@ -229,7 +211,7 @@ | |||
273 | 229 | 211 | ||
274 | 230 | >>> spammy_hierarchy = CustomHierarchy(root, request) | 212 | >>> spammy_hierarchy = CustomHierarchy(root, request) |
275 | 231 | >>> spammy_hierarchy.items() | 213 | >>> spammy_hierarchy.items() |
277 | 232 | [<Breadcrumb | 214 | [<TextualBreadcrumb |
278 | 233 | url='http://launchpad.dev/joy-of-cooking/spam' | 215 | url='http://launchpad.dev/joy-of-cooking/spam' |
279 | 234 | text='Spam' | 216 | text='Spam' |
280 | 235 | icon='<img src="/@@/recipe"/>'>] | 217 | icon='<img src="/@@/recipe"/>'>] |
281 | @@ -276,7 +258,7 @@ | |||
282 | 276 | >>> breadcrumb_no_icon, breadcrumb_with_icon = hierarchy.items() | 258 | >>> breadcrumb_no_icon, breadcrumb_with_icon = hierarchy.items() |
283 | 277 | 259 | ||
284 | 278 | >>> breadcrumb_no_icon | 260 | >>> breadcrumb_no_icon |
286 | 279 | <Breadcrumb | 261 | <TextualBreadcrumb |
287 | 280 | url='http://launchpad.dev/joy-of-cooking' | 262 | url='http://launchpad.dev/joy-of-cooking' |
288 | 281 | text='Joy of cooking'> | 263 | text='Joy of cooking'> |
289 | 282 | 264 | ||
290 | @@ -284,7 +266,7 @@ | |||
291 | 284 | False | 266 | False |
292 | 285 | 267 | ||
293 | 286 | >>> breadcrumb_with_icon | 268 | >>> breadcrumb_with_icon |
295 | 287 | <Breadcrumb | 269 | <TextualBreadcrumb |
296 | 288 | url='http://launchpad.dev/joy-of-cooking/spam' | 270 | url='http://launchpad.dev/joy-of-cooking/spam' |
297 | 289 | text='Spam' | 271 | text='Spam' |
298 | 290 | icon='<img src="/@@/recipe"/>'> | 272 | icon='<img src="/@@/recipe"/>'> |
299 | 291 | 273 | ||
300 | === modified file 'lib/canonical/launchpad/webapp/breadcrumb.py' | |||
301 | --- lib/canonical/launchpad/webapp/breadcrumb.py 2009-08-17 20:20:17 +0000 | |||
302 | +++ lib/canonical/launchpad/webapp/breadcrumb.py 2009-08-24 17:47:34 +0000 | |||
303 | @@ -7,7 +7,6 @@ | |||
304 | 7 | 7 | ||
305 | 8 | __all__ = [ | 8 | __all__ = [ |
306 | 9 | 'Breadcrumb', | 9 | 'Breadcrumb', |
307 | 10 | 'BreadcrumbBuilder', | ||
308 | 11 | ] | 10 | ] |
309 | 12 | 11 | ||
310 | 13 | 12 | ||
311 | @@ -16,38 +15,15 @@ | |||
312 | 16 | from zope.interface import implements | 15 | from zope.interface import implements |
313 | 17 | 16 | ||
314 | 18 | from canonical.launchpad.webapp import canonical_url | 17 | from canonical.launchpad.webapp import canonical_url |
317 | 19 | from canonical.launchpad.webapp.interfaces import ( | 18 | from canonical.launchpad.webapp.interfaces import IBreadcrumb |
316 | 20 | IBreadcrumb, IBreadcrumbBuilder) | ||
318 | 21 | 19 | ||
319 | 22 | 20 | ||
320 | 23 | class Breadcrumb: | 21 | class Breadcrumb: |
344 | 24 | """See `IBreadcrumb`.""" | 22 | """See `IBreadcrumb`. |
322 | 25 | implements(IBreadcrumb) | ||
323 | 26 | |||
324 | 27 | def __init__(self, url, text, icon=None): | ||
325 | 28 | self.url = url | ||
326 | 29 | self.text = text | ||
327 | 30 | self.icon = icon | ||
328 | 31 | |||
329 | 32 | def __repr__(self): | ||
330 | 33 | if self.icon is not None: | ||
331 | 34 | icon_repr = " icon='%s'" % self.icon | ||
332 | 35 | else: | ||
333 | 36 | icon_repr = "" | ||
334 | 37 | |||
335 | 38 | return "<%s url='%s' text='%s'%s>" % ( | ||
336 | 39 | self.__class__.__name__, self.url, self.text, icon_repr) | ||
337 | 40 | |||
338 | 41 | |||
339 | 42 | # XXX: salgado, 2009-08-17: Since this adapter now provides a default | ||
340 | 43 | # value for the 'url' attribute, we could easily convert it into an | ||
341 | 44 | # adapter for IBreadcrumb, just changing the Hierarchy view. | ||
342 | 45 | class BreadcrumbBuilder: | ||
343 | 46 | """See `IBreadcrumbBuilder`. | ||
345 | 47 | 23 | ||
346 | 48 | This class is intended for use as an adapter. | 24 | This class is intended for use as an adapter. |
347 | 49 | """ | 25 | """ |
349 | 50 | implements(IBreadcrumbBuilder) | 26 | implements(IBreadcrumb) |
350 | 51 | 27 | ||
351 | 52 | rootsite = 'mainsite' | 28 | rootsite = 'mainsite' |
352 | 53 | text = None | 29 | text = None |
353 | @@ -66,14 +42,11 @@ | |||
354 | 66 | return queryAdapter( | 42 | return queryAdapter( |
355 | 67 | self.context, IPathAdapter, name='image').icon() | 43 | self.context, IPathAdapter, name='image').icon() |
356 | 68 | 44 | ||
366 | 69 | def make_breadcrumb(self): | 45 | def __repr__(self): |
367 | 70 | """See `IBreadcrumbBuilder.`""" | 46 | if self.icon is not None: |
368 | 71 | if self.text is None: | 47 | icon_repr = " icon='%s'" % self.icon |
369 | 72 | raise AssertionError( | 48 | else: |
370 | 73 | "The builder has not been given valid text for the " | 49 | icon_repr = "" |
362 | 74 | "breadcrumb.") | ||
363 | 75 | if self.url is None: | ||
364 | 76 | raise AssertionError( | ||
365 | 77 | "The builder has not been given a valid breadcrumb URL.") | ||
371 | 78 | 50 | ||
373 | 79 | return Breadcrumb(self.url, self.text, icon=self.icon) | 51 | return "<%s url='%s' text='%s'%s>" % ( |
374 | 52 | self.__class__.__name__, self.url, self.text, icon_repr) | ||
375 | 80 | 53 | ||
376 | === modified file 'lib/canonical/launchpad/webapp/interfaces.py' | |||
377 | --- lib/canonical/launchpad/webapp/interfaces.py 2009-08-18 14:36:16 +0000 | |||
378 | +++ lib/canonical/launchpad/webapp/interfaces.py 2009-08-24 17:47:34 +0000 | |||
379 | @@ -259,17 +259,6 @@ | |||
380 | 259 | icon = Attribute("An <img> tag showing this breadcrumb's 14x14 icon.") | 259 | icon = Attribute("An <img> tag showing this breadcrumb's 14x14 icon.") |
381 | 260 | 260 | ||
382 | 261 | 261 | ||
383 | 262 | class IBreadcrumbBuilder(IBreadcrumb): | ||
384 | 263 | """An object that builds `IBreadcrumb` objects.""" | ||
385 | 264 | # We subclass IBreadcrumb to minimize interface drift. | ||
386 | 265 | |||
387 | 266 | def make_breadcrumb(): | ||
388 | 267 | """Return an object implementing the `IBreadcrumb` interface. | ||
389 | 268 | |||
390 | 269 | If for any reason no IBreadcrumb object can be created, return None. | ||
391 | 270 | """ | ||
392 | 271 | |||
393 | 272 | |||
394 | 273 | # | 262 | # |
395 | 274 | # Canonical URLs | 263 | # Canonical URLs |
396 | 275 | # | 264 | # |
397 | 276 | 265 | ||
398 | === modified file 'lib/canonical/launchpad/zcml/personproduct.zcml' | |||
399 | --- lib/canonical/launchpad/zcml/personproduct.zcml 2009-07-17 00:26:05 +0000 | |||
400 | +++ lib/canonical/launchpad/zcml/personproduct.zcml 2009-08-24 17:47:34 +0000 | |||
401 | @@ -28,9 +28,9 @@ | |||
402 | 28 | /> | 28 | /> |
403 | 29 | 29 | ||
404 | 30 | <adapter | 30 | <adapter |
406 | 31 | provides="canonical.launchpad.webapp.interfaces.IBreadcrumbBuilder" | 31 | provides="canonical.launchpad.webapp.interfaces.IBreadcrumb" |
407 | 32 | for="canonical.launchpad.interfaces.personproduct.IPersonProduct" | 32 | for="canonical.launchpad.interfaces.personproduct.IPersonProduct" |
409 | 33 | factory="canonical.launchpad.browser.personproduct.PersonProductBreadcrumbBuilder" | 33 | factory="canonical.launchpad.browser.personproduct.PersonProductBreadcrumb" |
410 | 34 | permission="zope.Public" | 34 | permission="zope.Public" |
411 | 35 | /> | 35 | /> |
412 | 36 | 36 | ||
413 | 37 | 37 |
= Summary =
When I redesigned our breadcrumb infrastructure I made the
BreadcbrumbBuilder abstraction unnecessary, but my branch was already
too big so I left it to be removed later.
== Proposed fix ==
Remove BreadcrumbBuilder by directly adapting objects into IBreadcrumbs,
instead of adapting them into IBreadcrumbBuilder and calling
make_breadcrumb() on it.
In order to actually complete this I had to change all existing adapters
so that they inherit from Breadcrumb rather than BreadcrumbBuilder, but
that brought the diff to 1.4KLOC, so I'll ask for a review of that
later.
== Pre-implementation notes ==
== Implementation details ==
== Tests ==
./bin/test -vvt hierarchical- menu.txt
== Demo and Q/A ==
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files: /launchpad/ webapp/ interfaces. py /launchpad/ browser/ launchpad. py /launchpad/ webapp/ breadcrumb. py /launchpad/ zcml/personprod uct.zcml /launchpad/ browser/ personproduct. py /launchpad/ doc/hierarchica l-menu. txt
lib/canonical
lib/canonical
lib/canonical
lib/canonical
lib/canonical
lib/canonical
== Pyflakes Doctest notices ==
lib/canonical/ launchpad/ doc/hierarchica l-menu. txt
144: 'canonical_url' imported but unused
This has been removed already.
== Pylint notices ==
lib/canonical/ launchpad/ browser/ launchpad. py
103: [F0401] Unable to import 'lazr.uri' (No module named uri)