Issue #11261: keep the search bar open #11996

pldilley · 2015-12-12T08:40:10Z

Reference:
Issue: #11261 - "keep the search bar open"

Based on this pull request that is waiting on the user: #11692 ( @adrianhartanto0 ). However I ended up taking a different approach; upon testing I found that the original approach had hiccups. The changes are influenced by the preference idea by @zaggino and @abose in the aforementioned pull request.

Changes:

Added in a menu option under View to allow find bar to stay open/automatically close
Add a close icon to the search bar (for completeness)
Fix escape key for in-editor searches
Remember last search phrase if no selection for in-editor searches
~~Go to the next search item if the bar is open and the user clicks Find or hits ctrl+F~~

Testing:

All tests pass for FindReplace-test.js after some changes to the code I wrote and one of the tests
Manual testing was briefly performed as such:
- In-Editor - Find the same item by resetting the cursor position: Pass
- In-Editor - Find a custom item by setting the cursor position: Pass
- In-Editor - Find the first item by setting the cursor position after the last: Pass
- In-Editor - Cycle through the items by hitting ctrl+F while still open: Pass
- In-Editor - Closes when the "Automatically close search" option is checked and the editor is focused: Pass
- In-Editor - Moving to a different editor closes the search: Pass
- In-Editor - Pre-selecting text and re-opening the search shows the newly selected text: Pass
- In-Editor - Pre-selecting nothing and re-opening the search shows the last query: Pass
- In-Editor - Selecting the caps and regexp options functions as expected: Pass
- View --> Automatically close search - Preference is remembered on application restart: Pass
- In-File: Search functions as expected, and the same tests for In-Editor, where applicable, also pass.

…as a result of clicking in the main editor. This is done by commenting out the code in line 256. The search bar will also close when the ESC key is pressed while the main editor has focus

…/automatically close * Add a close icon to the search bar (for completeness) * Fix escape key for in-editor searches * Remember last search phrase if no selection for in-editor searches

…#11261

… until I can confirm with a Quebecois at work

Denisov21 · 2015-12-12T20:24:48Z

👍 Awersome PR!

petetnt · 2015-12-13T13:45:45Z

src/search/FindReplace.js

        if (editor) {
+            // Make note of the original query details
+            var state = getSearchState(editor._codeMirror),
+                qry = (findBar && state && state.parsedQuery) || "",


Minor nitpick: this variable could be just query

pldilley · 2015-12-13T17:41:41Z

Thanks @Denisov21 and @petetnt ! Will look into getting these fixed in the next few weeks :-)

pldilley · 2015-12-14T03:51:48Z

src/search/FindReplace.js

@@ -695,6 +702,8 @@ define(function (require, exports, module) {
                findBar.close();
            }
        }
+
+        editor.lastEditorSearchPosStr = "";     // Positions have shifted, so we abort the auto-next on findbar launch


No longer relevant - remove.

…#11261

abose · 2016-06-29T15:46:53Z

addresses #12280

petetnt · 2016-12-08T11:02:05Z

Welp, how has this been left on the backburner for so long. Sorry about that @pldilley! If you could fix that one conflict so we can merge this in ASAP.

Sorry again!

pldilley · 2016-12-08T14:46:03Z

@petetnt No problem, I have a bit of time this weekend, I'll retest it too to make sure that the behaviour hasn't changed.

swmitra · 2017-02-15T06:49:54Z

Bump. Is anyone working on this PR now?

IanButterworth · 2017-11-18T01:58:31Z

I'd appreciate this being incorporated if anyone has the time to iron out the conflicts

IanButterworth · 2018-01-18T19:02:15Z

Bumping again, as I get frustrated by this issue almost every time I use brackets. Please, can this be incorporated.

petetnt · 2018-01-18T20:15:20Z

@ianshmean sorry about that, this one has slipped through our fingers somehow once again. I'll try to re-review this tomorrow.

Thanks for your patience @pldilley too!

pldilley · 2018-01-18T20:20:07Z

