Integrating Kallithea with TortoiseHg (plus some other comments/suggestions from my team)

[Angel Ezquerra]

On Mon, Feb 15, 2016 at 1:42 PM, Thomas De Schampheleire
<patrickdepinguin at gmail.com> wrote:
> On Mon, Feb 15, 2016 at 12:04 AM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 02/14/2016 07:38 PM, Angel Ezquerra wrote:
>>>
>>> Hi,
>>>
>>> on Friday I showed Kallithea to the people on my team and most really
>>> liked. However some said that they'd prefer to be able to do some (if
>>> not all) of the code reviews directly in their TortoiseHg client,
>>> which is what we use as our mercurial client.
>>
>>
>> Yes, that could be convenient. We have considered it too. A first challenge
>> was that most people actually rather would have integration with their
>> editor or merge tool ... or doesn't use TortoiseHg.
>>
>> I think the right answer to that (and other problems) is that review
>> comments really should be "first class objects" in the VCS. They should work
>> the same across all UIs ... and they should do The Right Thing when
>> changesets are amended or rebased.
>>
>>> In particular, these were some of the things they requested:
>>>
>>> 1. To be able to open the Kallithea review page for a given revision
>>> from TortoiseHg
>>>
>>> This should be pretty straightforward to do. I just need to add a
>>> Custom Tool that opens the corresponding URL in Kallithea.
>>>
>>> Ideally it should be possible to jump straight to the bottom of the
>>> code review page, where the Approve form is found. Looking at the
>>> current code I could just use the main_form id as the anchor for the
>>> html, that is:
>
> Odd request, see below.
>
>>>
>>> http://kallithea-server:port/REPOSITORY_PATH/CHANGESET#main_form
>>>
>>> Can we expect the main form id to remain unchanged in the foreseeable
>>> future?
>>
>>
>> I guess it is very likely we want to totally rework that page soon. But when
>> you control both client and server in your installation, you can always
>> tweak it to match eather or be backwards compatible.
>>
>>> In fact, it would be nice to be able to quickly access the approve
>>> form from the top of the page. Perhaps the "[Not approved]" text that
>>> appears at the top of the review page could be a link to the approve
>>> form?
>>
>>
>> That text is generally referring to that _other_ users didn't approve yet.
>> Having the link you describe could be confusing.
>>
>> But also, it seems wrong to encourage people to approve before looking at
>> the diff?
>>
>> (But also, Jan and Thomas have tweaks that kind of goes in the same
>> direction ...)
>
> It's a different goal, though.
>
> I find the request to skip directly to the approve button very
> strange, to be honest. Code review is not just about
> approving/rejecting, but also on giving code comments (which are most
> useful when placed inline in the code, really).
>
> Why would you want to skip to that button and give the impression that
> you cannot/should not comment on the code?


Both you and Mads make similar comments, so I´ll answer here to both of you :-)

It does not make sense, as you say, to want to directly approve or
reject a revision without reviewing it first.
However it does make sense to review the revision _outside_ Kallithea
(e.g. using TortoiseHg and your favorite editor) and then want to just
mark the revision as Approved or Rejected in Kallithea.
That is basically the reason for the request. People are used to their
tools, and they'd want to use them to review the code (in addition to
being able to do it offline). Ideally they'd like to be able to right
click on the revision on TortoiseHg and select "Approved" on some sort
of Kallithea or Code Review menu. Since that is not possible the
second best solution is to just open the page and jump straight to the
main form.

This is also the reason why I'd like to be able to jump to the main
form by adding a link somewhere at the top of the code review page and
so on.

>>> 2. To be able to jump straight to the review section corresponding to
>>> a particular file from TortoiseHg
>>>
>>> This does not seem possible at the moment. It seems that the anchors
>>> corresponding to changeset files on the changeset review page are not
>>> easy to generate. Ideally they should have a format of the sort
>>> CHANGESETID-URL_ENCODED_FILEPATH but they do not seem to follow that
>>> sort of convention :-(
>>
>>
>> Yes, I don't know why it is exactly the way it is. But the filepaths can be
>> very long and are currently repeated a lot of times. It could be changed ...
>> but should be done carefully as it would break links in existing comments
>> and mails and chats. You can perhaps from the beginning use a different
>> format in your setup. Or perhaps tweak it to have two set of anchors?
>>
>>> 3. To be able to mark a revision as "Approved" (or Rejected, etc) from
>>> TortoiseHg
>>>
>>> Step #1 above already kind of solves this request, but if we wanted to
>>> go for a more integrated solution it would be nice if we could do one
>>> of two things:
>>>
>>> - Open a page that only had the approve form for the selected
>>> changeset, and nothing else. We could then open it from TortoiseHg.
>>> - Use the JSON API to directly approve or reject a changeset. I
>>> checked the current JSON API but I did not see anything in there that
>>> would allow to do this. Ideally there should be an API call that let
>>> you set the revision state. We could then call it from TortoiseHg
>>> (using a custom tool or with a more integrated solution).
>>
>>
>> Yes, that could make sense.
>
> Again, I find it a very strange request. You'd essentially bypass all
> the good things that code review can bring you. Do you expect that all
> users will add all their comments about a change in the one box near
> the approve button? I think you'll get better quality reviews if
> people actually comment on the lines of the code.

See may comment above.