Clean up email formatting, add structured messages on issue page

- Keep track of which comments are attached to which messages, and expose it in the issue page UI
- Revamp emails to use the fact that the URL contains the filename:
   - Remove individual line links
   - Remove explicit listings of file names, replaced with the URL

[Ilia Mirkin, 2010-10-03 08:18:22]

Demo at http://36.latest.rietveld-test.appspot.com/1/

"Legacy" messages should still show up fine (in fact the first 3 messages there
were made with the old code). Obviously with old code running on the new data
model, you won't see the inline comments, but that scenario shouldn't really
happen.

[imirkin_alum.mit.edu, 2010-10-03 08:21:02]

[Fixing mailing list name]

On Sun, Oct 3, 2010 at 4:18 AM,  <ibmirkin@gmail.com> wrote:
> Reviewers: GvR,
>
> Message:
> Demo at http://36.latest.rietveld-test.appspot.com/1/
>
> "Legacy" messages should still show up fine (in fact the first 3
> messages there were made with the old code). Obviously with old code
> running on the new data model, you won't see the inline comments, but
> that scenario shouldn't really happen.
>
> Description:
> - Keep track of which comments are attached to which messages, and
> expose it in the issue page UI
> - Revamp emails to use the fact that the URL contains the filename:
>   - Remove individual line links
>   - Remove explicit listings of file names, replaced with the URL
>
>
> Please review this at http://codereview.appspot.com/2341045/
>
> Affected files:
>  M codereview/models.py
>  M codereview/views.py
>  M static/styles.css
>  M templates/issue.html
>  M templates/mails/comment.txt
>  M templates/mails/review.txt
>  M templates/publish.html
>
>
>



-- 
Ilia Mirkin
imirkin@alum.mit.edu

[imirkin_alum.mit.edu, 2010-11-06 01:47:00]

Ping?

Let me know if there's no interest in this, and I'll drop this patch.

Thanks,

  -ilia

On Sun, Oct 3, 2010 at 4:21 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> [Fixing mailing list name]
>
> On Sun, Oct 3, 2010 at 4:18 AM,  <ibmirkin@gmail.com> wrote:
>> Reviewers: GvR,
>>
>> Message:
>> Demo at http://36.latest.rietveld-test.appspot.com/1/
>>
>> "Legacy" messages should still show up fine (in fact the first 3
>> messages there were made with the old code). Obviously with old code
>> running on the new data model, you won't see the inline comments, but
>> that scenario shouldn't really happen.
>>
>> Description:
>> - Keep track of which comments are attached to which messages, and
>> expose it in the issue page UI
>> - Revamp emails to use the fact that the URL contains the filename:
>>   - Remove individual line links
>>   - Remove explicit listings of file names, replaced with the URL
>>
>>
>> Please review this at http://codereview.appspot.com/2341045/
>>
>> Affected files:
>>  M codereview/models.py
>>  M codereview/views.py
>>  M static/styles.css
>>  M templates/issue.html
>>  M templates/mails/comment.txt
>>  M templates/mails/review.txt
>>  M templates/publish.html
>>
>>
>>

[techtonik, 2010-11-10 12:24:16]

Where is the original issue on Rietveld to compare how it looked like before?
Can you put these links into issue description?

[Andi Albrecht, 2010-11-10 13:18:52]

techtonik@gmail.com writes:

> Where is the original issue on Rietveld to compare how it looked like
> before? Can you put these links into issue description?

It's in the first message. Ilia has setup a preview of this change here:
http://36.latest.rietveld-test.appspot.com/1/

-Andi

>
>
> http://codereview.appspot.com/2341045/
>
> --
> You received this message because you are subscribed to the Google Groups
"codereview-discuss" group.
> To post to this group, send email to codereview-discuss@googlegroups.com.
> To unsubscribe from this group, send email to
codereview-discuss+unsubscribe@googlegroups.com.
> For more options, visit this group at
http://groups.google.com/group/codereview-discuss?hl=en.

[Ilia Mirkin, 2010-11-13 10:28:31]

I've made some changes -- comment preview should be closer to what you
requested.

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

The context really pushes the comment off to the side, plus it's grayed out.
Perhaps we should only show the context when the message is expanded?

