Replace 'smart autocomplete' with a purpose-built QuickSearchField module #7227
Conversation
…dule. Simplifies QuickOpen code, removing workarounds and allowing it to use the ModalBar autoClose option. Fixes bugs with arrow key handling, cleans up APIs, and more consistently highlights the item that pressing Enter will select.
|
Oh thank goodness. |
| * | ||
| * @param {!function(*, string):string} options.formatter | ||
| * Converts one result object to a string of HTML text. Passed the item and the current query. | ||
| * @param {!function(*, string):void} options.onCommit |
There was a problem hiding this comment.
Is there a reason to do these as options instead of regular jQuery events?
There was a problem hiding this comment.
@njx Partly it's following how the existing smart-autocomplete API did things. But also, the resultProvider and formatter don't really support a one-to-many mapping because they require a single, specific return value. So it seemed simpler to keep the other two (onCommit and onHighlight) using a similar interface rather than having half use events and half not...
…search-field Conflicts: src/search/QuickOpen.js src/styles/brackets.less src/widgets/ModalBar.js
…ewline in editor (new, cmv4-only issue) - Fix bug: clicking Ctrl+T item without typing any text didn't work - Fix bug: pressing Enter to finish Goto Line didn't set cursor pos - Clean up docs, remove some TODOs - Remove unused keyup listener code (jQuery was silently no-oping when asked to attach a null event handler) - Simplify "@" detection in the three core Ctrl+T providers
…rchField. Remove logging code and old TODOs. Round out documentation. Fix bug where pressing enter in Goto Line mode did nothing if a previous search mode had been used earlier (currentPlugin was stale).
peterflynn
changed the title
| return true; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Select the selected item in the current document | ||
| * Scroll top the selected item in the current document (unless no query string entered yet, |
There was a problem hiding this comment.
the comment doesn't match the current signature anymore.
|
I'm adding medium priority to this since I think we want it to keep moving forward (and not get too stale). |
|
@njx Enhancing visibility by putting on the Nomination label. |
|
@peterflynn Is this planned to be land in 1.3? I can't seem to find a list of major changes/improvements that's planned for 1.3, and I would think this would be a good PR to land. |
…search-field Reimplements the fixes from PR #9624 & #9153 in a simpler way: instead of trying to selectively suppress ModalBar's scroll pos adjustments, in cases where we need to revert scroll pos in one file, just wait until ModalBar is done adjusting scroll pos and *then* reset the one we care about. Made possible by the fact that we now listen for ModalBar's "close" event and the event passes info on the reason for closing. Removes delay before results list opens: in CEF 2171 it seems both unneeded (animation is smooth without it) and it triggers an apparent Chrome bug where the post-animation state is displayed during the delay period even though the computed style reflects the pre-animation state. Conflicts: src/search/QuickOpen.js src/styles/brackets.less src/styles/brackets_colors.less src/widgets/ModalBar.js
- Fix bug where arrow keys with bare "@" didn't scroll to item in editor (with unit test) - Fix code style nits & update docs. Reponding to code review only one year late :-) - Fix JSHint nits about unused vars - CSS tweak when last item in list highlighted
|
Whew, well... only about a year late, I finally have this caught up with master and in good shape to merge :-) I've addressed all of @ingorichter's code review comments and fixed one lingering problem with up/down arrow keys. I ran off this branch all day with no problems, so it seems solid. Until recently, the user-facing benefits of this PR were pretty subtle (it was mostly about maintainability). But it appears to fix #9283, which is a pretty high-visibility CEF 2171 regression, so that makes it more valuable to get into 1.3. @nethip do you have time to review in the next week or so any any chance? (I'm assuming Ingo doesn't have the cycles) |
|
Hey @peterflynn Sorry I somehow missed this. I am already going through the long list of files in this PR :) . The changes seem to be fine. I will do a quick round of unit testing and merge this. Thanks! |
|
Thanks @nethip! |
Replace 'smart autocomplete' with a purpose-built QuickSearchField module
|
Thanks @peterflynn! |
|
@peterflynn The QuickOpen Integration Tests are failing on this merge (used git bisect). Could you please give me some pointers so that I could fix this Test failure. |
|
@rroshan1 This looks like a time out issue. Have you tried increasing the time limit to say 10 secs and try running the tests? |
|
@nethip Yes, I have tried increasing the time limit but that does not seem to fix it. |
|
QuickOpen Test failures resolved with PR #10922. |
Simplifies QuickOpen code -- trimming out about 200 lines -- removing workarounds and allowing it to use the ModalBar autoClose option. Fixes bugs with arrow key handling, cleans up APIs, and more consistently highlights the item that pressing enter will select.
Fixed issues include: #5871, #3405, #5360, #2757, #5029, #6646, #3444, #1550, #333, #7367, #8063, #9283, an unfiled bug where pressing the right arrow key closes Quick Open instead of moving the cursor, and #652 (which may reduce the priority of #6203 since it's the only user-visible part of that issue). I'm guessing this also fixes #5344.
There are still some loose ends to polish up here (unit tests & removing logging code) -- so it's definitely not ready to merge yet.Unit tests added, code is ready for review now!