TypeStrong / atom-typescript

The only TypeScript package you will ever need
https://atom.io/packages/atom-typescript
MIT License
1.13k stars 204 forks source link

SemanticView optimizations #1575

Closed aminya closed 3 years ago

aminya commented 3 years ago

Description:

Fixes #1570

In the long run, we should add the missing functionality to outline that makes you use a separate semantic view.

aminya commented 3 years ago

Could you please state the missing capabilities from the outline so I can move them to the outline and just remove SemanticsView from atom-typescript?

I recently added another set of optimizations to the outline which further increases its performance. I have also talked to some UI experts, and they have sent me a very complex level-aware lazy-rendered tree that has a virtualized scroller, and I am planning to use it inside the outline.

People are already complaining about the duplication and its performance drawbacks. I don't think this duplication is good for the ecosystem.

lierdakil commented 3 years ago

I don't think this duplication is good for the ecosystem.

I disagree. Competition begets innovation.

Anyway, I'm not going to agree to removing semantic view in the foreseeable future. For one, removing features isn't something to do lightly, even if those features are somewhat redundant. For two, it's not my work, and I need to have a really damn good reason to throw it out. People who prefer atom-ide's outline don't have to use semantic view, and vice versa -- the configuration literally boils down to enabling one tick and disabling another.

I've just pushed a17290dca5de7fd2cc06f95f05583b7654f60f9b to master which in my limited testing fixes the performance issues with onDidChangeCursorPosition event handler (it avoids costly reflows via some structure reshuffling and patches DOM directly instead of going through etch). I suggest rebasing and reverting its removal.

If you don't want to spend any more time on this pull request, let me know, I will tweak it myself when I find the time.

lierdakil commented 3 years ago

I've made some tweaks. If you could do a quick review and give me the green light that would be nice.

aminya commented 3 years ago

I don't think this duplication is good for the ecosystem.

I disagree. Competition begets innovation.

Anyway, I'm not going to agree to removing semantic view in the foreseeable future. For one, removing features isn't something to do lightly, even if those features are somewhat redundant. For two, it's not my work, and I need to have a really damn good reason to throw it out. People who prefer atom-ide's outline don't have to use semantic view, and vice versa -- the configuration literally boils down to enabling one tick and disabling another.

Why should we compete to achieve the very same thing while we could join forces and collaborate to make a better product that suits the needs of both projects?

I see that you have a very different perspective on the issue, which is different from how I think.

I wanted to help this project, and that's why I spent duplicating my optimization efforts just to help SemanticsView keep up with the outline. By suggesting moving the features from this repository to outline, I wanted to help all the languages benefit from this and improve Atom as a whole not just one project.

I've made some tweaks. If you could do a quick review and give me the green light that would be nice.

I was planning to move lineCountIfLarge to atom-ide-base, but now it is hardly integrated as a class method which makes this impossible.

Given my failed efforts for helping this project, feel free to use this branch as you wish.

lierdakil commented 3 years ago

Why should we compete to achieve the very same thing while we could join forces and collaborate to make a better product that suits the needs of both projects?

By the same logic we should abandon Atom and switch to VSCode, because both are similar products, and we should strive to make one-size-fits-all solution instead of fragmenting the ecosystem. I hope it's pretty obvious things rarely work out this way.

I was planning to move lineCountIfLarge to atom-ide-base, but now it is hardly integrated as a class method which makes this impossible.

Not impossible, it's actually pretty easy to factor out. As I said above, pulling from Atom config on module load is pretty bad, since config schema is still being initialized at that point, so there are no guarantees you'll get what you expect. One could, however, move all the initialization code into a factory function, and then return lineCountIfLarge as a DisposableLike & { (editor: TextEditor) => number } (i.e. a function with a dispose method). I can't say I'm particularly motivated to do the legwork required however, from my point of view this is used exactly once in this here code base, hence I don't see much benefit in generalizing it.

aminya commented 3 years ago

By the same logic we should abandon Atom and switch to VSCode, because both are similar products, and we should strive to make one-size-fits-all solution instead of fragmenting the ecosystem. I hope it's pretty obvious things rarely work out this way.

The amount of difference between two IDEs is night and day while the difference between two outline tabs is just a matter of the icon colors.

I am sad to do this, once I implemented all the features of semantics view in the outline, I will disable SemanticsView using configs to ensure the ecosystem is not hurt by the fragmentation. I care about the end users way more than the developers.

lierdakil commented 3 years ago

The amount of difference between two IDEs is night and day

Atom and VSCode are not IDEs. Both are hackable text editors. Both use the same platform, and IDE features come from plug-ins, which are reasonably flexible in both editors (more so in Atom, but that usually requires fiddling with poorly-documented internals). The primary difference is, IDE UI features and LSP compatibility have first-party support in VSCode, where in Atom it's our headache. For the end-user, the differences are in fact extremely minor, but those are enough to prefer one over the other (that said, familiarity plays a huge role as well). Of course, there are many internal differences, but most of those are completely invisible to the end-user (aside from the performance issues in Atom). So I literally have no idea what you mean here.

once I implemented all the features of semantics view in the outline

Besides the style (which is a non-trivial difference actually), I think the only differences in terms of functionality are private/protected/public indicators and focus following the cursor? I use neither semantic view nor outline, and I wrote neither of those, so I might be missing something. Nevertheless, the former is impossible with the current API, but nothing's stopping you from changing that. And the latter you've opted to remove, so I don't know whether you're considering adding it back in.

I will disable SemanticsView using configs

There aren't any actually. Well, except whether to show it by default. But actually, I don't mind that, provided you only disable the semantic view when the outline view is installed (and enabled). The situation where a user might want both at the same time is pretty hard to imagine. That said, https://github.com/TypeStrong/atom-typescript/issues/1566 aka https://github.com/atom-community/atom-ide-javascript/issues/14 is still an issue, so try to leave the users an out in case they want to have a weird configuration. Be warned that in case you'll decide to actively impede users from choosing to use semantic view for whatever reason, I'll be very annoyed -- "caring for the end users" by limiting their options is a farce.