Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

QuickOpen: Don't trigger item focus on hover #5872

Merged
merged 3 commits into from
Nov 7, 2013
Merged

Conversation

njx
Copy link

@njx njx commented Nov 6, 2013

For #4791.

NOTE: if you pull this branch to review it, your life will be happier if you do git submodule deinit src/thirdparty/smart-auto-complete before checking it out--this will clean up your local state. If you then need to check out master again, just do git submodule update --init again afterwards to get it back. You need git 1.8.4 or later to use deinit.

Smart-autocomplete didn't distinguish between arrow-key selection and mouse hover, so if you happened to bump the mouse over the list while using Quick Open and then hit Enter, it would select the item the mouse was over instead of the first item or the item you selected with arrow keys.

We decided to go ahead and just monkeypatch smart-autocomplete since we will likely replace it in the future anyway. So, the first commit below (7338907) just pulls in the latest source from the upstream repo into smart-auto-complete-local and removes the old submodule. It should be exactly the same except that I added a note at the beginning of the README indicating where it came from.

The second commit (faf21e0) reverts a patch in the upstream repo that breaks Quick Open in Brackets. Its original intention was to make it so holding down the arrow keys would properly repeat. We should eventually look at a fix for that, but it would likely require changes to the (fragile) key handling in Quick Open and smart autocomplete.

The third commit (e6f50d6) is the actual fix for this bug. It simply comments out the JS mouseenter/mouseleave handlers (so itemFocus/itemUnfocus no longer gets sent out for mouseovers), and just adds a simple highlight in CSS. (The other changes in the LESS file are just a small refactoring to move some separate rules into nested &-rules.)

Narciso Jaramillo added 3 commits November 6, 2013 10:54
…m upstream repo (as local files). Point Brackets at new version. NOTE that this commit breaks Quick Open, but I wanted to keep this commit at the exact version from the original repo--the next commit will fix Quick Open.
@njx
Copy link
Author

njx commented Nov 6, 2013

/cc @SAplayer @WebsiteDeveloper @larz0 - see note at beginning of pull request about dealing with the submodule change when you check out this branch.

@redmunds
Copy link
Contributor

redmunds commented Nov 7, 2013

@njx This looks good. Thanks for splitting the changes into atomic commits!

I have a question before I merge: is there anything that can be done by me when merging this pull request to try to avoid the git submodule resync hell that we had the last time a submodule was removed?

@njx
Copy link
Author

njx commented Nov 7, 2013

I don't think there's any automatic thing we can do, except that we should let people know that, if possible, they should do git submodule deinit src/thirdparty/smart-auto-complete before pulling the latest master with this change; otherwise they'll get the error about not being able to remove the old folder, I believe. However, even if they do that after pulling master, I think everything will still be okay--they might just have to then manually remove the old folder.

@njx
Copy link
Author

njx commented Nov 7, 2013

Hmmm, no, it looks like you can't do the deinit after pulling the changes. That's still probably fine--people just have to manually remove the files, and the only issue is that there will still be some stale stuff left in their .git/modules folder. But that shouldn't affect anything, I think...

@njx
Copy link
Author

njx commented Nov 7, 2013

(Note that we won't have the same need to do sync because we're not changing the URL on an existing submodule--we're just removing it. And the new files are in a different folder, so nothing should get too confused.)

@redmunds
Copy link
Contributor

redmunds commented Nov 7, 2013

I guess there's nothing that can be done. The worst problem is when you jump between a bunch of branches that are before/after the change. Oh, well. Merging.

redmunds added a commit that referenced this pull request Nov 7, 2013
QuickOpen: Don't trigger item focus on hover
@redmunds redmunds merged commit c771265 into master Nov 7, 2013
@redmunds redmunds deleted the nj/issue-4791 branch November 7, 2013 22:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants