Fix prepending multiple gutters at once #13272

Merged
merged 1 commit into from Dec 12, 2016

Projects

None yet

5 participants

@vjeux
Contributor
vjeux commented Nov 19, 2016

There's a bug when multiple gutters are prepended at once where their order is not correctly preserved in the dom. The issue is that we do not update indexInOldGutters even though we inserted a dom node in the old gutters dom.

Honestly, I'm unconvinced that this entire logic is correct, but this fixes the use case we have in Nuclide so it's more correct than before :)

Released under CC0

@vjeux vjeux Fix prepending multiple gutters at once
There's a bug when multiple gutters are prepended at once where their order is not correctly preserved in the dom. The issue is that we do not update indexInOldGutters even though we inserted a dom node in the old gutters dom.

Honestly, I'm unconvinced that this entire logic is correct, but this fixes the use case we have in Nuclide so it's more correct than before :)

Released under CC0
c8a398e
@ungb
ungb commented Dec 5, 2016

Hey @vjeux,

Is there a way I can repro the issue in Nuclide? I wanted to test the fix if possible.

I'm not sure why AppVeyor is failing. looking at the logs, I didn't find anything.

@50Wliu 50Wliu added the needs-testing label Dec 6, 2016
@50Wliu
Member
50Wliu commented Dec 6, 2016

I'm not sure why AppVeyor is failing. looking at the logs, I didn't find anything.

It timed out for whatever reason. I restarted the build.

@vjeux
Contributor
vjeux commented Dec 6, 2016

If you open a file that has both diagnostics and breakpoint gutter, you can see that the diagnostics is left of the breakpoint even though diagnostic has priority -1000 ( https://github.com/facebook/nuclide/blob/master/pkg/nuclide-diagnostics-ui/lib/gutter.js#L79 ) and breakpoint -1100 ( https://github.com/facebook/nuclide/blob/master/pkg/nuclide-debugger/lib/BreakpointDisplayController.js#L63 )

screen shot 2016-12-06 at 8 13 34 am

@ungb
ungb commented Dec 12, 2016

Verified your change fixes the issue.

image

Will merge change today unless any one has any feedback on the change @atom/core.

@ungb ungb removed the needs-testing label Dec 12, 2016
@vjeux
Contributor
vjeux commented Dec 12, 2016

Thanks!

@iolsen iolsen merged commit f9c9681 into master Dec 12, 2016

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@iolsen iolsen deleted the fb-vjeux-fix-gutter-prepend branch Dec 12, 2016
@iolsen
Contributor
iolsen commented Dec 12, 2016

🎳 thanks, @vjeux!

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