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

Fix 2663: MultiRangeInlineEditor case #3819

Closed
wants to merge 2 commits into from
Closed

Fix 2663: MultiRangeInlineEditor case #3819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
058e12b
Fix for #2663. Works by putting contiguous matches ahead of special s…
dangoor May 14, 2013
d98e2ec
Merge branch 'master' into dangoor/2663-odd-quickopen
dangoor May 16, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 61 additions & 12 deletions src/utils/StringMatch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,26 +324,75 @@ define(function (require, exports, module) {
return false;
}

var contiguousCount = 0;

// This function peeks back through the results to see how many contiguous
// characters the user has typed. It's possbile that those characters will
// cross the SpecialMatch/NormalMatch boundaries. In other words, it's
// possible that we jumped ahead to the next special character to find a match
// when in reality that was the next character.
function _computeContiguousCount() {
var count = 1;
if (result.length === 1) {
if (strCounter - 1 !== result[0].index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also need to check for this in the case where result.length > 1? It seems like the code below that handles that case will return a contiguous count even if the contiguous matches are all further back in the string from where we're currently matching.

I guess another way to put it is, why isn't this whole function just something like:

var count = 0;
var lastMatch = strCounter - 1; // because strCounter has been ++'ed before we call this function
var i;
for (i = result.length - 1; i >= 0; i--) {
    if (result[i].index !== lastMatch) {
        break;
    }
    count++;
    lastMatch--;
}

count = 0;
}
return count;
}

var i = result.length - 1;
var lastMatch = result[i].index;
for (--i; i >= 0; i--) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This expression looks like it should be a complex emoticon of some sort. :)

if (result[i].index !== lastMatch - 1) {
break;
}
count++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like lastMatch should be updated somewhere in here (e.g. lastMatch = result[i].index, which should be equivalent to lastMatch-- if I'm understanding the code correctly).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If there is indeed a bug here, that probably means it's worth adding a test case with more than 2 consecutive ambiguous matches.)

}
return count;
}

while (true) {

// keep looping until we've either exhausted the query or the string
while (queryCounter < query.length && strCounter < str.length && strCounter <= deadBranches[queryCounter]) {
if (state === SPECIALS_MATCH) {
if (!findMatchingSpecial()) {
state = ANY_MATCH;
}
}

if (state === ANY_MATCH) {
// we look character by character for matches
// If the user has already typed two characters in a row that match,
// try to keep it going.
if (contiguousCount > 1) {
if (query[queryCounter] === str[strCounter]) {
// got a match! record it, and switch back to searching specials
contiguousCount++;
queryCounter++;
result.push(new NormalMatch(strCounter++));
state = SPECIALS_MATCH;
if (specials.indexOf(strCounter) > -1) {
result.push(new SpecialMatch(strCounter++));
} else {
result.push(new NormalMatch(strCounter++));
}
} else {
// no match, keep looking
strCounter++;
contiguousCount = 0;
}
}

if (contiguousCount < 2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a little clearer as contiguousCount <= 1, to make it clear that it's basically the opposite of the previous if (i.e., it would have been an else except for the fact that the previous if might have set contiguousCount to 0).

if (state === SPECIALS_MATCH) {
if (!findMatchingSpecial()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that there's an else, it might be clearer to reverse the sense of this if.

state = ANY_MATCH;
} else {
contiguousCount = _computeContiguousCount();
}
}

if (state === ANY_MATCH) {
// we look character by character for matches
if (query[queryCounter] === str[strCounter]) {
// got a match! record it, and switch back to searching specials
queryCounter++;
result.push(new NormalMatch(strCounter++));
state = SPECIALS_MATCH;
contiguousCount = _computeContiguousCount();
} else {
// no match, keep looking
strCounter++;
}
}
}
}
Expand Down
28 changes: 20 additions & 8 deletions test/spec/StringMatch-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,10 @@ define(function (require, exports, module) {
expect(result).toEqual([new SpecialMatch(13), new NormalMatch(14)]);

result = generateMatchList("doc", path, specialsInfo.specials, specialsInfo.lastSegmentSpecialsIndex);
expect(result).toEqual([new SpecialMatch(13), new NormalMatch(14), new SpecialMatch(21)]);
expect(result).toEqual([new SpecialMatch(13), new NormalMatch(14), new NormalMatch(15)]);

result = generateMatchList("doch", path, specialsInfo.specials, specialsInfo.lastSegmentSpecialsIndex);
expect(result).toEqual([new SpecialMatch(13), new NormalMatch(14), new SpecialMatch(21), new SpecialMatch(28)]);
expect(result).toEqual([new SpecialMatch(13), new NormalMatch(14), new NormalMatch(15), new SpecialMatch(28)]);
});

it("should handle contiguous matches that stand alone", function () {
Expand Down Expand Up @@ -127,7 +127,7 @@ define(function (require, exports, module) {
expect(result).toEqual([new SpecialMatch(0), new NormalMatch(11)]);

result = generateMatchList("mamo", btpath, btspecials.specials, 0);
expect(result).toEqual([new SpecialMatch(0), new NormalMatch(1), new SpecialMatch(4), new NormalMatch(9)]);
expect(result).toEqual([new SpecialMatch(0), new NormalMatch(1), new NormalMatch(2), new NormalMatch(3)]);

btpath = "AbcdefzBcdefCdefDefEfF";
btspecials = fSC(btpath);
Expand All @@ -154,7 +154,19 @@ define(function (require, exports, module) {
new NormalMatch(12), new NormalMatch(13), new SpecialMatch(14)
]);
});

it("should prefer contiguous matches after a point", function () {
var str = "MultiRangeInlineEditor.js";
var strSpecials = fSC(str);
str = str.toLowerCase();
var result = generateMatchList("multir", str, strSpecials.specials, 0);

str = "MultiRangeInlineEditor-test.js";
strSpecials = fSC(str);
str = str.toLowerCase();
var result2 = generateMatchList("multir", str, strSpecials.specials, 0);
expect(result).toEqual(result2);
});
});

describe("_computeRangesAndScore", function () {
Expand Down Expand Up @@ -224,7 +236,7 @@ define(function (require, exports, module) {
matchList: [
new SpecialMatch(13),
new NormalMatch(14),
new SpecialMatch(21)
new NormalMatch(15)
]
});

Expand Down Expand Up @@ -291,7 +303,7 @@ define(function (require, exports, module) {
matchList: [
new SpecialMatch(13),
new NormalMatch(14),
new SpecialMatch(21)
new NormalMatch(15)
]
});
});
Expand Down Expand Up @@ -319,13 +331,13 @@ define(function (require, exports, module) {
new SpecialMatch(0),
new SpecialMatch(13),
new NormalMatch(14),
new SpecialMatch(21)
new NormalMatch(15)
]);

expect(wholeStringSearch("doc", comparePath, sc.specials, sc.lastSegmentSpecialsIndex)).toEqual([
new SpecialMatch(13),
new NormalMatch(14),
new SpecialMatch(21)
new NormalMatch(15)
]);

expect(wholeStringSearch("z", comparePath, sc.specials, sc.lastSegmentSpecialsIndex)).toEqual(null);
Expand All @@ -336,7 +348,7 @@ define(function (require, exports, module) {
new NormalMatch(6),
new SpecialMatch(13),
new NormalMatch(14),
new SpecialMatch(21)
new NormalMatch(15)
]);

// test for a suspected bug where specials are matched out of order.
Expand Down