discussion on issue #77 (review icon position for in-line code comments)

[Thomas De Schampheleire]

On Mon, Jan 26, 2015 at 4:59 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 01/26/2015 02:41 AM, Sean Farley wrote:
>>
>> Mads Kiilerich writes:
>>>
>>> I also think it is odd that both Bitbucket, Github and Kallithea shows
>>> the line numbers for each line in a diff. It is usually worthless
>>> information when reviewing through the web. If using the actual source
>>> for reference and reviewing locally, the line number of the chunk is
>>> more releant than the line number of every line in the diff. I thus
>>> think we could make an even better solution if we didn't use horizontal
>>> space for both showing line numbers for every line and also leaving
>>> space for 'add comment' and 'link to this line' (and in the future
>>> perhaps also for annotate information). Perhaps by overlaying the latter
>>> on top of the first when hovering over a line.
>>
>> Ok, this paragraph lost me. I'm guessing you agree to "have a way to
>> link to a hunk"
>
>
> yes ... and also for linking to a specific line. But we do not necessarily
> have to use screen space to all the time show a link for every line of code
> we show on the screen.
>
>>   but posit that "the line number might not make sense
>> (especially on the web)"?
>
>
> Yes, as long as I can link to exactly the right line, it doesn't matter much
> what the actual number is.
>
> (I guess the main use case for showing line numbers next to every line is
> when doing pair programming / reviewing using the same screen and wanting to
> be able to talk about lines without pointing. When using a review tool you
> usually have to point and click at the line anyway - just quoting its number
> is a bad hyper reference.)
>
>> Why wouldn't the line numbers in the first column not match with the
>> line numbers in the previous version of the file? Same for the second
>> column and the diff'ed version of the file?
>
>
> I don't know why they wouldn't (and I don't see how that question is related
> to what I said). (But one common reason for that is that you are looking at
> a slightly different version of the file.)
>
> My point is just that it is a bit inefficient to use screen space to show a
> lot of line numbers where each one just one bigger than the previous. There
> is not a lot of information in that. Nobody needs an overview of line
> numbers like they need an overview of code. You know when you are looking
> for a line number, and you always do it like "now I want to know the line
> number of this line". Given that we have limited screen space and want to
> use it efficiently, I think we should consider if it could make sense to
> show something else instead of the line numbers.
>
>> I'm all for displaying more information (like you mention with annotate)
>> ESPECIALLY for a way to click back through history for that given hunk /
>> section of a file. That's getting off-topic for this discussion, though.
>
>
> It is still about how we can make the diff view "right" so all the relevant
> information and functionality is available and easy to use. We should avoid
> making micro optimizations if it doesn't help the bigger picture or
> conflicts with it.
>



Just saw that on github the review icon is shown on top of the
vertical line that separates the line number from the code. The
icon/clickable area is still fairly large. As far as I can see, this
covers all requirements a to e.

Random example:
https://github.com/AnneTheAgile/build-failure-analyzer-plugin/commit/4c87b6e8cfd656a4c0cefcdb1abb8b38b8cbd753

I haven't checked the implementation yet.

Best regards,
Thomas