Kallithea issues archive

Issue #91: code comments: links to 'next comment' should cross inline/commit boundary

Reported by: Thomas De Schampheleire
State: resolved
Created on: 2015-01-26 15:58
Updated on: 2015-04-29 21:42


Commit views contains at the top: '4 comments (2 inline) First comment'. The 'First comment' link points to the first inline comment. At the top of this first inline comment, a 'Next comment' link to the second inline comment is provided. However, there is no 'Next comment' from that last inline comment to the first general comment. This should be added, so that there is one chain to follow to see all comments on one commit.



Comment by Mads Kiilerich, on 2015-01-26 16:21

That could make sense. Sounds like an easy fix ;-)

Comment by Thomas De Schampheleire, on 2015-01-28 21:16

Sounds easy, but still I'm bumping into a stupid problem: I tried changing this line (in templates/pullrequests/pullrequest_show.html)

      linkInlineComments(document.getElementsByClassName('firstlink'), document.getElementsByClassName('inline-comment'));


      linkInlineComments(document.getElementsByClassName('firstlink'), document.getElementsByClassName('comment'));

which should work since all inline-comments also have class comment.

However, what I see is that the list of comments passed to function linkInlineComments contains the inline comments twice, and I have no idea why. So as example I have two inline comments (a, b) and three global comments (c,d,e). The list in the second argument to linkInlineComments then contains a1,b1,a2,b2,c,d,e. Where a1 and b1 refer to both classes inline-comment and comment, and a2,b2 only to comment.

I'm by far a Javascript expert, so any help is welcome here. I tried removing the inline-comment class altogether, but for some reason these inline comments are always present twice.

The result of them being present twice is that the first one points to the second, which then points back to the first.

With the above change, the global comments point to each other, and the first one's previous link does indeed point back to the last inline comment, so that part is fine.

Comment by Mads Kiilerich, on 2015-01-28 23:55

(A good example of how fancy markup of technical discussion is annnnnnoying and usually get it wrong unless people take en extra effort to make it "correct").

(This is also something it probably would be better to post on the mailing list so not only reach the few that are stupid enough to browse a bug tracker for fun.)

From the example you show, I can see that this is a part that haven't been jQuery-fiscated yet. Some might dislike it as unnecessary overhead, but I think it makes many things simpler and more predictable. My first step would thus probably be to change this code to jQuery, before trying to improve it.

The reason they appear twice could be that they first appear in one place in the DOM, then some javascript will copy them to the right inline location. (If the right inline location can't be found (because the diff context is smaller than when the comment was created), it should still remain in its old position. That can perhaps be used to find that location and to see if such comments also show twice.)

I guess one solution to the problem could be to specify a css-like selector to jQuery so it only look for comments that actually are inline in the actual diff.

Comment by Thomas De Schampheleire, on 2015-01-29 20:54

See pull request #81. The problem was indeed related to the moving/copying of inline elements, where the original elements remained. I fixed it by deleting the elements after moving/copying them.

Comment by Thomas De Schampheleire, on 2015-02-11 21:17

Fixed with commit 990ec5ed4ee5.

Comment by Paul Gorbas, on 2015-04-29 21:42

The comments in out system doesn't have ANY links to the next comments at all. At the top the bubble brings you to the first comment, and that's it - additional comments we have to manually scroll to view, I could really use that next comment link...

Also I do no think Replies to comments should bump the comment count.