[imirkin_alum.mit.edu, 2010-11-13 10:32:43]

On Wed, Nov 10, 2010 at 7:24 AM,  <techtonik@gmail.com> wrote:
> Where is the original issue on Rietveld to compare how it looked like
> before? Can you put these links into issue description?

I would prefer not to. I consider links in the descriptions to be
semi-permanent, while it is quite likely that I will take down that
version and put up another one.

  -ilia

[Andi Albrecht, 2010-11-16 05:03:39]

On Sat, Nov 13, 2010 at 11:32 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Wed, Nov 10, 2010 at 7:24 AM,  <techtonik@gmail.com> wrote:
>> Where is the original issue on Rietveld to compare how it looked like
>> before? Can you put these links into issue description?
>
> I would prefer not to. I consider links in the descriptions to be
> semi-permanent, while it is quite likely that I will take down that
> version and put up another one.

Are there any opinions on this change?
I'd prefer to keep the plain text messages. OTOH this cleans up the UI a bit.

-Andi

>
>  -ilia
>
> --
> You received this message because you are subscribed to the Google Groups
"codereview-discuss" group.
> To post to this group, send email to codereview-discuss@googlegroups.com.
> To unsubscribe from this group, send email to
codereview-discuss+unsubscribe@googlegroups.com.
> For more options, visit this group at
http://groups.google.com/group/codereview-discuss?hl=en.
>
>

[imirkin_alum.mit.edu, 2010-11-16 05:14:27]

On Tue, Nov 16, 2010 at 12:03 AM, Andi Albrecht
<albrecht.andi@googlemail.com> wrote:
> On Sat, Nov 13, 2010 at 11:32 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Wed, Nov 10, 2010 at 7:24 AM,  <techtonik@gmail.com> wrote:
>>> Where is the original issue on Rietveld to compare how it looked like
>>> before? Can you put these links into issue description?
>>
>> I would prefer not to. I consider links in the descriptions to be
>> semi-permanent, while it is quite likely that I will take down that
>> version and put up another one.
>
> Are there any opinions on this change?
> I'd prefer to keep the plain text messages. OTOH this cleans up the UI a bit.

Perhaps I'm pointing out the obvious, but this basically allows us to
make emails more streamlined (fewer long links, just more readable in
general) while maintaining navigability from the web UI. That was one
of the main motivations of the change.

  -ilia

[GvR, 2010-11-16 05:29:16]

I'm okay with the general idea of displaying the messages that come from
Rietveld itself slightly different -- parsing stuff we sent ourselves is kind of
silly.  But my innocent little test shows a slight problem (at least when viewed
in FF): My comment ("asdf3") is not displayed *below* the line to which it
applies but after it, in a different style.  I don't like that.  I also find it
unintuitive that you must click on the line to show the quoted text -- it's a
fine feature to suppress the text by default and have a way to show it, but the
particular UI is too hidden (I discovered it by accident when I clicked on the
"Line 13" link and notice that it first expanded the comment and then followed
the link, apparently due to the Internet delay.)

[imirkin_alum.mit.edu, 2010-11-16 05:46:06]

On Tue, Nov 16, 2010 at 12:29 AM,  <gvanrossum@gmail.com> wrote:
> I'm okay with the general idea of displaying the messages that come from
> Rietveld itself slightly different -- parsing stuff we sent ourselves is
> kind of silly.  But my innocent little test shows a slight problem (at
> least when viewed in FF): My comment ("asdf3") is not displayed *below*
> the line to which it applies but after it, in a different style.  I
> don't like that.  I also find it unintuitive that you must click on the
> line to show the quoted text -- it's a fine feature to suppress the text
> by default and have a way to show it, but the particular UI is too
> hidden (I discovered it by accident when I clicked on the "Line 13" link
> and notice that it first expanded the comment and then followed the
> link, apparently due to the Internet delay.)

Hmmm... well it's the same UI as to expand messages on files as well
as in the top view (you just click the text and the comment expands).
Perhaps it would be more obvious if I did not show the line of code in
the preview before it is expanded?

-- 
Ilia Mirkin
imirkin@alum.mit.edu

[gvrpython, 2010-11-17 16:51:24]

