Community
Participate
Working Groups
Steps: 1. Open a review 2. Click the Change-Id link in the Attributes section The review is opened again but the Reviewers and Patch Set sections are nore rendered (see screenshot).
Created attachment 232225 [details] screenshot
It appeared that the offline data was stored using the commit based ID rather than the numeric ID which seems odd: reviews_xml//org.eclipse.mylyn.gerrit-http-localhost--gerrit-2.5.4/Review/9.reviews reviews_xml//org.eclipse.mylyn.gerrit-http-localhost--gerrit-2.5.4/Review/Id3869b0dc9a008389a2b17de8a00e7394d4cd295.reviews
I'm confused. Is this an issue with Mylyn Reviews or the Tasks connector?
It's the Gerrit connector. It looks to me as if the task ID is not determined properly when the review is opened from the hyperlink.
Moving to Gerrit Connector.
(In reply to comment #2) > It appeared that the offline data was stored using the commit based ID rather > than the numeric ID which seems odd: > > reviews_xml//org.eclipse.mylyn.gerrit-http-localhost--gerrit-2.5.4/Review/9.reviews > reviews_xml//org.eclipse.mylyn.gerrit-http-localhost--gerrit-2.5.4/Review/Id3869b0dc9a008389a2b17de8a00e7394d4cd295.reviews That's really weird. I'm not sure what's going on here.
*** Bug 410535 has been marked as a duplicate of this bug. ***
So I thought this was using the URL handler, but it's not. It must have been long-standing -- we need to find some way to map the change id to the internal task Id so that it is intercepted before we try to get the task data, but it's not clear where in the Tsak framework we could do that.
Until recently, clicking that link started a job which would spin for a while and then refresh the review editor. Now it opens a second editor. When clicking a change-id link, the review should be opened with the correct title and all sections properly showing; if it's already open, the editor should be activated.
I'm not really sure how that ever worked -- when I traced through the code that trigerred the getTaskData call it looked like all Mylyn task internal, but I didn't look at all carefully. Sam, can you investigate while I work on bug 410397?
I assume that it's taskId parameter that is passed to GerritTaskDataHandler.getTaskData() that's the problem here. In case of the link it's not the numeric ID. The connector would need to map that before creating the task data.
I would not count on Tomek or I being able to work on this before June 19, but we can consider this for after that date. Note that this also happens when clicking the change-id in a git commit. After refreshing the editor, the patch sets and reviewers show up but the title is still wrong. I don't think this is higher priority than other things on 2.0.1 so I suggest lowering to P3. I notice that in the editor opened this way, the change-id is not a link. I wonder if that is a clue to what changed here.
(In reply to comment #13) > I would not count on Tomek or I being able to work on this before June 19, but > we can consider this for after that date. Note that this also happens when > clicking the change-id in a git commit. After refreshing the editor, the patch > sets and reviewers show up but the title is still wrong. I don't think this is > higher priority than other things on 2.0.1 so I suggest lowering to P3. Defects are assigned P2 (or higher) by default to make sure they appear on top of the plan: http://wiki.eclipse.org/Mylyn/Contributor_Reference#Triage . We don't always follow this consistently but particularly for major release it's helpful to make sure they get prioritized.
Thanks, I guess I should read through that again.
Postponing to 2.1 for now.
This actually __creates__ a second review in the task list with the change id as it's task id. I don't understand why we even render the change-id as a link (to the same review) at all, or why we show it in the attributes as well as the description.
(In reply to comment #17) > This actually __creates__ a second review in the task list with the change id > as it's task id. I don't understand why we even render the change-id as a link > (to the same review) at all, or why we show it in the attributes as well as the > description. Because the code properly set the task ID (vs key) when fetching task data before this regression was introduced.
But what is the point of making change-ids links?
(In reply to comment #19) > But what is the point of making change-ids links? It's just a side effect of using a rich text viewer with a hyperlink detector for rendering. The same problem applies for any change reference in the description or comments. I just used the attribute section as an example since it shows for every review and allows to reproduce the problem easily. Just try this: 1. Open https://git.eclipse.org/r/#/c/7193/ in the task editor 2. Click "I27c3b7daffda4beb5b5423c3f832ee2f552e4dfe" in the description
Right, but I'm suggesting that we shouldn't have a hyperlink detector that matches change-ids.
(In reply to Sam Davis from comment #21) > Right, but I'm suggesting that we shouldn't have a hyperlink detector that > matches change-ids. Yeah, but it's not entirely crazy. See Steffen's example -- you might very well have an embedded nasty hex string in a commit comment or something that you want to be able to click on..
I wouldn't expect a change id to be used anywhere other than the header of commit message. When looking at a review, the hyperlink detector just turns it into a link to the review you are already looking at, which isn't useful. I guess it's useful though in the EGit history view and commit viewer. I think it would be better to have an "Open Review" action in the context menu instead of having to hover over the change ID and select one of the "Open Task Iad6261ab4b153ecde99808b25aa330c73529270f in..." options but I guess it could be hard to add that to every view that might show a commit. So I think we should fix this but also consider removing change-id from the attributes section since it's redundant with the description.
I don't follow this argument. It's not uncommon to use change-ids to refer to other reviews. In that case I have often see the abbreviated form being used , e.g. see Iad6261ab . A bugfix in GerritTaskDataHandler is straight forward so I don't get why would change the UI and remove linking functionality due to a simple regression. (In reply to comment #23) > I wouldn't expect a change id to be used anywhere other than the header of > commit message. When looking at a review, the hyperlink detector just turns it > into a link to the review you are already looking at, which isn't useful. It is the only place in the UI that is guaranteed to shows the full change ID which is useful for copy and paste. > So I think we should fix this but also consider removing change-id from the > attributes section since it's redundant with the description. I disagree. There is no guarantee that the description will have a change id header. Before we waste more time discussing this I'll look into fixing this.
Proposed fix: https://git.eclipse.org/r/#/c/21885/.
Ok, I have never seen anyone use a change id that way and I would rather people standardize on the review id instead, but your argument makes sense. Thanks for submitting a fix.
(In reply to comment #26) > Ok, I have never seen anyone use a change id that way and I would rather people > standardize on the review id instead, but your argument makes sense. Thanks for > submitting a fix. The thing is that the numeric review IDs are actually deprecated (they are database ids). I think this is related to moving Gerrit towards a git backed review storage. Lucky for us the numeric IDs still work in the latest Gerrit but they may go way in the future.
The fix was merged.
