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

Improve performance of placing placeholder #390

Merged
merged 10 commits into from May 13, 2018

Conversation

Projects
None yet
8 participants
@Ben3eeE
Member

Ben3eeE commented Oct 26, 2016

Since atom/tree-view#525 is using code from the tabs package for placing the placeholder stealing the ideas from @BinaryMuse in atom/tree-view#525 (comment) seems to also work for tabs when there are many tabs open.

Before:
drag tabs before

After:
drag tabs binarymuse

To note is that the placeholder doesn't get removed when leaving the tab bar because of the change in the onDragLeave event also stolen from the tree-view PR.
drag tabs binarymuse also

Which is strange when dragging tabs to other panes:
gdgd

Ben3eeE added some commits Oct 26, 2016

@damir

This comment has been minimized.

damir commented Mar 2, 2017

Can someone merge this? Dragging tabs is really slow and annoying.
Thank you.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Sep 24, 2017

@Ben3eeE I believe I fixed the placeholder-not-disappearing issue you were experiencing. The entry.target is entry.currentTarget line you added isn't present in the Tree View PR and that check would always return true anyways except when dragging outside of the tab bar, serving no real benefit.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Sep 25, 2017

I believe I fixed the placeholder-not-disappearing issue you were experiencing.

Amazing 😍

The entry.target is entry.currentTarget line you added isn't present in the Tree View PR and that check would always return true anyways except when dragging outside of the tab bar, serving no real benefit.

Huh, I'm sure I didn't add anything when opening this. I must have been really confused if I did 😀

Edit: It is part of the original PR? https://github.com/atom/tree-view/pull/525/files#diff-22e094e871c347ed2946550500ecec83R56

And exists for this case:
placeholder perf 1

On master doing ☝️ doesn't remove the placeholder.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Sep 25, 2017

Ah you're right, I was only looking at the one comment's patch by accident.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Sep 25, 2017

I did the same thing and was confused why I had added the extra case 😀

@50Wliu

This comment has been minimized.

Member

50Wliu commented Sep 25, 2017

☝️ fixes most issues but I just encountered another one that I'm working on fixing.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Sep 25, 2017

Looks like the resize handle is causing a dragleave event to be fired before the drag goes outside the tab bar since it's ~8px wide. What I would give for an event that fires when the element goes outside the area where the event handler is.

@Ben3eeE Ben3eeE referenced this pull request Oct 3, 2017

Open

"Packages" sub-menu lags tab movement #1004

1 of 1 task complete
@ha404

This comment has been minimized.

ha404 commented Nov 14, 2017

Any progress on this? It's been excruciating moving tabs around.

What I would give for an event that fires when the element goes outside the area where the event handler is.

Something like mouseleave? https://developer.mozilla.org/en-US/docs/Web/Events/mouseleave

@50Wliu

This comment has been minimized.

Member

50Wliu commented Nov 14, 2017

Ooh, that looks like it has potential. Thanks!

@ha404

This comment has been minimized.

ha404 commented Nov 28, 2017

@50Wliu I should've read more into your actual problem. It looks like you need to use event.stopPropagation so that your events don't bubble up and trigger other events. This could be somewhat related: https://stackoverflow.com/questions/15902920/resizing-an-element-triggers-the-resize-event-of-the-window

@@ -284,24 +285,33 @@ class TabBarView
false
onDragLeave: (event) ->

This comment has been minimized.

@ha404

ha404 Nov 28, 2017

You might want to do event.stopPropagation() here?

This comment has been minimized.

@50Wliu

50Wliu Dec 5, 2017

Member

dragLeave is uncancelable.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Dec 5, 2017

@ha404 so yes, mouseleave is exactly what I need. The problem, of course, is that it doesn't get fired when a drag-and-drop operation is in progress. I discovered that in #523.

@50Wliu

This comment has been minimized.

Member

50Wliu commented Dec 6, 2017

I have a hunch that the relatedTarget attribute will be able to solve this problem as it refers to the new (current) target. Unfortunately, relatedTarget was always null until Chrome 60 (bug report). Atom is currently using Chrome 56, and the most recent stable Electron version (1.7.x) is using Chrome 58. Electron 1.8 beta is using Chrome 59. The update to Chrome 60 was skipped in favor of Chrome 61. Therefore it might take quite a while for Atom to pick up the required Chrome version 😕.

@50Wliu 50Wliu added the blocked label Dec 6, 2017

@zinkkrysty

This comment has been minimized.

zinkkrysty commented Apr 16, 2018

Any news on this? The sluggish drag and drop of tabs is extremely annoying. (in my case, the delay is about 5-10 seconds so my editor gets frozen for that amount of time!)

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented Apr 16, 2018

@zinkkrysty No news. This is still blocked #390 (comment)

@hansonw

This comment has been minimized.

hansonw commented May 4, 2018

Bump - this seems like a pretty high-profile issue (and makes Atom look bad when it comes to performance). Is there an issue that doesn't require an Electron upgrade? It sounds like that would be several months out :(

@50Wliu 50Wliu removed the blocked label May 12, 2018

@50Wliu

This comment has been minimized.

Member

50Wliu commented May 12, 2018

Ok, I think this should work. Requires at least Atom 1.27.0-dev-c5dea46ac or higher.

@Ben3eeE

This comment has been minimized.

Member

Ben3eeE commented May 13, 2018

@50Wliu LGTM :shipit:

@50Wliu 50Wliu merged commit 61560e3 into master May 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the placeholder-performance branch May 13, 2018

@diaswrd

This comment has been minimized.

diaswrd commented Jun 21, 2018

Oh my god, this problem was triggering me in so many levels and for so long... @Ben3eeE THANK YOU!

@pkriete pkriete referenced this pull request Jun 22, 2018

Open

Tab reordering extremely slow in large projects #17561

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment