From bd07054a1bec531b0735b8c73ff3fc41ca5aa1be Mon Sep 17 00:00:00 2001 From: Francisco Ryan Tolmasky I Date: Sun, 21 Jun 2009 20:25:29 -0700 Subject: [PATCH] Fix for removeIndex in CPIndexSet and added tests for CPIndexSet. Reviewed by me. --- AppKit/NEWCPTableView.j | 9 +- Foundation/CPIndexSet.j | 145 ++++++++----- Tests/Foundation/CPIndexSetTest.j | 332 ++++++++++++++++++++++++++++++ 3 files changed, 434 insertions(+), 52 deletions(-) create mode 100644 Tests/Foundation/CPIndexSetTest.j diff --git a/AppKit/NEWCPTableView.j b/AppKit/NEWCPTableView.j index 32ef24b24..09a3b7e3d 100644 --- a/AppKit/NEWCPTableView.j +++ b/AppKit/NEWCPTableView.j @@ -552,8 +552,8 @@ CPTableViewSelectionHighlightStyleSourceList = 1; lastColumn = NUMBER_OF_COLUMNS() - 1; // Don't bother doing the expensive removal of hidden indexes if we have no hidden columns. - if (_numberOfHiddenColumns < 0) - return [CPIndexSet indexSetWithIndexesInRange:CPMakeRange(column, lastColumn - column)]; + if (_numberOfHiddenColumns <= 0) + return [CPIndexSet indexSetWithIndexesInRange:CPMakeRange(column, lastColumn - column + 1)]; // var indexSet = [CPIndexSet indexSet]; @@ -913,12 +913,15 @@ console.profile("cell-load"); newlyExposedRows = [exposedRows copy], newlyExposedColumns = [exposedColumns copy]; +// console.log("exposed rows: " + exposedRows + " exposed columns: " + exposedColumns); +// console.log("but rows: " + _exposedRows + " exposed columns: " + _exposedColumns); + [newlyExposedRows removeIndexes:_exposedRows]; [newlyExposedColumns removeIndexes:_exposedColumns]; [previouslyExposedRows removeIndexes:newlyExposedRows]; [previouslyExposedColumns removeIndexes:newlyExposedColumns]; - //console.log("newly exposed rows: " + newlyExposedRows + "\nnewly exposed columns: " + newlyExposedColumns); +// console.log("newly exposed rows: " + newlyExposedRows + "\nnewly exposed columns: " + newlyExposedColumns); _exposedRows = exposedRows; _exposedColumns = exposedColumns; diff --git a/Foundation/CPIndexSet.j b/Foundation/CPIndexSet.j index 87e208d68..040d32690 100644 --- a/Foundation/CPIndexSet.j +++ b/Foundation/CPIndexSet.j @@ -224,22 +224,20 @@ */ - (BOOL)intersectsIndexesInRange:(CPRange)aRange { - //FIXME: OLD - // This is fast thanks to the _cachedIndexRange. - if(!_count) + if (_count <= 0) return NO; - var i = SOERangeIndex(self, aRange.location), - count = _ranges.length, - upper = CPMaxRange(aRange); + var lhsRangeIndex = assumedPositionOfIndex(_ranges, aRange.location); - // Stop if the location is ever bigger than or equal to our - // non-inclusive upper bound - for (; i < count && _ranges[i].location < upper; ++i) - if(CPIntersectionRange(aRange, _ranges[i]).length) - return YES; + if (FLOOR(lhsRangeIndex) === lhsRangeIndex) + return YES; - return NO; + var rhsRangeIndex = assumedPositionOfIndex(_ranges, CPMaxRange(aRange) - 1); + + if (FLOOR(rhsRangeIndex) === rhsRangeIndex) + return YES; + + return lhsRangeIndex !== rhsRangeIndex; } /*! @@ -411,22 +409,40 @@ - (CPString)description { - var desc = [super description] + " "; + var description = [super description]; if (_count) { - desc += "[number of indexes: " + _count + " (in " + _ranges.length + " ranges), indexes: ("; - for (i = 0; i < _ranges.length; i++) + var index = 0, + count = _ranges.length; + + description += "[number of indexes: " + _count + " (in " + count; + + if (count === 1) + description += " range), indexes: ("; + else + description += " ranges), indexes: ("; + + for (; index < count; ++index) { - desc += _ranges[i].location; - if (_ranges[i].length > 1) desc += "-" + (CPMaxRange(_ranges[i])-1) + "["+_ranges[i].length+"]"; - if (i+1 < _ranges.length) desc += " "; + var range = _ranges[index]; + + description += range.location; + + if (range.length > 1) + description += "-" + (CPMaxRange(range) - 1); + + if (index + 1 < count) + description += " "; } - desc += ")]"; + + description += ")]"; } + else - desc += "(no indexes)"; - return desc; + description += "(no indexes)"; + + return description; } @end @@ -475,20 +491,20 @@ return; } -var x = aRange.location - 1, y = CPMaxRange(aRange); + var rangeCount = _ranges.length, lhsRangeIndex = assumedPositionOfIndex(_ranges, aRange.location - 1), lhsRangeIndexCEIL = CEIL(lhsRangeIndex); -try{ + if (lhsRangeIndexCEIL === lhsRangeIndex && lhsRangeIndexCEIL < rangeCount) aRange = CPUnionRange(aRange, _ranges[lhsRangeIndexCEIL]); -}catch(e) { alert("123456789 " + lhsRangeIndexCEIL + " " + rangeCount + " " + lhsRangeIndex);} + var rhsRangeIndex = assumedPositionOfIndex(_ranges, CPMaxRange(aRange)), rhsRangeIndexFLOOR = FLOOR(rhsRangeIndex); if (rhsRangeIndexFLOOR === rhsRangeIndex && rhsRangeIndexFLOOR > 0) aRange = CPUnionRange(aRange, _ranges[rhsRangeIndexFLOOR]); -//print("the returned indxes were for searching for " + x + " to " + y + " are " + lhsRangeIndex + " and " + rhsRangeIndex); + var removalCount = rhsRangeIndexFLOOR - lhsRangeIndexCEIL + 1; if (removalCount === _ranges.length) @@ -566,7 +582,6 @@ try{ */ - (void)removeIndexesInRange:(CPRange)aRange { - // FIXME: OLD // If empty range, bail. if (aRange.length <= 0) return; @@ -576,36 +591,68 @@ try{ return; var rangeCount = _ranges.length, - lhsRangeIndex = assumedPositionOfIndex(_ranges, aRange.location - 1), + lhsRangeIndex = assumedPositionOfIndex(_ranges, aRange.location), lhsRangeIndexCEIL = CEIL(lhsRangeIndex); - if (lhsRangeIndexCEIL === lhsRangeIndex && lhsRangeIndexCEIL < rangeCount) - aRange = CPUnionRange(aRange, _ranges[lhsRangeIndexCEIL]); + // Do we fall on an actual existing range? + if (lhsRangeIndex === lhsRangeIndexCEIL && lhsRangeIndexCEIL < rangeCount) + { + var existingRange = _ranges[lhsRangeIndexCEIL]; - var rhsRangeIndex = assumedPositionOfIndex(_ranges, CPMaxRange(aRange)), + // If these ranges don't start in the same place, we have to cull it. + if (aRange.location !== existingRange.location) + { + var maxRange = CPMaxRange(aRange), + existingMaxRange = CPMaxRange(existingRange); + + existingRange.length = aRange.location - existingRange.location; + + // If this range is internal to the existing range, we have a unique splitting case. + if (maxRange < existingMaxRange) + { + _count -= aRange.length; + [_ranges insertObject:CPMakeRange(maxRange, existingMaxRange - maxRange) atIndex:lhsRangeIndexCEIL + 1]; + + return; + } + else + { + _count -= existingMaxRange - aRange.location; + lhsRangeIndexCEIL += 1; + } + } + } + + var rhsRangeIndex = assumedPositionOfIndex(_ranges, CPMaxRange(aRange) - 1), rhsRangeIndexFLOOR = FLOOR(rhsRangeIndex); - if (rhsRangeIndexFLOOR === rhsRangeIndex && rhsRangeIndexFLOOR > 0) - aRange = CPUnionRange(aRange, _ranges[rhsRangeIndexFLOOR]); + if (rhsRangeIndex === rhsRangeIndexFLOOR && rhsRangeIndexFLOOR >= 0) + { + var maxRange = CPMaxRange(aRange), + existingRange = _ranges[rhsRangeIndexFLOOR], + existingMaxRange = CPMaxRange(existingRange); + + if (maxRange !== existingMaxRange) + { + _count -= maxRange - existingRange.location; + rhsRangeIndexFLOOR -= 1; // This is accounted for, and thus as if we got the previous spot. + + existingRange.location = maxRange; + existingRange.length = existingMaxRange - maxRange; + } + } var removalCount = rhsRangeIndexFLOOR - lhsRangeIndexCEIL + 1; - if (removalCount === 1) + if (removalCount > 0) { - if (lhsRangeIndexCEIL < _ranges.length) - _count -= _ranges[lhsRangeIndexCEIL].length; + var removal = lhsRangeIndexCEIL, + lastRemoval = lhsRangeIndexCEIL + removalCount - 1; - _count += aRange.length; - _ranges[lhsRangeIndexCEIL] = aRange; - } + for (; removal <= lastRemoval; ++removal) + _count -= _ranges[removal].length; - else - { - if (removalCount > 0) - [_ranges removeObjectsInRange:CPMakeRange(lhsRangeIndexCEIL, removalCount)]; - - [_ranges insertObject:aRange atIndex:lhsRangeIndexCEIL]; - //FIXME COUNT! + [_ranges removeObjectsInRange:CPMakeRange(lhsRangeIndexCEIL, removalCount)]; } } @@ -820,7 +867,7 @@ var assumedPositionOfIndex = function(ranges, anIndex) positionFLOOR = FLOOR(position); if (position === positionFLOOR) - {try{ + { if (positionFLOOR - 1 >= 0 && anIndex < CPMaxRange(ranges[positionFLOOR - 1])) high = middle - 1; @@ -828,10 +875,10 @@ var assumedPositionOfIndex = function(ranges, anIndex) low = middle + 1; else - return positionFLOOR - 0.5;}catch(e) { alert("here!");} + return positionFLOOR - 0.5; } else - {try{ + { var range = ranges[positionFLOOR]; if (anIndex < range.location) @@ -841,7 +888,7 @@ var assumedPositionOfIndex = function(ranges, anIndex) low = middle + 1; else - return positionFLOOR;}catch(e){alert("yes!");} + return positionFLOOR; } } diff --git a/Tests/Foundation/CPIndexSetTest.j b/Tests/Foundation/CPIndexSetTest.j new file mode 100644 index 000000000..1d1b68fee --- /dev/null +++ b/Tests/Foundation/CPIndexSetTest.j @@ -0,0 +1,332 @@ +@import + +function descriptionWithoutEntity(aString) +{ + var descriptionWithEntity = [aString description]; +//print(descriptionWithEntity); + return descriptionWithEntity.substr(descriptionWithEntity.indexOf('>') + 1); +} + +@implementation CPIndexSetTest : OJTestCase +{ + CPIndexSet _set; +} + +- (void)testAddIndexes +{ + var indexSet = [CPIndexSet indexSet]; + + // Test no indexes + [self assert:descriptionWithoutEntity(indexSet) equals:@"(no indexes)"]; + + // Test adding initial range + [indexSet addIndexesInRange:CPMakeRange(30,10)]; + + [self assert:@"[number of indexes: 10 (in 1 range), indexes: (30-39)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test adding range after existing ranges. + [indexSet addIndexesInRange:CPMakeRange(50,10)]; + + [self assert:@"[number of indexes: 20 (in 2 ranges), indexes: (30-39 50-59)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test adding range before existing ranges. + [indexSet addIndexesInRange:CPMakeRange(10,10)]; + + [self assert:@"[number of indexes: 30 (in 3 ranges), indexes: (10-19 30-39 50-59)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test adding range inbetween existing ranges. + [indexSet addIndexesInRange:CPMakeRange(45,2)]; + + [self assert:@"[number of indexes: 32 (in 4 ranges), indexes: (10-19 30-39 45-46 50-59)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test adding single index inbetween existing ranges. + [indexSet addIndexesInRange:CPMakeRange(23,1)]; + + [self assert:@"[number of indexes: 33 (in 5 ranges), indexes: (10-19 23 30-39 45-46 50-59)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test adding range inbetween existing ranges that forces a combination + [indexSet addIndexesInRange:CPMakeRange(47,3)]; + + [self assert:@"[number of indexes: 36 (in 4 ranges), indexes: (10-19 23 30-39 45-59)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test adding range across ranges forcing a combination + [indexSet addIndexesInRange:CPMakeRange(35,15)]; + + [self assert:@"[number of indexes: 41 (in 3 ranges), indexes: (10-19 23 30-59)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test adding range across two empty slots forcing a combination + [indexSet addIndexesInRange:CPMakeRange(0,70)]; + + [self assert:@"[number of indexes: 70 (in 1 range), indexes: (0-69)]" equals:descriptionWithoutEntity(indexSet)]; +} + +- (void)testRemoveIndexes +{ + var indexSet = [CPIndexSet indexSet]; + + // Test no indexes + [self assert:descriptionWithoutEntity(indexSet) equals:@"(no indexes)"]; + + // Test adding initial range + [indexSet addIndexesInRange:CPMakeRange(0, 70)]; + + [self assert:@"[number of indexes: 70 (in 1 range), indexes: (0-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test remove range that is subset of existing range, causing a split. + [indexSet removeIndexesInRange:CPMakeRange(30, 10)]; + + [self assert:@"[number of indexes: 60 (in 2 ranges), indexes: (0-29 40-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test remove range that is subset of existing range, causing a split. + [indexSet removeIndexesInRange:CPMakeRange(50, 5)]; + + [self assert:@"[number of indexes: 55 (in 3 ranges), indexes: (0-29 40-49 55-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test remove index that is subset of existing range, causing a split. + [indexSet removeIndex:57]; + + [self assert:@"[number of indexes: 54 (in 4 ranges), indexes: (0-29 40-49 55-56 58-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test remove range that is an exactly represented in the set. + [indexSet removeIndexesInRange:CPMakeRange(40, 10)]; + + [self assert:@"[number of indexes: 44 (in 3 ranges), indexes: (0-29 55-56 58-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test remove range that isn't in the set. + [indexSet removeIndexesInRange:CPMakeRange(35, 3)]; + + [self assert:@"[number of indexes: 44 (in 3 ranges), indexes: (0-29 55-56 58-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test remove range that is partially in a left range. + [indexSet removeIndexesInRange:CPMakeRange(25, 7)]; + + [self assert:@"[number of indexes: 39 (in 3 ranges), indexes: (0-24 55-56 58-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test remove range that is partially in a left range. + [indexSet removeIndexesInRange:CPMakeRange(57, 3)]; + + [self assert:@"[number of indexes: 37 (in 3 ranges), indexes: (0-24 55-56 60-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Test remove range that is partially in a left and right range. + [indexSet removeIndexesInRange:CPMakeRange(20, 36)]; + + [self assert:@"[number of indexes: 31 (in 3 ranges), indexes: (0-19 56 60-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Remove single index that represents an entire range. + [indexSet removeIndex:56]; + + [self assert:@"[number of indexes: 30 (in 2 ranges), indexes: (0-19 60-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Remove index set that is subset of existing range, causing a split. + [indexSet removeIndexes:[CPIndexSet indexSetWithIndexesInRange:CPMakeRange(5, 10)]]; + + [self assert:@"[number of indexes: 20 (in 3 ranges), indexes: (0-4 15-19 60-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Remove indexes that are partially in 2 ranges and contains intermediate range. + [indexSet removeIndexesInRange:CPMakeRange(2, 62)]; + + [self assert:@"[number of indexes: 8 (in 2 ranges), indexes: (0-1 64-69)]" equals:descriptionWithoutEntity(indexSet)]; + + // Remove indexes that fit exactly in 2 ranges. + [indexSet removeIndexesInRange:CPMakeRange(0, 70)]; + + [self assert:@"(no indexes)" equals:descriptionWithoutEntity(indexSet)]; + + indexSet = [CPIndexSet indexSetWithIndexesInRange:CPMakeRange(0, 30)]; + + // Remove indexes from left hand of single range + [indexSet removeIndexesInRange:CPMakeRange(0, 29)]; + + [self assert:@"[number of indexes: 1 (in 1 range), indexes: (29)]" equals:descriptionWithoutEntity(indexSet)]; + + indexSet = [CPIndexSet indexSetWithIndexesInRange:CPMakeRange(0, 30)]; + + // Remove indexes from right hand of single range + [indexSet removeIndexesInRange:CPMakeRange(1, 29)]; + + [self assert:@"[number of indexes: 1 (in 1 range), indexes: (0)]" equals:descriptionWithoutEntity(indexSet)]; +} + +- (void)setUp +{ + _set = [CPIndexSet indexSetWithIndexesInRange:CPMakeRange(10, 10)]; +} + +- (void)tearDown +{ + _set = nil; +} + +- (void)testIndexSet:(CPIndexSet)set containsRange:(CPRange)range +{ + [self assertFalse:[set containsIndex:range.location -1]]; + + for (var i=range.location, max=CPMaxRange(range); i