Inline editor pops open during close animation

[njx]

1. Open Getting Started project
2. Open index.html
3. Put cursor in <h1> tag
4. Cmd-E to open inline editor
5. Hit Esc

Result: The inline editor animates closed, but then pops open before disappearing. This doesn't seem to repro in Release 41.

[njx]

Should be fixed for release 42 since it's a recent regression. Setting medium priority. I wasn't able to bisect this fully because a lot of the interim commits in the themes work seem to make Brackets not start up for me, but it does seem like it was something in themes that broke it, so assigning to @dangoor for now - feel free to reassign.

[dangoor]

cc @MiguelCastillo

[peterflynn]

@njx Git-guru question for you :-) I wonder if there's any way to run a git bisect that only follows one side of merge commits, such that you're just bisecting along the historical values of master rather than all commits in the history log. That way you never get any states from the unfinished middle of a pull request -- only states that represent the final, reviewed merges.

[redmunds]

You can use git bisect skip, but it seems to give you the next linear commit, so sometimes it takes a while to hit the next test point.

[peterflynn]

That's pretty cool though. I wonder if you could use a shell script to repeatedly call skip in a loop until you hit a commit with "Merge pull request #..." in the commit message...

[njx]

@peterflynn It doesn't look like there's a way to do what you were asking in your previous comment, although that does seem useful. (Although really the right answer is that ideally the app would run after each commit :)) You can specify a set of ranges to skip, so if you know that there's a specific PR that had a lot of in-between commits you don't care about, you could list the range of commits within that PR.

In this case, it's weird because there was a pretty large range of commits for which Brackets didn't seem to start up properly - it came up partially but didn't show any files, and I didn't see errors in the console. It's hard for me to believe that Brackets didn't run after most of those commits on the developer's machine, so I'm thinking there must have been something else going on as I was bisecting. (I tried clearing my cache a couple of times but that didn't seem to help. I also made sure my submodules were up to date after each bisect step.)

[MiguelCastillo]

So I just checkout a new master and did a hard reset to ce7e691, which the check in right before themes was merged. The issue does not happen there.

git reset --hard ce7e691f7347279e36a2e6d5842a41a2a21e9975

I am digging through right now.

[MiguelCastillo]

Actually, I can reproduce the issue with when I reset my branch to ce7e691 and installing my themes extension. So, that at least tells me that it is not any of the integration points into core brackets.

[MiguelCastillo]

So, the problem is in this call https://github.com/adobe/brackets/blob/master/src/view/ThemeManager.js#L206

Gotta figure out what to do about it now. :)

[peterflynn]

@MiguelCastillo Do you know why it's getting called? It doesn't seem like closing an inline editor should do anything that would make ThemeManager think it needs to refresh stuff... Is it dispatching a spurious preference-change event or something?

[RaymondLim]

Closing an inline editor triggers activeEditorChange event and ThemeManager is refreshing the editor on that event. But ThemeManager should not refresh if switching from inline editor to the main editor.

[MiguelCastillo]

Yeah, there were issues in the past where inline widgets would render their gutters correctly, especially for themes that added padding to the gutter. I never could really trace it down but forcing an editor resize would basically cause things to redraw and themes would work then.

I am going to some more testing to narrow down the problem this is supposed to address...

[MiguelCastillo]

Correction... Would render their gutter incorrectly

[MiguelCastillo]

Found the initial issue form themes MiguelCastillo/Brackets-Themes#5

[peterflynn]

Yikes -- it would be nice to avoid doing that on activeEditorChange... it's a noticeable performance hit, and when switching between full editors, EditorManager actually already does this -- so it would be redundant, too.

[MiguelCastillo]

@peterflynn I know, I am not proud... I am looking into removing that whole refresh routine. So far tests look really promising.

[MiguelCastillo]

I have been running tests on the solarized theme the issue was initially logged against and tested against. I have not ran into the issue yet. I am going to continue testing for a bit tonight, but I am most likely going to submit another PR where I remove the entire refreshEditor routine.

It actually seems like CM fixed the issue on their end.

[MiguelCastillo]

The issue I refer to above is the issue that needed the editor refresh...

[MiguelCastillo]

Sorry for the double PR.

As it turns out, I can reproduce some weird behavior with an old version of the solarized theme. The newer version of solarized has had the troublesome padding removed, the issue no longer occurs there. codemirror/codemirror5@fbf533c

The interesting thing is that I can reproduce the issue with and without the refreshEditor routine. So, I removed the refreshEditor routine. I will investigate what we can regarding the old solarized theme and submit a separate PR for it if it makes sense.

Any objections?

[dangoor]

The opening-during-closing is fixed, but there's a different problem with font sizes or something that I'll file separately.

FBNC to @njx

[njx]

Confirmed, closing.