Hook up implementation of showing a mildly prettified unified diff on the issue

Hook up implementation of showing a mildly prettified unified diff on the issue page directly, make sure that the 'i' key goes to that rather than the patch page. [The latter is still available via the remaining link]

[Ilia Mirkin, 2010-11-13 21:36:04]

http://37.latest.rietveld-test.appspot.com/1/

Which actually includes all of the changes I've sent out thus far.

Note that this changes the behaviour of 'i'. I'd be OK with taking that out if
people felt strongly, or making it a user configurable option. I'm not a huge
fan of the "(show)" thing, but I didn't want to remove the commentable unified
diff view entirely, since I assume it was there for a reason.

I've had to comment out some code which was designed around a "show all unified
diffs" feature which is a little harder to implement given that there are
multiple patchsets, etc. [But it's not like that code ever got called in
Rietveld...]

[GvR, 2010-11-17 16:47:05]

In the demo the (show) link gives me a page that says "false". Surely
something's not right there?

There are definitely situations where the commentable unified view is right. 
(It would be nice if it got the pretty file-based urls you did for the
side-by-side diff.)

I agree that (show) is ugly.  I wonder if we should do something more radical
like changing the "Unified diffs" and "side-by-side diffs" column headings so
that the sbs diff is first and linked to the filename.  Then in the unified
column we could have two links, "view" and "inline".  (Or maybe a different word
for "view" to remind people of the UI change. But what?)

[imirkin_alum.mit.edu, 2010-11-17 17:01:56]

On Wed, Nov 17, 2010 at 11:47 AM,  <gvanrossum@gmail.com> wrote:
> In the demo the (show) link gives me a page that says "false". Surely
> something's not right there?

What says "false"? It works fine for me (Chrome/Linux, but pretty sure
I've also tested this in Firefox/Linux, and the code that backs this
is the super-original code that was copied from s back in the day).
Browser/platform? Perhaps I messed up some silly integration point
with the code that was already in place?

>
> There are definitely situations where the commentable unified view is
> right.  (It would be nice if it got the pretty file-based urls you did
> for the side-by-side diff.)

I can throw it on my list. In my local repo, that view is gone, so I
don't have a patch I can just cherry-pick out :)

>
> I agree that (show) is ugly.  I wonder if we should do something more
> radical like changing the "Unified diffs" and "side-by-side diffs"
> column headings so that the sbs diff is first and linked to the
> filename.  Then in the unified column we could have two links, "view"
> and "inline".  (Or maybe a different word for "view" to remind people of
> the UI change. But what?)

