Use logical coordinates in DisplayBuffer

[as-cii]

This PR moves all the code that handles pixel positions into TextEditorPresenter, where it belongs. This is part of the effort to merge #8811, as we realized we needed the DisplayBuffer to be clean before proceeding with measurements.

This refactoring was quite big, so I'd ❤️ to gather some feedback on the changes I have made (both on code quality and regressions).

What's missing:

• Fix missing failing specs.
• Fix package specs (by shimming the original API).
• Maintain a scroll queue to preserve scroll history.
• Serialize scroll state in terms of logical coordinates.
• Confirm that packages work as expected.
• Confirm that atom core works as expected.
• Shim TextEditor APIs to use their TextEditorElement counterpart.
• Fix discovered bugs.

Thanks! 🙇

/cc: @nathansobo @atom/feedback

[as-cii]

Okay, the build is 💚 now and I think I have addressed all the concerns we had during our call. 🎉

I have found, however, a method on Marker which makes use of the display buffer's pixel conversion methods (i.e. Marker::getPixelRange) and a cursory search on GitHub showed that someone is actually using it 😞.

We cannot shim it to a corresponding method on the view, though, as we don't have any reference to TextEditor in DisplayBuffer: passing a TextEditor instance when constructing DisplayBuffer seems like a bad move to me (other than being an unhealthy dependency, I think we'd have serialization issues) so I went ahead and simply deleted it in a027772 and 7cd318f.

If we want to keep it, I think the only option would be to deprecate it for a few versions and then delete it afterwards (so that package developers have some time to upgrade). On the other hand I'd ❤️ to clean the DisplayBuffer as soon as possible... What do you think?

Other than that, I think this is ready to 🚢! @nathansobo: please, have one last 👀.

Thanks everyone for the feedback! 🙏

[nathansobo]

and a cursory search on GitHub showed that someone is actually using it

It wasn't in the public API right? How popular is the package in question? Maybe we should open an issue or even open a PR if it's a simple fix.

[nathansobo]

It's going to be so great to drop this.

[as-cii]

Agreed! 🎆

[nathansobo]

Gave this a quick scan. Ultimately if tests are green and the structure hasn't changed beyond our discussion, I'm good to merge once we drop deprecated methods from the docs.

[as-cii]

It wasn't in the public API right? How popular is the package in question? Maybe we should open an issue or even open a PR if it's a simple fix.

Nope, the method wasn't in the public API. The package in question is color-picker but I think the fix is straightforward enough to deserve a PR. I'll do that shortly and 🚢 this PR afterwards.

Thanks, @nathansobo!

[nathansobo]

💥🎉🇮🇹

[maxbrunsfeld]

Awesome!

[maxbrunsfeld]

@as-cii, apparently our internal CI goes red when deprecated methods are called by core or bundled packages. I actually didn't know this either, until now, but I guess it makes sense, because we don't want the somewhat-expensive Grim.deprecate calls to be happening in the next release. I temporarily commented out the Grim.deprecate calls that you added in TextEditor, just to get CI green.

When you get a chance, could you update core and bundled packages to use the new APIs? The deprecations occurred in these tests:

• find-and-replace
• symbols-view
• wrap-guide
• atom core
  • ::page{Up,Down}
  • ::selectPage{Up,Down}