Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not execute compare action on mousedown events #9977

Merged
merged 1 commit into from Jun 11, 2020

Conversation

rafeca
Copy link
Contributor

@rafeca rafeca commented Jun 10, 2020

Closes #9971

Description

This PR removes the call to executeCompare() that was performed on mousdedown events against the compare branch list.

Context

Before this PR, two executeCompare() calls got executed when clicking on an item on the compare branch list: the first one happens on the mousedown event (the one I'm removing) and the second one on the click event (which happens after the mouseup event).

This caused an issue when the first executeCompare() call finished before the click event happened:

The executeCompare() action would update the filter text of the branch list with the selected branch name, causing the filter to be applied and therefore the branch list to get updated.

Then, when the second executeCompare() function was called it could potentially use another branch since the list elements could get updated.

In most situations where using a branch name as a filter won't match other branch names this didn't cause an issue, since after the first executeCompare(), the list of branches would only contain a single branch (the clicked branch) and the second executeCompare() action would be done against no item (and therefore ignored) or against the correct branch:

Screen Shot 2020-06-10 at 10 50 10

But this could cause an issue in some scenarios where a branch name is a subset of another branch name, for example when having a branch called X and another one called XY. In this case the initial list would sort the branches by most recent ones:

Screen Shot 2020-06-10 at 10 52 36

But once the user clicks on the X branch and the executeCompare() action gets called, the filtering gets applied and the order of displayed branches changes:

Screen Shot 2020-06-10 at 10 53 57

(this can be seen more clearly on the "Screenshots" section below).

About this PR

The call that I removed was introduced in #4412 to address an issue that is not present anymore in the compare branch UI (I've tested it with the changes of this PR), and I think that this is because with #4751 (which was merged a bit later) the whole need of the two executeCompare() calls is not needed anymore.

Still, the logic of the compare branches UI is quite complex and I had a bit of trouble understanding all the different scenarios and how things play together, so it's possible that I'm missing something (I also found a couple of existing bugs while testing this PR which I'll document separately).

Screenshots

Before:

before

After:

after

Release notes

Notes: [Fixed] Select Branch to Compare no longer selects wrong branches

@rafeca rafeca requested review from niik and outofambit June 10, 2020 09:59
@tierninho tierninho self-requested a review June 10, 2020 19:28
Copy link
Contributor

@tierninho tierninho left a comment

Choose a reason for hiding this comment

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

Nice job ✨ Verifying all looks good now with the changes.

Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

This PR was a joy to read @rafeca, great investigation and I really love that you provided links to where this was originally introduced, that always helps me build confidence in changes!

@niik niik self-assigned this Jun 11, 2020
@rafeca rafeca merged commit ba04922 into development Jun 11, 2020
@rafeca rafeca deleted the do-not-filter-before-comparing branch June 11, 2020 15:23
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.

Select Branch to Compare Selects Wrong Branch
3 participants