Show match index in place of match count

[RaymondLim]

This is the implementation of https://trello.com/c/0GXqt5GF/1089-s-find-next-indicate-current-match-index.

[JeffryBooher]

Can't this just be var index = resultSet.indexOf(cursor) ?

[peterflynn]

@JeffryBooher I think it can't, because it's not the same object by === standards. _.findIndex() does a deep comparison instead.

[JeffryBooher]

Seems to work for normal find/replace from the find bar but:

• Does not work for Find Across Files which is clearly outside the scope of this change but we should decide if we want it and add a trello card for it.
• Does not work for "Skip and Add Next Match" or "Add Next Match to Selection" commands. We should probably update for those 2 commands as well.

[JeffryBooher]

done with initial review

[peterflynn]

Might want to add a note here that findNext()'s call to _showMatchIndex() is what upates this label in the other case -- and that any call to updateResultSet() is guaranteed to be followed by a call to findNext() (unless there's no query, in which case we've already cleared the find count label above).

[peterflynn]

@RaymondLim This might have performance implications though -- now it's O(n) every time you invoke Find Next. Might not be noticeable most of the time, but it's not ideal. Do you think it's worth trying my earlier suggestion? If you use the previous matchIndex as the starting point (and then search in the direction the caller passes in as a hint), then it will remain O(1) for most cases.

[RaymondLim]

I'm not sure I understand your suggestion of using previous matchIndex as the starting point. So you're not suggesting to use _.findIndex() which does not take another argument to use the prev match index. Also, as I mentioned in previous comment as an update, this change does uncover one issue that is in my prior implementation for replace scenario. So can you show me the code that you're suggesting? And does your suggestion work for replace scenario where the index does not change after replacement, but the total match count does reduce one.

[RaymondLim]

@peterflynn I think I found a solution that is O(1) for most of the subsequent updates of the match index.

[RaymondLim]

@JeffryBooher and @peterflynn Ready for re-review.

[JeffryBooher]

we should make this searchingBackwards to avoid rev being confused with something else like revision

[RaymondLim]

I was using the same variable from the caller function, but I agree that rev can be misleading.

[JeffryBooher]

@RaymondLim just the one nit on changing rev to searchingBackwards all tests pass and I believe it's good to go after that.

[RaymondLim]

Ready for re-review.

[JeffryBooher]

Looks good. Merging