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

Remove activation hook #300

Merged
merged 2 commits into from
Jul 28, 2020
Merged

Remove activation hook #300

merged 2 commits into from
Jul 28, 2020

Conversation

aminya
Copy link
Member

@aminya aminya commented May 28, 2020

Tool-bar late loading does not look good. It makes the Atom look bulky. I think 20ms is not that long to defer it.

@ericcornelissen
Copy link
Contributor

ericcornelissen commented May 28, 2020

Do you mean that the toolbar is being loaded in quite late and it suddenly just appears? I'm not entirely against removing it, but I'm personally not in favor of it. 20ms is not much but 1) that might be more on a slower computer and 2) if every package maintainer thinks that it adds up quickly.

Some alternatives we could consider:

  • Remove the activation hook but update our implementation to get that 20ms down even further
  • Add an animation for when the toolbar get's loaded, I suspect that will make it look less "bulky"

Also, if we do decide to merge this Pull Request I would like to see all changes from #283 reverted 🙂

@aminya
Copy link
Member Author

aminya commented May 28, 2020

Yes. It appears suddenly. I don't know how to add an animation. If you know, that can help.

For performance, I can improve the loading time. That is on my todo list. 👍

@aminya
Copy link
Member Author

aminya commented May 29, 2020

@suda Could you merge this? It makes Atom look much better.

@suda
Copy link
Collaborator

suda commented Jun 3, 2020

Hi Amin, the activation hook was added on purpose to improve Atom's performance.
I agree with adding the animation on showing the toolbar, and a CSS transition should be enough to create "slide-in" animation.

@aminya
Copy link
Member Author

aminya commented Jun 3, 2020

Hi Wojtek! In #304 and #302, I improved the actual loading time of the package. The toolbar gets almost twice fast (90ms to 45ms). I think this activation hook is not necessarily useful anymore.

Note that I don't know how to add this CSS animation.

@ericcornelissen
Copy link
Contributor

Hi Wojtek! In #304 and #302, I improved the actual loading time of the package. The toolbar gets almost twice fast (90ms to 45ms). I think this activation hook is not necessarily useful anymore.

In that case it would definitely make more sense to merge those PRs before merging this one 😉 If for whatever reason those PRs don't get merged we don't want this one having been merged already.

Note that I don't know how to add this CSS animation.

That's fine, someone else can make that contribution if we decide not to merge this PR.

@suda
Copy link
Collaborator

suda commented Jun 3, 2020

I agree we can add the animations later. And other PRs by Amin give a great performance boost anyway!

The toolbar gets almost twice fast (90ms to 45ms)

Yes because it's trying to be loaded in parallel with Atom's core making it a bit slower instead. The goal of the hook was to give Atom all resources to bootstrap and then let it decide what to initialize in what order. We shouldn't try to squeeze ourselves into this process.

@aminya
Copy link
Member Author

aminya commented Jul 8, 2020

@suda Can we merge this? Trying the trick in #306 was not successful for me.

@suda
Copy link
Collaborator

suda commented Jul 10, 2020

As I said, there was a reason for adding the activation hook and I wouldn't like to remove it. We should come up with a nicer implementation of #306 instead of going against Atom's recommendation of using the hook IMO.

@aminya
Copy link
Member Author

aminya commented Jul 10, 2020

@suda

Activation hook is very useful for packages that don't need to load a feature right away. For example, they just work in the text editor, etc. But toolbar should appear when atom loads, and if does not, it makes atom look weird (compared to other IDEs)

I mentioned in the other repository too:

I think here we are doing things that are not common in other software/IDEs. All the usual software, load their toolbar normally during the loading time. We don't need to circumvent doing this!

Tool-bar is, in my opinion, a crucial part of Atom, and it is so useful that I want to include in atom-community builds: https://github.com/atom-ide-community/atom/projects/4#card-41191288

@suda
Copy link
Collaborator

suda commented Jul 28, 2020

You have a fair point. Lets merge this in then 👍

@suda suda merged commit a2610c2 into atom-community:master Jul 28, 2020
@aminya
Copy link
Member Author

aminya commented Jul 28, 2020

Thanks! 🎉

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.

None yet

3 participants