Skip to content

Fixes: #2346 - duplicated inline annotations#2347

Merged
eamodio merged 1 commit intogitkraken:mainfrom
YonatanGreenfeld:2346-fix-duplicated-annotations
Nov 15, 2022
Merged

Fixes: #2346 - duplicated inline annotations#2347
eamodio merged 1 commit intogitkraken:mainfrom
YonatanGreenfeld:2346-fix-duplicated-annotations

Conversation

@YonatanGreenfeld
Copy link

@YonatanGreenfeld YonatanGreenfeld commented Nov 14, 2022

Description

Refs: #2346

In the case of multiple cursors (selections) in the same line, an annotation was created per selection, resulting in an odd experience. Filter out duplicated lines by the line number and commit sha (sha is a safety measure for the probably impossible case of 2 commits in the same line).

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@YonatanGreenfeld YonatanGreenfeld force-pushed the 2346-fix-duplicated-annotations branch 3 times, most recently from c27f87c to 2e77698 Compare November 15, 2022 07:30
Copy link
Member

@eamodio eamodio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

Since this is performance critical code, can you please change the following:

const commitLines = [
...filterMap<LineSelection, [number, GitCommit]>(selections, selection => {
const state = this.container.lineTracker.getState(selection.active);
if (state?.commit == null) {
Logger.debug(scope, `Line ${selection.active} returned no commit`);
return undefined;
}
return [selection.active, state.commit];
}),
];

To:

		const commitLines = new Map<number, GitCommit>();
		for (const selection of selections) {
			const state = this.container.lineTracker.getState(selection.active);
			if (state?.commit == null) {
				Logger.debug(scope, `Line ${selection.active} returned no commit`);
				continue;
			}

			commitLines.set(selection.active, state.commit);
		}

And then:

this.getPullRequests(
repoPath,
commitLines.filter(([, commit]) => !commit.isUncommitted),
{ timeout: timeout },
)

To:

				  this.getPullRequests(repoPath, [...filter(commitLines, ([, commit]) => !commit.isUncommitted)], {
						timeout: timeout,
				  })

Thanks!

@YonatanGreenfeld YonatanGreenfeld force-pushed the 2346-fix-duplicated-annotations branch 2 times, most recently from 0158e7d to d596a24 Compare November 15, 2022 07:59
@YonatanGreenfeld
Copy link
Author

Thank you for your contribution!

Since this is performance critical code, can you please change the following:

const commitLines = [
...filterMap<LineSelection, [number, GitCommit]>(selections, selection => {
const state = this.container.lineTracker.getState(selection.active);
if (state?.commit == null) {
Logger.debug(scope, `Line ${selection.active} returned no commit`);
return undefined;
}
return [selection.active, state.commit];
}),
];

To:

		const commitLines = new Map<number, GitCommit>();
		for (const selection of selections) {
			const state = this.container.lineTracker.getState(selection.active);
			if (state?.commit == null) {
				Logger.debug(scope, `Line ${selection.active} returned no commit`);
				continue;
			}

			commitLines.set(selection.active, state.commit);
		}

And then:

this.getPullRequests(
repoPath,
commitLines.filter(([, commit]) => !commit.isUncommitted),
{ timeout: timeout },
)

To:

				  this.getPullRequests(repoPath, [...filter(commitLines, ([, commit]) => !commit.isUncommitted)], {
						timeout: timeout,
				  })

Thanks!

Thank you for the detailed solution! I've pushed your suggestions

In case of multiple cursors (selections) in the same line, an
annotation was created per selection, resulting in an odd experience.
Filter out duplicated lines by the line number.

Add author to CHANGELOG and README

Fixes: gitkraken#2346
@YonatanGreenfeld YonatanGreenfeld force-pushed the 2346-fix-duplicated-annotations branch from d596a24 to 416bec5 Compare November 15, 2022 08:06
Copy link
Member

@eamodio eamodio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great -- TY!

@eamodio eamodio merged commit 1bf52c7 into gitkraken:main Nov 15, 2022
@eamodio eamodio added this to the 13.1 milestone Nov 15, 2022
@eamodio
Copy link
Member

eamodio commented Nov 15, 2022

Thank you for your contribution!

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inline annotations are duplicated when there are multiple selections in the same line

2 participants