Hmm, so I've resolved the merge conflicts based on master. However I haven't tested it, and am perhaps a bit too busy in life now to have time. Can someone please just do a sanity check on it before merging?

swmitra · 2018-01-19T06:30:45Z

@petetnt Once you are done with the review, I will merge this PR 👍 I was trying to merge this an year back BTW, well almost! But this thread was not active, couldn't incorporate this.

petetnt

LGTM codewise.

The new tests are failing now for some reason, not exactly sure why yet.

There's also a tiny bug where if you have the search bar open and toggle "Automatically close search" option the search bar only stays open after closing it once. However I don't think that's a blocker by any means.

👍

petetnt · 2018-01-21T15:46:57Z

test/spec/FindReplace-test.js

                    expect(getSearchField().val()).toEqual("Foo");
                    expectHighlightedMatches(capitalFooSelections);
                    expectSelection(capitalFooSelections[0]);
                    expectMatchIndex(0, 3);
                    expect(myEditor.centerOnCursor.calls.length).toEqual(3);

+                    expect(myEditor).toHaveCursorPosition(8, 8, true);


This tests fails for me with expected the cursor to be at (8, 8) but it actually was at (2, 8) and more than one character was selected, hmm.

petetnt · 2018-01-21T15:47:36Z

test/spec/FindReplace-test.js

                    twCommandManager.execute(Commands.CMD_FIND_NEXT);
+
+                    expect(myEditor).toHaveCursorPosition(8, 8, true);


Similar error with this one: expected the cursor to be at (8, 8) but it actually was at (2, 31) and more than one character was selected, hmm.

IanButterworth · 2018-02-28T21:17:29Z

Checking in on this. Seems to still be missing in 1.12.
Still a big fan of this feature being worked in

nethip · 2018-03-01T05:09:27Z

@ianshmean Totally agree with you. I hope to see this PR merged as well. @pldilley Can you please have a look at the review comments posted by @petetnt and incorporate the changes, if needed?.

petetnt

LGTM, see #14141 for additional commit

petetnt · 2018-03-02T14:57:00Z

Landed in #14141 with additional commit, thanks for your patience @pldilley, @ianshmean and @adrianhartanto0

pldilley · 2018-03-02T22:08:54Z

P.S. Sorry I did not get to the test fixes. I hope this code change works out :)

nethip · 2018-03-05T05:15:17Z

Thanks @petetnt and @pldilley for your efforts.

thejackhammer and others added 14 commits September 12, 2015 16:23

It fixes issue adobe#11261 by preventing the search bar from closing …

8c668bc

…as a result of clicking in the main editor. This is done by commenting out the code in line 256. The search bar will also close when the ESC key is pressed while the main editor has focus

Commented out unnecessary code around line 256

a120c12

