Show index during search traversal. #3230

zimbabao · 2017-06-27T07:01:58Z

Associated Issue: #3204

Summary of Changes

Shows the index of search
This is bit naive version, easy optimization yet to be done.

Transfer of data from worker can be optimized by using transfer objects.

codecov · 2017-07-05T14:52:48Z

Codecov Report

Merging #3230 into master will increase coverage by 0.06%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #3230      +/-   ##
==========================================
+ Coverage   49.35%   49.41%   +0.06%     
==========================================
  Files          99       99              
  Lines        4156     4159       +3     
  Branches      856      858       +2     
==========================================
+ Hits         2051     2055       +4     
+ Misses       2105     2104       -1

Impacted Files	Coverage Δ
src/reducers/ui.js	`18.57% <ø> (ø)`	⬆️
src/utils/editor/source-search.js	`7.01% <0%> (+0.23%)`	⬆️
src/utils/search/get-matches.js	`100% <100%> (ø)`
src/utils/search/index.js	`100% <100%> (ø)`	⬆️
src/components/Editor/SearchBar.js	`20.71% <5.88%> (-0.38%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb7eaf5...724bd0a. Read the comment docs.

jasonLaster

some quick thoughts

jasonLaster · 2017-07-05T14:57:24Z

src/components/Editor/SearchBar.js

-      const newIndex = findFnc(ctx, query, true, modifiers.toJS());
+      const findPos = findFnc(ctx, query, true, modifiers.toJS());
+      const { ch: newIndex } = findPos;
+      const matchedLocationIndex = matchedLocations


It might be nice to move this function to a helper: getSearchIndex(currentPos, matchedLocations)

jasonLaster · 2017-07-05T14:58:01Z

src/components/Editor/SearchBar.js

-        count
+        matchedLocations,
+        matchedLocationIndex,
+        count: matchedLocations.length,


could matchedLocations be matches with a type definition that makes it clear what is stored?

jasonLaster · 2017-07-05T15:00:53Z

src/utils/search/get-matches.js

@@ -0,0 +1,21 @@
+import buildQuery from "./build-query";


we might want to add this function to the search worker so that it never slows down the main process:

diff --git a/src/utils/search/index.js b/src/utils/search/index.js index 2ec930c..fcb55bb 100644 --- a/src/utils/search/index.js +++ b/src/utils/search/index.js @@ -6,3 +6,4 @@ export const startSearchWorker = dispatcher.start.bind(dispatcher); export const stopSearchWorker = dispatcher.stop.bind(dispatcher); export const countMatches = dispatcher.task("countMatches"); +export const getMatches = dispatcher.task("getMatches"); diff --git a/src/utils/search/worker.js b/src/utils/search/worker.js index dbba6c1..75f7b2c 100644 --- a/src/utils/search/worker.js +++ b/src/utils/search/worker.js @@ -1,4 +1,6 @@ import buildQuery from "./utils/build-query"; +import getMatches from "./getMatches"; + import { workerUtils } from "devtools-utils"; const { workerHandler } = workerUtils; @@ -14,4 +16,4 @@ export function countMatches( return match ? match.length : 0; } -self.onmessage = workerHandler({ countMatches }); +self.onmessage = workerHandler({ countMatches, getMatches });

LGTM. I was planning to do some opts as next step.

Use transfer objects to transfer matches from worker to main

Make next and previous to be O(1) operation if cursor position has not changed from last search.

Thanks for taking care of the CL. I could not get to it quickly :(.

No problem! Maybe we can do it in a second pr?

codehag

A few comments! need to confirm a few things so this might change

codehag · 2017-07-05T09:52:55Z

src/components/Editor/SearchBar.js

@@ -490,10 +496,19 @@ class SearchBar extends Component {

    if (modifiers) {
      const findFnc = rev ? findPrev : findNext;
-      const newIndex = findFnc(ctx, query, true, modifiers.toJS());
+      const findPos = findFnc(ctx, query, true, modifiers.toJS());


this code (lines 499 - 511) is very similar to lines 436-447, perhaps we can consolidate them into a function that is called from both locations.

codehag · 2017-07-05T10:02:07Z

src/components/Editor/SearchBar.js

@@ -433,9 +433,15 @@ class SearchBar extends Component {
      clearIndex(ctx, query, modifiers.toJS());
    }

-    const newIndex = find(ctx, query, true, modifiers.toJS());
+    const findPos = find(ctx, query, true, modifiers.toJS());
+    const { ch: newIndex } = findPos;


nit (can be taken care of while consolidating the lines): the naming here is confusing. newIndex is more descriptive than ch, but we don't really need this as an alias, especially since we use findPos.ch on line 439. findPos might also be misleading, since it uses a verb and sounds like it should be a function. Maybe we should choose one way or the other. At the moment I am leaning towards not using newIndex as an alias, and just sticking to const { ch, line } = find(ctx, query, true, modifiers.toJS()) then we can sidestep the whole issue :)

codehag · 2017-07-05T10:45:36Z

src/utils/editor/source-search.js


+    matchIndex = searchLocation ? searchLocation.from : null;


maybe we can write doSearch like this? we will also be able to check and expect that there is always a { ch: number, line: number } flowtype being returned. Otherwise, we will end up with sometimes { ch, line } sometimes null, which will complicate things for functions relying on this one, and makes it difficult to reason about what is going on.

Also, changed some of the let declarations to const as they were not being reassigned, and made some changes to naming.

function doSearch(ctx, rev, query, keepSelection, modifiers: SearchModifiers) { const { cm } = ctx; const defaultIndex = { line: -1, ch: -1 }; let matchIndex = null; cm.operation(function () { if (!query || isWhitespace(query)) { return; } const state = getSearchState(cm, query, modifiers); const isNewQuery = state.query !== query; state.query = query; updateOverlay(cm, state, query, modifiers); updateCursor(cm, state, keepSelection); const searchLocation = searchNext(ctx, rev, query, isNewQuery, modifiers); matchIndex = searchLocation ? searchLocation.from : defaultIndex; state.matchIndex = matchIndex.ch; }); return matchIndex || defaultIndex; }

looks reasonable to me

codehag · 2017-07-05T11:09:10Z

src/reducers/ui.js

@@ -20,6 +20,8 @@ export type FileSearchModifiers = Record<{
 export type SymbolSearchType = "functions" | "variables";

 export type SearchResults = {
+  matchedLocations: Array<Object>,


👍 great! more flowtypes!

codehag · 2017-07-05T11:14:03Z

src/utils/editor/source-search.js

    // NOTE: We would like to find the correct match index based on where the
    // match is in the document.
-    state.matchIndex = matchIndex;
+    state.matchIndex = matchIndex ? matchIndex.ch : -1;


its strange that the matchIndex diverges here... maybe we should refactor to keep them in sync? Also, we might want to take a look at findNext, findPrev, to make sure that their functionality is not broken.

I noticed that state.matchIndex is not used anywhere in the codebase... we are using the return value of doSearch instead. Should I remove it?

codehag · 2017-07-05T11:40:02Z

src/components/Editor/SearchBar.js

@@ -423,7 +423,7 @@ class SearchBar extends Component {

    const ctx = { ed, cm: ed.codeMirror };

-    const newCount = await countMatches(
+    const matchedLocations = await countMatches(


the function name countMatches no longer reflects what it is doing. we want either a new function or rename this function to getMatches.

codehag · 2017-07-06T10:29:01Z

Rebased! added a couple of tweeks. Should be ready to go, @jasonLaster, @zimbabao can you take one more look?

jasonLaster · 2017-07-06T16:11:45Z

src/components/Editor/SearchBar.js

@@ -67,6 +61,8 @@ class SearchBar extends Component {
    selectedSource?: SourceRecord,
    highlightLineRange: ({ start: number, end: number }) => void,
    clearHighlightLineRange: () => void,
+    symbolSearchOn?: boolean,
+    symbolSearchResults: Array<any>,


I'm a bit confused by why we need symbolSearchResults and searchResults

did we remove symbolSearch? I can no longer trigger the buttons...

take a look at master. We removed this stuff when we added the search modal.

Must just need a quick cleanup

jasonLaster · 2017-07-06T16:12:17Z

src/components/Editor/Tabs.js

    togglePrettyPrint: string => void,
    togglePaneCollapse: () => void,
+    toggleActiveSearch: (?string) => void,


why do we need toggleProjectSearch and toggleActiveSearch?

jasonLaster · 2017-07-06T16:12:45Z

src/utils/editor/source-search.js

-    let state = getSearchState(cm, query, modifiers);
-    const newQuery = state.query != query;
+    const state = getSearchState(cm, query, modifiers);
+    const isNewQuery = state.query !== query;


Fixes: firefox-devtools#3204

jasonLaster · 2017-07-07T13:45:38Z

src/components/Editor/SearchBar.js

@@ -67,6 +61,8 @@ class SearchBar extends Component {
    selectedSource?: SourceRecord,
    highlightLineRange: ({ start: number, end: number }) => void,
    clearHighlightLineRange: () => void,
+    symbolSearchOn?: boolean,
+    symbolSearchResults: Array<any>,


take a look at master. We removed this stuff when we added the search modal.

Must just need a quick cleanup

jasonLaster · 2017-07-07T18:44:16Z

Thanks @zimbabao and @codehag!

codehag force-pushed the show-index-in-search branch from d7ed24a to e5e06d1 Compare July 5, 2017 14:52

jasonLaster reviewed Jul 5, 2017

View reviewed changes

codehag reviewed Jul 5, 2017

View reviewed changes

codehag force-pushed the show-index-in-search branch from ab0c945 to ddb9eec Compare July 6, 2017 10:28

jasonLaster reviewed Jul 6, 2017

View reviewed changes

zimbabao and others added 4 commits July 7, 2017 13:30

Show index during search traversal.

c7e6e72

Fixes: firefox-devtools#3204

make linter happy

b59baa5

a bit of cleanup

80e3056

partially address comments

213b067

codehag force-pushed the show-index-in-search branch from ddb9eec to 213b067 Compare July 7, 2017 12:16

remove hangover from symbolSearch

724bd0a

jasonLaster approved these changes Jul 7, 2017

View reviewed changes

jasonLaster merged commit e08209d into firefox-devtools:master Jul 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show index during search traversal. #3230

Show index during search traversal. #3230

zimbabao commented Jun 27, 2017 •

edited by jasonLaster

codecov bot commented Jul 5, 2017 •

edited

jasonLaster left a comment

jasonLaster Jul 5, 2017

jasonLaster Jul 5, 2017

jasonLaster Jul 5, 2017

codehag Jul 5, 2017

zimbabao Jul 6, 2017

codehag Jul 7, 2017

codehag left a comment

codehag Jul 5, 2017

codehag Jul 5, 2017

codehag Jul 5, 2017

jasonLaster Jul 5, 2017

codehag Jul 5, 2017

codehag Jul 5, 2017

codehag Jul 5, 2017

jasonLaster Jul 5, 2017

codehag Jul 5, 2017

codehag commented Jul 6, 2017

jasonLaster Jul 6, 2017

codehag Jul 7, 2017

jasonLaster Jul 7, 2017

jasonLaster Jul 6, 2017

jasonLaster Jul 6, 2017

jasonLaster Jul 7, 2017

jasonLaster commented Jul 7, 2017

Show index during search traversal. #3230

Show index during search traversal. #3230

Conversation

zimbabao commented Jun 27, 2017 • edited by jasonLaster

Summary of Changes

codecov bot commented Jul 5, 2017 • edited

Codecov Report

jasonLaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codehag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codehag commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonLaster commented Jul 7, 2017

zimbabao commented Jun 27, 2017 •

edited by jasonLaster

codecov bot commented Jul 5, 2017 •

edited