Skip to content
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

SemanticView optimizations #1575

Merged
merged 4 commits into from
Mar 27, 2021
Merged

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Feb 28, 2021

  • All compiled assets are included (atom packages are git tags and hence the built files need to be a part of the source control)

Description:

  • Replace continuous cursor following with a command
  • Large file optimization for updating outline

Fixes #1570

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

@aminya aminya marked this pull request as ready for review February 28, 2021 17:06
Copy link
Collaborator

@lierdakil lierdakil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First and foremost, thank you for the pull request.

I've looked it over, and I have some suggestions. I'll mark this as "request changes" for now.

lib/main/atom/views/outline/navigationTreeComponent.tsx Outdated Show resolved Hide resolved
lib/main/atom/views/outline/navigationTreeComponent.tsx Outdated Show resolved Hide resolved
lib/main/atom/utils/atom.ts Outdated Show resolved Hide resolved
const buffer = editor.getBuffer()
for (let i = 0, len = lineCount; i < len; i++) {
if (buffer.lineLengthForRow(i) > longLineLength) {
return longLineLength
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it returning the max length of line as a line count???

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aminya I really need to understand this snippet. Shouldn't it return this.largeFileLineCount or something similar? Line length is not equivalent to line count by any stretch of the imagination.

@aminya
Copy link
Contributor Author

aminya commented Mar 19, 2021

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
Copy link
Collaborator

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 a17290d 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
Copy link
Collaborator

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

@aminya
Copy link
Contributor Author

aminya commented Mar 20, 2021

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 lierdakil merged commit 3bcb499 into TypeStrong:master Mar 27, 2021
@lierdakil
Copy link
Collaborator

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
Copy link
Contributor Author

aminya commented Mar 27, 2021

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
Copy link
Collaborator

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, #1566 aka atom-community/atom-ide-javascript#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Semantics view large file optimizations
2 participants