billsedison / vscode-gitlens

Supercharge the Git capabilities built into Visual Studio Code — Visualize code authorship at a glance via Git blame annotations and code lens, seamlessly navigate and explore Git repositories, gain valuable insights via powerful comparison commands, and so much more
http://gitlens.amod.io
MIT License
0 stars 17 forks source link

Line decorations are being synchronized incorrectly in the Splitted Diff View #120

Open cagdas001 opened 5 years ago

cagdas001 commented 5 years ago

Describe the bug A change has been implemented in order to sync comment line decorations immediately, in the splitted Diff View. (It was decorating upon window.onDidChangeActiveTextEditor event before)

However, it has an issue that is causing wrong line decorations

To Reproduce

  1. Go to Commit Search view
  2. Select one or several commit(s)
  3. Click on any file to open the splitted Diff View
  4. Find a section that this commit has made a change (added/deleted lines). The purpose is to have different contents on the same line numbers
  5. On the right side, select any line from 4th step and click Blue icon
  6. Login to BitBucket if necessary
  7. Click Add Comment and add a comment. The line should be decorated.
  8. On the left side, check the same line number with 7th step. It should be decorated too.
  9. Select the line mentioned in the 8th step and click Blue icon. You will not see any comment.

Expected behavior It should detect which line has a comment and decorate accordingly, instead of decorating directly the same line numbers of the other view

Actual Behaviour It decorates the same line numbers, even if there are different contents

Screencast 2019-01-01_04-28-04

Additional context Here is the related code block:

https://github.com/billsedison/vscode-gitlens/blob/tc-dev/src/annotations/commentsDecoratorController.ts#L141

        for (const editor of window.visibleTextEditors) {
            if (editor.viewColumn === undefined && this._activeEditor !== editor) {
                editor.setDecorations(this.bookmarkDecorationType, this.decorations);
                break;
            }
        }
billsedison commented 5 years ago

@cagdas001 Thanks for pointing this out. That's another problem to bind the comment with the content, similar to https://github.com/billsedison/vscode-gitlens/issues/118. I'm still not sure if it's easy to fix.

Please share your thoughts.

If it's not easy to fix, I think we can just simply disable a comment decoration on the left window.

billsedison commented 5 years ago

Your bug hunt payment: https://software.topcoder.com/review/actions/ViewProjectDetails?pid=30078156

cagdas001 commented 5 years ago

No problem to fix it. Currently comments' line numbers (returned from API endpoint), are directly decorated. We just need to keep a Map that stores source line (key) numbers against result line (value) numbers. Then we can find which line we should decorate actually. (even if document is edited)

Details about source and result lines: https://stackoverflow.com/questions/49370690/what-is-sourceline-resultline-in-git-blame-incremental-output

In addition, we had actually discussed this before #31

In my opinion, triggering in windows.onDidChangeActiveTextEditor is OK, but we can do some optimization to avoid fetching the data whenever it is active.

So this mechanism currently refreshes decorations on this event. When you switch to the left side, it will be decorated too. (however currently has issues, will be fixed after #119 )

But, as I said, if the preference is to be synced immediately, it's not that hard. However, in this case we should clarify something.

If you're working with too many open documents, second may cause some issues (I'm not sure yet)

billsedison commented 5 years ago

Thanks for your detailed explanation.

But, as I said, if the preference is to be synced immediately, it's not that hard. However, in this case we should clarify something.

This sync feature will work only in Diff View? (so two documents) Or in all open tabs?

Let's still make the sync work on all open tabs, because that is what we have done for now, the client was happy with that. If we only make the sync work in the Diff view, so it might cause some tabs have comment decorations, some don't have. I'm afraid it might confuse the user.

If you're working with too many open documents, second may cause some issues (I'm not sure yet)

Let's assume the client won't work with too many open documents for now.

billsedison commented 5 years ago

I merged the two PR as suggested.

For the commit 31b6570, on Bitbucket side, there are 21 comments on src/commands/showCommitSearch.ts. image

But on the VSCode side, I can only see one. image

It was correct previously.

Please check. Thanks

cagdas001 commented 5 years ago

@billsedison

Please make your test repo public again, I can't access it right now

billsedison commented 5 years ago

@cagdas001 Done, sorry for the trouble.

cagdas001 commented 5 years ago

@billsedison

There are 20 file-level comments, and 1 inline comment in showCommitSearch.ts, for commit 31b6570 (so total 21 comments)

Inline comment is on the 7th line, you can expand the file to see it on the BitBucket side

File-level comments are being shown at top of file, but most of them had been deleted for this file/commit. But their (deleted comments) replies are still being shown on BitBucket side

chrome_2019-01-21_17-19-50

This is not the case in the GitLens. It doesn't load the comment if its deleted, so you can't see replies of a deleted comment

https://github.com/billsedison/vscode-gitlens/blob/tc-dev/src/gitCommentService.ts#L337

...
if (!c!.deleted) {
    const comment = { Commit: commit } as Comment;
...