loggerhead

Merge lp:~mwhudson/loggerhead/bug-383631 into lp:loggerhead

  1. bug-383631
  2. Merge into trunk-rich
Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: John A Meinel
Approved revision: 463
Merged at revision: 462
Proposed branch: lp:~mwhudson/loggerhead/bug-383631
Merge into: lp:loggerhead
Diff against target: 45 lines (+5/-5)
2 files modified
loggerhead/templates/breadcrumbs.pt (+1/-1)
loggerhead/util.py (+4/-4)
To merge this branch: bzr merge lp:~mwhudson/loggerhead/bug-383631
Reviewer Review Type Date Requested Status
John A Meinel Needs Information
Review via email: mp+87877@code.launchpad.net

Description of the change

This branch fixes the linked bug, twice over:

1) It changes the URLs of the in-branch breadcrumbs to be based on paths, like everything else in Loggerhead has been for years.

2) It removes 'file_id' from the set of query arguments that the (disgusting, I'd forgotten how horrible this bit was) machinery used by context_url preserves, now that we never ever generate a url with a file_id argument.

Cheers,
mwh

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/8/2012 10:54 PM, Michael Hudson-Doyle wrote:
> Michael Hudson-Doyle has proposed merging
> lp:~mwhudson/loggerhead/bug-383631 into lp:loggerhead.
>
> Requested reviews: Loggerhead Reviewers (loggerhead-reviewers)
> Related bugs: Bug #383631 in loggerhead: "Breadcrumbs while
> annotating a file in head shows the most recent revision in the url
> instead of head" https://bugs.launchpad.net/loggerhead/+bug/383631
>
> For more details, see:
> https://code.launchpad.net/~mwhudson/loggerhead/bug-383631/+merge/87877
>
> This branch fixes the linked bug, twice over:
>
> 1) It changes the URLs of the in-branch breadcrumbs to be based on
> paths, like everything else in Loggerhead has been for years.
>
> 2) It removes 'file_id' from the set of query arguments that the
> (disgusting, I'd forgotten how horrible this bit was) machinery
> used by context_url preserves, now that we never ever generate a
> url with a file_id argument.
>
> Cheers, mwh

I love getting rid of file_id in the URLs, but will this break
existing URLs if people are saving them somewhere? ISTR some people
wanted a good way to point people at :head of a file like README, etc.

I especially like this:
- - 'file_id': inv.path2id('/'.join(dir_parts[:index + 1])),
+ 'path': '/'.join(dir_parts[:index + 1]),

Internally it was already doing paths, and forcing a translation to
file_id at this part.

Anyway, if it is incompatible, is there a way we could try to migrate
gracefully?

 review: needsinfo

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8W3/UACgkQJdeBCYSNAAOPCQCfX+WJIFLOagJvmKJnLy+/tKXJ
snsAn0jlmnE6b3DHidXeMLPZLnOIUaP0
=UHk1
-----END PGP SIGNATURE-----

review: Needs Information
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

On Wed, 18 Jan 2012 16:06:29 +0100, John Arbash Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 1/8/2012 10:54 PM, Michael Hudson-Doyle wrote:
> > Michael Hudson-Doyle has proposed merging
> > lp:~mwhudson/loggerhead/bug-383631 into lp:loggerhead.
> >
> > Requested reviews: Loggerhead Reviewers (loggerhead-reviewers)
> > Related bugs: Bug #383631 in loggerhead: "Breadcrumbs while
> > annotating a file in head shows the most recent revision in the url
> > instead of head" https://bugs.launchpad.net/loggerhead/+bug/383631
> >
> > For more details, see:
> > https://code.launchpad.net/~mwhudson/loggerhead/bug-383631/+merge/87877
> >
> > This branch fixes the linked bug, twice over:
> >
> > 1) It changes the URLs of the in-branch breadcrumbs to be based on
> > paths, like everything else in Loggerhead has been for years.
> >
> > 2) It removes 'file_id' from the set of query arguments that the
> > (disgusting, I'd forgotten how horrible this bit was) machinery
> > used by context_url preserves, now that we never ever generate a
> > url with a file_id argument.
> >
> > Cheers, mwh
>
> I love getting rid of file_id in the URLs, but will this break
> existing URLs if people are saving them somewhere? ISTR some people
> wanted a good way to point people at :head of a file like README, etc.

This branch doesn't change how any URLs are processed, it just changes
the ones which are generated on the page. It will change where the
links on pages accessed via ?file_id=xxx go to, but from ones that are
broken to ones that work, so I think that's probably a good thing :-)

Cheers,
mwh

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'loggerhead/templates/breadcrumbs.pt'
2--- loggerhead/templates/breadcrumbs.pt 2008-09-30 02:04:14 +0000
3+++ loggerhead/templates/breadcrumbs.pt 2012-01-08 21:53:23 +0000
4@@ -10,7 +10,7 @@
5
6 <!-- Branch breadcrumbs (for the path within the branch) -->
7 <span metal:define-macro="branch" class="breadcrumb">
8- <tal:block repeat="crumb branch_breadcrumbs">/<a tal:attributes="href python:url([crumb['suffix'], change.revno], file_id=crumb['file_id'])" tal:content="crumb/dir_name" tal:condition="not:repeat/crumb/end" /><span tal:replace="crumb/dir_name" tal:condition="repeat/crumb/end" /></tal:block>
9+ <tal:block repeat="crumb branch_breadcrumbs">/<a tal:attributes="href python:url([crumb['suffix'], change.revno, crumb['path']])" tal:content="crumb/dir_name" tal:condition="not:repeat/crumb/end" /><span tal:replace="crumb/dir_name" tal:condition="repeat/crumb/end" /></tal:block>
10 </span>
11
12 <!-- Directory breadcrumbs (for the path leading up to the branch) -->
13
14=== modified file 'loggerhead/util.py'
15--- loggerhead/util.py 2011-09-08 00:33:28 +0000
16+++ loggerhead/util.py 2012-01-08 21:53:23 +0000
17@@ -226,7 +226,7 @@
18 def html_escape(s):
19 """Transform dangerous (X)HTML characters into entities.
20
21- Like cgi.escape, except also escaping " and '. This makes it safe to use
22+ Like cgi.escape, except also escaping \" and '. This makes it safe to use
23 in both attribute and element content.
24
25 If you want to safely fill a format string with escaped values, use
26@@ -484,7 +484,7 @@
27 for index, dir_name in enumerate(dir_parts):
28 inner_breadcrumbs.append({
29 'dir_name': dir_name,
30- 'file_id': inv.path2id('/'.join(dir_parts[:index + 1])),
31+ 'path': '/'.join(dir_parts[:index + 1]),
32 'suffix': '/' + view,
33 })
34 return inner_breadcrumbs
35@@ -555,8 +555,8 @@
36 # for re-ordering an existing page by different sort
37
38 t_context = threading.local()
39-_valid = ('start_revid', 'file_id', 'filter_file_id', 'q', 'remember',
40- 'compare_revid', 'sort')
41+_valid = (
42+ 'start_revid', 'filter_file_id', 'q', 'remember', 'compare_revid', 'sort')
43
44
45 def set_context(map):

Subscribers

People subscribed via source and target branches