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

[Performance] improve atom.tooltips.add #307

Open
aminya opened this issue Jun 10, 2020 · 1 comment · May be fixed by #308
Open

[Performance] improve atom.tooltips.add #307

aminya opened this issue Jun 10, 2020 · 1 comment · May be fixed by #308

Comments

@aminya
Copy link
Member

aminya commented Jun 10, 2020

You would be surprised if you know that most of the time spent for creating the buttons, is spent in adding the tooltips to them! We might want to write a faster version of the original
https://github.com/suda/tool-bar/blob/937f75719a1002af5b207c073b6e45dd82617f44/lib/items/tool-bar-button-view.js#L121

https://github.com/atom/atom/blob/e4b9c1e0815c5c07514a0649e7ebb9b5226a2931/src/tooltip-manager.js#L113-L181

For 30 buttons, this becomes ~6ms.

Details

image

Which is almost the same as the whole time spent on the buttons:

Details

image

I am going to port the function to our package and see if I can write a faster version.

@aminya
Copy link
Member Author

aminya commented Jun 10, 2020

Further investigation shows that the time is spent here, to add keyBindingCommand, which is because we set keyBindingCommand even when the user gives it null
https://github.com/atom/atom/blob/e4b9c1e0815c5c07514a0649e7ebb9b5226a2931/src/tooltip-manager.js#L126-L139

  const { keyBindingCommand, keyBindingTarget } = options;

  if (keyBindingCommand != null) {
    const bindings = atom.tooltips.keymapManager.findKeyBindings({
      command: keyBindingCommand,
      target: keyBindingTarget
    });
    const keystroke = getKeystroke(bindings);
    if (options.title != null && keystroke != null) {
      options.title += ` ${getKeystroke(bindings)}`;
    } else if (keystroke != null) {
      options.title = getKeystroke(bindings);
    }
  }

We don't need to use keyBindingCommand, because we already add a callback through callback option, and tooltip callback is redundant.

@aminya aminya linked a pull request Jun 10, 2020 that will close this issue
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 a pull request may close this issue.

1 participant