On Mon, Nov 15, 2010 at 9:46 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Tue, Nov 16, 2010 at 12:29 AM,  <gvanrossum@gmail.com> wrote:
>> I'm okay with the general idea of displaying the messages that come from
>> Rietveld itself slightly different -- parsing stuff we sent ourselves is
>> kind of silly.  But my innocent little test shows a slight problem (at
>> least when viewed in FF): My comment ("asdf3") is not displayed *below*
>> the line to which it applies but after it, in a different style.  I
>> don't like that.  I also find it unintuitive that you must click on the
>> line to show the quoted text -- it's a fine feature to suppress the text
>> by default and have a way to show it, but the particular UI is too
>> hidden (I discovered it by accident when I clicked on the "Line 13" link
>> and notice that it first expanded the comment and then followed the
>> link, apparently due to the Internet delay.)
>
> Hmmm... well it's the same UI as to expand messages on files as well
> as in the top view (you just click the text and the comment expands).

But it's a totally different use case.

> Perhaps it would be more obvious if I did not show the line of code in
> the preview before it is expanded?

No, I like the line of code. I just don't like anything *next* to it
that is not code, even if it is in a different font.

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

[imirkin_alum.mit.edu, 2010-11-17 16:53:03]

On Wed, Nov 17, 2010 at 11:51 AM, Guido van Rossum <guido@python.org> wrote:
> On Mon, Nov 15, 2010 at 9:46 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> Perhaps it would be more obvious if I did not show the line of code in
>> the preview before it is expanded?
>
> No, I like the line of code. I just don't like anything *next* to it
> that is not code, even if it is in a different font.

My thought was to start out by basically emulating the current UI, but
with stuff linkified instead of having the full links spelled out.
Then we can circle back and do a more general UI improvement, which
becomes vastly easier when you have structured data.

-- 
Ilia Mirkin
imirkin@alum.mit.edu

[gvrpython, 2010-11-17 19:39:00]

On Wed, Nov 17, 2010 at 8:53 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Wed, Nov 17, 2010 at 11:51 AM, Guido van Rossum <guido@python.org> wrote:
>> On Mon, Nov 15, 2010 at 9:46 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>> Perhaps it would be more obvious if I did not show the line of code in
>>> the preview before it is expanded?
>>
>> No, I like the line of code. I just don't like anything *next* to it
>> that is not code, even if it is in a different font.
>
> My thought was to start out by basically emulating the current UI, but
> with stuff linkified instead of having the full links spelled out.
> Then we can circle back and do a more general UI improvement, which
> becomes vastly easier when you have structured data.

But that's not what your implementation looks like right now. It
displays the line of code and the reviewer's comment next to each
other, so (unless you have a fine eye for fonts) it's not clear where
the code ends and the comment begin.

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

[imirkin_alum.mit.edu, 2010-11-17 21:05:13]

On Wed, Nov 17, 2010 at 2:38 PM, Guido van Rossum <guido@python.org> wrote:
> On Wed, Nov 17, 2010 at 8:53 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Wed, Nov 17, 2010 at 11:51 AM, Guido van Rossum <guido@python.org> wrote:
>>> On Mon, Nov 15, 2010 at 9:46 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>>> Perhaps it would be more obvious if I did not show the line of code in
>>>> the preview before it is expanded?
>>>
>>> No, I like the line of code. I just don't like anything *next* to it
>>> that is not code, even if it is in a different font.
>>
>> My thought was to start out by basically emulating the current UI, but
>> with stuff linkified instead of having the full links spelled out.
>> Then we can circle back and do a more general UI improvement, which
>> becomes vastly easier when you have structured data.
>
> But that's not what your implementation looks like right now. It
> displays the line of code and the reviewer's comment next to each
> other, so (unless you have a fine eye for fonts) it's not clear where
> the code ends and the comment begin.

Just wanted to make sure you approved of the general direction before
I actually did anything :)

FWIW, I also don't like the code and the comment mashed together. I
didn't use to show the code at all in the earlier version of the
changeset.

[Ilia Mirkin, 2010-11-18 05:23:19]

Time for another look:

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

(which has just this change)

I've basically ripped out a lot of the auto-collapsing fanciness, and just show
everything always. A more sophisticated UI can be revisited in a future change.