core-ai-bot
commented
3 years ago Open core-ai-bot opened 3 years ago
core-ai-bot
commented
3 years ago core-ai-bot
commented
3 years ago 
I had the cold-folding extension already installed so when I tried quick-edit it animated quite nicely but the line number for the line that contains the function that i'm quickviewing shifts over to the left 1 char. The odd thing is, is that with the code-folding extension installed all line numbers shift one character to the left and then shift once character back. This happens in master but, in your branch, the line number of the line above the location of where quick view opens, the line numbers do not shift back. Another side effect with the code-folding extension is that the line numbers in quick edit don't match the location of the line numbers of the file being edited.
To see this, install the code-folding extension and open project/FileIndexManager.js and go to line 138 and quick edit the function "ProjectManager.shouldShow".
This is probably more of a problem with the code-folding extension than with this feature this extension does some funky shifting even in master -- it's just that, when used in conjunction with your feature, it's more noticeable. I'm just not sure if it's important enough to do anything about and how do we fix a 3rd party extension. We can discuss this at Monday's standup with the larger group if you want. Probably not worth fixing on our side.
Other than that so far this looks good. I want to take one more pass at the code review so I should be done looking at this by Monday afternoon.
core-ai-bot
commented
3 years ago
another interesting side effect is if you click on the function name and press ctrl+e the quick edit collapses then re-expands. This was sort of a no op without the transition since the quick edit window wasn't animated and, as such, you barely noticed it was closed and reopened unless you had the code-folding extension loaded.
core-ai-bot
commented
3 years ago
Finished with review. One nit and a couple of things to talk about before we merge.
core-ai-bot
commented
3 years ago 
Regarding closing and reopening, I think it's safest to do what we're doing now, because we don't keep the inline editor in sync if the user makes changes to the surrounding code--i.e., we don't know that they haven't changed the function name in the meantime. It seems like enough of an edge case that it's not worth optimizing for.
core-ai-bot
commented
3 years ago
Hmmm, the problem with the code folding extension is actually worse--all the collapse triangles actually disappear while the inline editor is open. That happens in master too, though (whereas the line number shift doesn't).
core-ai-bot
commented
3 years ago
Ready for re-review.
It turns out that we never updated the code that tries to keep inline and host editor gutter widths in sync when we transitioned to CodeMirror v3 months ago. I've fixed that, which seems to fix the line number offset problem. Please take a look and let me know.
The problem with the folding triangles going away when an inline editor has focus is actually a bug in the extension--it only creates the folding triangles in the active editor, so when focus switches between the full and inline editors, the triangles in the outer editor disappear. It would probably be better for the extension to just persist the marks in all open editors. I'll file a bug in his repo.
core-ai-bot
commented
3 years ago
I also pushed some additions to the existing InlineEditorProviders tests that check to make sure that after opening an inline editor, its content is in the DOM and its height is nontrivial, and also check that after closing the editor the content is no longer in the DOM. Please review those as well. Once that all looks good, please go ahead and merge. Thanks!
core-ai-bot
commented
3 years ago
Review complete.
core-ai-bot
commented
3 years ago
Makes it so inline editors slide open and closed smoothly. Avoids various race conditions that can occur while inline editors are opening or closing.
While this seems to be working well, there are a few things I'm not sure about.
transform: translateZ(0)on the inline editor during the animation. However, that causes the entire window to flicker sometimes when an animation completes. The hack of settingbackface-visibility: hiddenonbodyappears to fix this. I don't think this causes the entire window to use the GPU (I don't see any performance issues with it set vs. unset), but we should verify that.Editor.setInlineWidgetHeight()once on its host editor. That's sort of weird from an API point of view--if the implementor of the inline editor doesn't know s/he needs to call this, the editor will never open and the promise will never resolve. I think it might already have been true that all inline editors needed to call this function even before adding the animation, so this isn't necessarily a new problem, but it does make me think we should clean up the API here--maybe requiringonAdded()to return a desired initial height?load(), andonAdded(). Bothload()andonAdded()are theoretically part of the external API (they're stubbed in InlineWidget), butload()is only ever used by inline editor providers themselves immediately after calling the constructor; it's never called by Editor (onlyonAdded()is). On top of that, it seems like there was at least one bug where something that happens inMultiRangeInlineEditor.load()that used to work when everything was synchronous now needs to happen inonAdded()instead (though it's not clear why). I added a workaround for that specific case, but it seems like we need to be clear about what should happen at construction time (before being added to the DOM) vs. after being added.The question in my mind for the last two is whether we should do those cleanups now, or have a larger inline editor architecture cleanup that would deal with some of the other architectural messiness we've had for awhile as well (like InlineTextEditor, which is a sort of vestigial subclass that no one uses directly).
njx included the following code: https://github.com/adobe/brackets/pull/4421/commits