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

remove didAttach which causes SuggestionListElement to render twice #895

Merged
merged 3 commits into from Oct 3, 2017

Conversation

Projects
None yet
3 participants
@leroix
Contributor

leroix commented Oct 1, 2017

When the suggestion list first appears, the the SuggestionListElement render method is being called twice in a single frame. I think maybe this was put in place to ensure that the suggestion list always appears with the latest suggestions. I've noticed that the suggestion list generally appears to be correct without the calls to didAttach. However, there is one problem I've been able to reproduce. After a reload, on the first key stroke, I get

screen shot 2017-09-30 at 9 38 31 pm

I'll try to see if I can resolve this without introducing a second call to .render. Does anyone know off the top of their head if there are any other issues with removing this?

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 1, 2017

Contributor

Working on these failing tests...

Contributor

leroix commented Oct 1, 2017

Working on these failing tests...

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2017

Member

I've been able to reproduce that "more" issue as well on master.

Member

50Wliu commented Oct 2, 2017

I've been able to reproduce that "more" issue as well on master.

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 2, 2017

Contributor

@50Wliu That's odd. I was thinking the "more" issue was caused by the change in this PR.

Contributor

leroix commented Oct 2, 2017

@50Wliu That's odd. I was thinking the "more" issue was caused by the change in this PR.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2017

Member

Hmm I should be more explicit. I see it for a split second before the "correct" suggestion list appears. So maybe the second render was an attempt to fix that.

Member

50Wliu commented Oct 2, 2017

Hmm I should be more explicit. I see it for a split second before the "correct" suggestion list appears. So maybe the second render was an attempt to fix that.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 2, 2017

Contributor

Nice. Still a bit fuzzy on whether this is ready or not but I'm cool with making this change so long as we test it well.

Contributor

nathansobo commented Oct 2, 2017

Nice. Still a bit fuzzy on whether this is ready or not but I'm cool with making this change so long as we test it well.

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 2, 2017

Contributor

I think it's good to go pending figuring out why travis hates me.

Contributor

leroix commented Oct 2, 2017

I think it's good to go pending figuring out why travis hates me.

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 2, 2017

Contributor

There's an uncaught exception that happens during https://github.com/atom/autocomplete-plus/blob/master/spec/autocomplete-manager-integration-spec.js#L2343. Though, it doesn't cause the test to fail.

screen shot 2017-10-02 at 4 05 40 pm

Looking into it...

Contributor

leroix commented Oct 2, 2017

There's an uncaught exception that happens during https://github.com/atom/autocomplete-plus/blob/master/spec/autocomplete-manager-integration-spec.js#L2343. Though, it doesn't cause the test to fail.

screen shot 2017-10-02 at 4 05 40 pm

Looking into it...

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2017

Member

☝️ Might want to check out #830. That exception has been going on for a while but no one's been able to find a consistent reproduction scenario yet. Would be 💯 if you could fix it :).

Member

50Wliu commented Oct 2, 2017

☝️ Might want to check out #830. That exception has been going on for a while but no one's been able to find a consistent reproduction scenario yet. Would be 💯 if you could fix it :).

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 2, 2017

Contributor

@50Wliu nice, I'll dig more into what's happening during that test and see if I can figure out what may be causing selectedLi to not be defined.

These failures in travis only seem to happen on the stable build. If I run the specs locally with Atom master, everything passes.

Contributor

leroix commented Oct 2, 2017

@50Wliu nice, I'll dig more into what's happening during that test and see if I can figure out what may be causing selectedLi to not be defined.

These failures in travis only seem to happen on the stable build. If I run the specs locally with Atom master, everything passes.

@@ -328,7 +326,7 @@ module.exports = class SuggestionListElement {
if (wordContainer && wordContainer.offsetLeft) {
this.uiProps.marginLeft = -wordContainer.offsetLeft
}
if (!this.uiProps.itemHeight) {
if (!this.uiProps.itemHeight && this.selectedLi) {

This comment has been minimized.

@leroix

leroix Oct 3, 2017

Contributor

@50Wliu could the fix for #830 literally just be this?

This also had the added bonus of making all the tests pass.

@leroix

leroix Oct 3, 2017

Contributor

@50Wliu could the fix for #830 literally just be this?

This also had the added bonus of making all the tests pass.

This comment has been minimized.

@50Wliu

50Wliu Oct 3, 2017

Member

Could be! Though I haven't ever looked at the ac+ codebase so I don't know. Should get rid of the exception at the very least.

@50Wliu

50Wliu Oct 3, 2017

Member

Could be! Though I haven't ever looked at the ac+ codebase so I don't know. Should get rid of the exception at the very least.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 3, 2017

Contributor

@ungb You may want to test this out to make sure there are no situations in which we aren't rendering the suggestions list, but I say 🚢 at will @leroix .

Contributor

nathansobo commented Oct 3, 2017

@ungb You may want to test this out to make sure there are no situations in which we aren't rendering the suggestions list, but I say 🚢 at will @leroix .

@leroix leroix merged commit 9d7bad6 into atom:master Oct 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leroix leroix deleted the leroix:double-render-bug branch Oct 3, 2017

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