[issue#11261] * Added in a menu option to allow find bar to stay open…

53f0427

…/automatically close * Add a close icon to the search bar (for completeness) * Fix escape key for in-editor searches * Remember last search phrase if no selection for in-editor searches

[issue#11261] Code clean up of initial mistakes of the previous commit

c2cdc65

[issue#11261] Strange issue with spacing in src/nls/fr/strings.js...

ea69c5a

[issue#11261] Strange issue with spacing in src/nls/fr/strings.js...

a57debb

Merge remote-tracking branch 'upstream/master' into issue#11261

e44b07e

[issue#11261] Further merge conflict resolution and clean up

20ad291

[issue#11261] Fix to branch and to testing

4637646

Merge branch 'master' of https://github.com/adobe/brackets into issue…

283ceb8

…#11261

[issue#11261] Fixes for incorrect regexp processing

923240b

[issue#11261] Why does JSLint keep changing its own file

9651160

[issue#11261] Not sure about the french one, let's exclude it for now…

2b57f0b

… until I can confirm with a Quebecois at work

[issue#11261] Test left test file with change...

949bd10

pldilley changed the title ~~Issue #11261 (via #11692)~~ Issue #11261 Dec 12, 2015

petetnt reviewed Dec 13, 2015
View reviewed changes

pldilley reviewed Dec 14, 2015
View reviewed changes

Paul Dilley and others added 3 commits January 2, 2016 08:41

Merge branch 'master' of https://github.com/adobe/brackets into issue…

ab1a454

…#11261

[issue adobe#11261] Minor fixes as per conversation of pull request

8981389

Merge branch 'master' of https://github.com/adobe/brackets into issue…

1fa7c2e

…#11261

ficristo changed the title ~~Issue #11261~~ Issue #11261: keep the search bar open Aug 14, 2016

zaggino added this to the Release 1.9 milestone Aug 28, 2016

petetnt self-assigned this Dec 8, 2016

petetnt mentioned this pull request Feb 6, 2017

A more persistent find bar would be helpful, persisting through code clicks #13083

Open

ficristo mentioned this pull request Feb 13, 2017

Issue #11261: keep the search bar open #11692

Closed

swmitra modified the milestones: Release 1.10, Release 1.9 Mar 15, 2017

pldilley added 2 commits January 18, 2018 15:01

Merge branch 'master' into issue#11261

dc8890f

Merged master incorrectly - fix bad merge

4df9aab

petetnt suggested changes Jan 21, 2018

View reviewed changes

petetnt mentioned this pull request Mar 2, 2018

Issue #11261: keep the search bar open #14141

Merged

petetnt approved these changes Mar 2, 2018

View reviewed changes

petetnt closed this Mar 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #11261: keep the search bar open #11996

Issue #11261: keep the search bar open #11996

pldilley commented Dec 12, 2015

Denisov21 commented Dec 12, 2015

petetnt Dec 13, 2015

pldilley commented Dec 13, 2015

pldilley Dec 14, 2015

abose commented Jun 29, 2016

petetnt commented Dec 8, 2016

pldilley commented Dec 8, 2016

swmitra commented Feb 15, 2017

IanButterworth commented Nov 18, 2017

IanButterworth commented Jan 18, 2018

petetnt commented Jan 18, 2018 •

edited

Loading

pldilley commented Jan 18, 2018

swmitra commented Jan 19, 2018 •

edited

Loading

petetnt left a comment

petetnt Jan 21, 2018

petetnt Jan 21, 2018

IanButterworth commented Feb 28, 2018

nethip commented Mar 1, 2018

petetnt left a comment

petetnt commented Mar 2, 2018

pldilley commented Mar 2, 2018

nethip commented Mar 5, 2018

		twCommandManager.execute(Commands.CMD_FIND_NEXT);

		expect(myEditor).toHaveCursorPosition(8, 8, true);

Issue #11261: keep the search bar open #11996

Issue #11261: keep the search bar open #11996

Conversation

pldilley commented Dec 12, 2015

Denisov21 commented Dec 12, 2015

petetnt Dec 13, 2015

Choose a reason for hiding this comment

pldilley commented Dec 13, 2015

pldilley Dec 14, 2015

Choose a reason for hiding this comment

abose commented Jun 29, 2016

petetnt commented Dec 8, 2016

pldilley commented Dec 8, 2016

swmitra commented Feb 15, 2017

IanButterworth commented Nov 18, 2017

IanButterworth commented Jan 18, 2018

petetnt commented Jan 18, 2018 • edited Loading

pldilley commented Jan 18, 2018

swmitra commented Jan 19, 2018 • edited Loading

petetnt left a comment

Choose a reason for hiding this comment

petetnt Jan 21, 2018

Choose a reason for hiding this comment

petetnt Jan 21, 2018

Choose a reason for hiding this comment

IanButterworth commented Feb 28, 2018

nethip commented Mar 1, 2018

petetnt left a comment

Choose a reason for hiding this comment

petetnt commented Mar 2, 2018

pldilley commented Mar 2, 2018

nethip commented Mar 5, 2018

petetnt commented Jan 18, 2018 •

edited

Loading

swmitra commented Jan 19, 2018 •

edited

Loading