In my local repo, the "unified diff" view is gone completely, and it
just says "filename (show)" with the show bringing out the view that
I've hooked up to 'i'. It's less than ideal in terms of discovery, as
you point out. But really it's really basically the same view as the
commentable unified view... perhaps I can figure out a way to iframe
the commentable view in there so that you have the full commenting
capability? (Except that the javascript will probably collide... ugh.
That's what we get for not using namespaces.)

How about a note above each patchset that just explains it for now?

-- 
Ilia Mirkin
imirkin@alum.mit.edu

[gvrpython, 2010-11-17 19:47:18]

On Wed, Nov 17, 2010 at 9:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Wed, Nov 17, 2010 at 11:47 AM,  <gvanrossum@gmail.com> wrote:
>> In the demo the (show) link gives me a page that says "false". Surely
>> something's not right there?
>
> What says "false"? It works fine for me (Chrome/Linux, but pretty sure
> I've also tested this in Firefox/Linux, and the code that backs this
> is the super-original code that was copied from s back in the day).
> Browser/platform? Perhaps I messed up some silly integration point
> with the code that was already in place?

Maybe it's unique to FF. I have FF 3.6.12 on a Mac (OS 10.5.8) and
when I click on the "(show)" link it takes me to a new nearly-empty
page saying "false". It works correctly on Chrome, so I guess it's a
JS issue.

>> There are definitely situations where the commentable unified view is
>> right.  (It would be nice if it got the pretty file-based urls you did
>> for the side-by-side diff.)
>
> I can throw it on my list. In my local repo, that view is gone, so I
> don't have a patch I can just cherry-pick out :)

Just keep that view as it is for now.

>> I agree that (show) is ugly.  I wonder if we should do something more
>> radical like changing the "Unified diffs" and "side-by-side diffs"
>> column headings so that the sbs diff is first and linked to the
>> filename.  Then in the unified column we could have two links, "view"
>> and "inline".  (Or maybe a different word for "view" to remind people of
>> the UI change. But what?)
>
> In my local repo, the "unified diff" view is gone completely, and it
> just says "filename (show)" with the show bringing out the view that
> I've hooked up to 'i'.

If you change (show) to (inline) that'll be better.

> It's less than ideal in terms of discovery, as
> you point out. But really it's really basically the same view as the
> commentable unified view...

Except it's not commentable and doesn't show comments.

>  perhaps I can figure out a way to iframe
> the commentable view in there so that you have the full commenting
> capability? (Except that the javascript will probably collide... ugh.
> That's what we get for not using namespaces.)

I hate iframes. I think that having the inline view not commentable is
fine; adding draft comments to this view probably makes it too busy
anyway.

> How about a note above each patchset that just explains it for now?

What would the note say? In general I hate UIs that require users to
read sentences to figure out what they can do.

-- 
--Guido van Rossum (python.org/~guido)

[imirkin_alum.mit.edu, 2010-11-17 22:39:40]

On Wed, Nov 17, 2010 at 2:46 PM, Guido van Rossum <guido@python.org> wrote:
> On Wed, Nov 17, 2010 at 9:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Wed, Nov 17, 2010 at 11:47 AM,  <gvanrossum@gmail.com> wrote:
>>> In the demo the (show) link gives me a page that says "false". Surely
>>> something's not right there?
>>
>> What says "false"? It works fine for me (Chrome/Linux, but pretty sure
>> I've also tested this in Firefox/Linux, and the code that backs this
>> is the super-original code that was copied from s back in the day).
>> Browser/platform? Perhaps I messed up some silly integration point
>> with the code that was already in place?
>
> Maybe it's unique to FF. I have FF 3.6.12 on a Mac (OS 10.5.8) and
> when I click on the "(show)" link it takes me to a new nearly-empty
> page saying "false". It works correctly on Chrome, so I guess it's a
> JS issue.

Problem debugged and has a known fix. Turns out it probably only
worked in Chrome due to some silliness on my part.

>
>>> There are definitely situations where the commentable unified view is
>>> right.  (It would be nice if it got the pretty file-based urls you did
>>> for the side-by-side diff.)
>>
>> I can throw it on my list. In my local repo, that view is gone, so I
>> don't have a patch I can just cherry-pick out :)
>
> Just keep that view as it is for now.

Done :)

>
>>> I agree that (show) is ugly.  I wonder if we should do something more
>>> radical like changing the "Unified diffs" and "side-by-side diffs"
>>> column headings so that the sbs diff is first and linked to the
>>> filename.  Then in the unified column we could have two links, "view"
>>> and "inline".  (Or maybe a different word for "view" to remind people of
>>> the UI change. But what?)
>>
>> In my local repo, the "unified diff" view is gone completely, and it
>> just says "filename (show)" with the show bringing out the view that
>> I've hooked up to 'i'.
>
> If you change (show) to (inline) that'll be better.

OK

>> How about a note above each patchset that just explains it for now?
>
> What would the note say? In general I hate UIs that require users to
> read sentences to figure out what they can do.

Nothing that you'd like to see :) Something awkward like """click on
"inline" to see a unified diff on this page, click on the filename for
a commentable view"""

-- 
Ilia Mirkin
imirkin@alum.mit.edu

[Ilia Mirkin, 2010-11-18 05:49:44]

http://37.latest.rietveld-test.appspot.com/1/

New code uploaded which fixes the bug that you saw with Firefox. Not sure how
the old code was supposed to work exactly... must have had something cleverer in
the href.