Skip to content

Commit fe51d30

Browse files
thecoolwinterthecoolwinter
authored
Fix Bugs With Multiple Highlight Providers (#346)
### Description Fixes a few issues when using multiple highlight providers. - RangeStore did not clamp its output to only the requested range when querying for runs in a specific range. This broke an assumption by the styled store container, which broke the resulting highlighted ranges. - Adds some randomized testing to the query method to ensure that runtime contract is maintained. - When reordering or adding new providers, their inferred priority would be lost. ### Related Issues * N/A ### Checklist - [x] I read and understood the [contributing guide](https://github.com/CodeEditApp/CodeEdit/blob/main/CONTRIBUTING.md) as well as the [code of conduct](https://github.com/CodeEditApp/CodeEdit/blob/main/CODE_OF_CONDUCT.md) - [x] The issues this PR addresses are related to each other - [x] My changes generate no new warnings - [x] My code builds and runs on my machine - [x] My changes are all related to the related issue above - [x] I documented my code ### Screenshots N/A
1 parent afc5752 commit fe51d30

File tree

6 files changed

+263
-178
lines changed

6 files changed

+263
-178
lines changed

‎Sources/CodeEditSourceEditor/Highlighting/Highlighter.swift‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ class Highlighter: NSObject {
163163
let difference = newIds.difference(from: existingIds).inferringMoves()
164164

165165
var highlightProviders = self.highlightProviders // Make a mutable copy
166-
var moveMap: [Int: HighlightProviderState] = [:]
166+
var moveMap: [Int: (Int, HighlightProviderState)] = [:]
167167

168168
for change in difference {
169169
switch change {
@@ -174,7 +174,8 @@ class Highlighter: NSObject {
174174
guard let movedProvider = moveMap[offset] else {
175175
continue
176176
}
177-
highlightProviders.insert(movedProvider, at: offset)
177+
highlightProviders.insert(movedProvider.1, at: offset)
178+
styleContainer.setPriority(providerId: movedProvider.0, priority: offset)
178179
continue
179180
}
180181
// Set up a new provider and insert it with a unique ID
@@ -188,12 +189,12 @@ class Highlighter: NSObject {
188189
language: language
189190
)
190191
highlightProviders.insert(state, at: offset)
191-
styleContainer.addProvider(providerIdCounter, documentLength: textView.length)
192+
styleContainer.addProvider(providerIdCounter, priority: offset, documentLength: textView.length)
192193
state.invalidate() // Invalidate this new one
193194
case let .remove(offset, _, associatedOffset):
194-
guard associatedOffset == nil else {
195+
if let associatedOffset {
195196
// Moved, add it to the move map
196-
moveMap[associatedOffset!] = highlightProviders.remove(at: offset)
197+
moveMap[associatedOffset] = (offset, highlightProviders.remove(at: offset))
197198
continue
198199
}
199200
// Removed entirely

‎Sources/CodeEditSourceEditor/Highlighting/StyledRangeContainer/StyledRangeContainer+runsIn.swift‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
//
2+
// StyledRangeContainer+runsIn.swift
3+
// CodeEditSourceEditor
4+
//
5+
// Created by Khan Winter on 7/18/25.
6+
//
7+
8+
import Foundation
9+
10+
extension StyledRangeContainer {
11+
/// Coalesces all styled runs into a single continuous array of styled runs.
12+
///
13+
/// When there is an overlapping, conflicting style (eg: provider 2 gives `.comment` to the range `0..<2`, and
14+
/// provider 1 gives `.string` to `1..<2`), the provider with a lower identifier will be prioritized. In the example
15+
/// case, the final value would be `0..<1=.comment` and `1..<2=.string`.
16+
///
17+
/// - Parameter range: The range to query.
18+
/// - Returns: An array of continuous styled runs.
19+
func runsIn(range: NSRange) -> [RangeStoreRun<StyleElement>] {
20+
func combineLowerPriority(_ lhs: inout RangeStoreRun<StyleElement>, _ rhs: RangeStoreRun<StyleElement>) {
21+
lhs.value = lhs.value?.combineLowerPriority(rhs.value) ?? rhs.value
22+
}
23+
24+
func combineHigherPriority(_ lhs: inout RangeStoreRun<StyleElement>, _ rhs: RangeStoreRun<StyleElement>) {
25+
lhs.value = lhs.value?.combineHigherPriority(rhs.value) ?? rhs.value
26+
}
27+
28+
// Ordered by priority, lower = higher priority.
29+
var allRuns = _storage.values
30+
.sorted(by: { $0.priority < $1.priority })
31+
.map { $0.store.runs(in: range.intRange) }
32+
33+
var runs: [RangeStoreRun<StyleElement>] = []
34+
var minValue = allRuns.compactMap { $0.last }.enumerated().min(by: { $0.1.length < $1.1.length })
35+
var counter = 0
36+
37+
while let value = minValue {
38+
// Get minimum length off the end of each array
39+
let minRunIdx = value.offset
40+
var minRun = value.element
41+
42+
for idx in (0..<allRuns.count).reversed() where idx != minRunIdx {
43+
guard let last = allRuns[idx].last else { continue }
44+
45+
if idx < minRunIdx {
46+
combineHigherPriority(&minRun, last)
47+
} else {
48+
combineLowerPriority(&minRun, last)
49+
}
50+
51+
if last.length == minRun.length {
52+
allRuns[idx].removeLast()
53+
} else {
54+
// safe due to guard a few lines above.
55+
allRuns[idx][allRuns[idx].count - 1].subtractLength(minRun)
56+
}
57+
}
58+
59+
if !allRuns[minRunIdx].isEmpty {
60+
allRuns[minRunIdx].removeLast()
61+
}
62+
63+
assert(minRun.length > 0, "Empty or negative runs are not allowed.")
64+
runs.append(minRun)
65+
minValue = allRuns.compactMap { $0.last }.enumerated().min(by: { $0.1.length < $1.1.length })
66+
67+
counter += 1
68+
}
69+
70+
return runs.reversed()
71+
}
72+
}

‎Sources/CodeEditSourceEditor/Highlighting/StyledRangeContainer/StyledRangeContainer.swift‎

Lines changed: 13 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ class StyledRangeContainer {
7070
}
7171
}
7272

73-
var _storage: [ProviderID: RangeStore<StyleElement>] = [:]
73+
var _storage: [ProviderID: (store: RangeStore<StyleElement>, priority: Int)] = [:]
7474
weak var delegate: StyledRangeContainerDelegate?
7575

7676
/// Initialize the container with a list of provider identifiers. Each provider is given an id, they should be
@@ -80,17 +80,21 @@ class StyledRangeContainer {
8080
/// - providers: An array of identifiers given to providers.
8181
init(documentLength: Int, providers: [ProviderID]) {
8282
for provider in providers {
83-
_storage[provider] = RangeStore<StyleElement>(documentLength: documentLength)
83+
_storage[provider] = (store: RangeStore<StyleElement>(documentLength: documentLength), priority: provider)
8484
}
8585
}
8686

87-
func addProvider(_ id: ProviderID, documentLength: Int) {
87+
func addProvider(_ id: ProviderID, priority: Int, documentLength: Int) {
8888
assert(!_storage.keys.contains(id), "Provider already exists")
89-
_storage[id] = RangeStore<StyleElement>(documentLength: documentLength)
89+
_storage[id] = (store: RangeStore<StyleElement>(documentLength: documentLength), priority: priority)
90+
}
91+
92+
func setPriority(providerId: ProviderID, priority: Int) {
93+
_storage[providerId]?.priority = priority
9094
}
9195

9296
func removeProvider(_ id: ProviderID) {
93-
guard let provider = _storage[id] else { return }
97+
guard let provider = _storage[id]?.store else { return }
9498
applyHighlightResult(
9599
provider: id,
96100
highlights: [],
@@ -99,64 +103,9 @@ class StyledRangeContainer {
99103
_storage.removeValue(forKey: id)
100104
}
101105

102-
/// Coalesces all styled runs into a single continuous array of styled runs.
103-
///
104-
/// When there is an overlapping, conflicting style (eg: provider 2 gives `.comment` to the range `0..<2`, and
105-
/// provider 1 gives `.string` to `1..<2`), the provider with a lower identifier will be prioritized. In the example
106-
/// case, the final value would be `0..<1=.comment` and `1..<2=.string`.
107-
///
108-
/// - Parameter range: The range to query.
109-
/// - Returns: An array of continuous styled runs.
110-
func runsIn(range: NSRange) -> [RangeStoreRun<StyleElement>] {
111-
func combineLowerPriority(_ lhs: inout RangeStoreRun<StyleElement>, _ rhs: RangeStoreRun<StyleElement>) {
112-
lhs.value = lhs.value?.combineLowerPriority(rhs.value) ?? rhs.value
113-
}
114-
115-
func combineHigherPriority(_ lhs: inout RangeStoreRun<StyleElement>, _ rhs: RangeStoreRun<StyleElement>) {
116-
lhs.value = lhs.value?.combineHigherPriority(rhs.value) ?? rhs.value
117-
}
118-
119-
// Ordered by priority, lower = higher priority.
120-
var allRuns = _storage.sorted(by: { $0.key < $1.key }).map { $0.value.runs(in: range.intRange) }
121-
var runs: [RangeStoreRun<StyleElement>] = []
122-
123-
var minValue = allRuns.compactMap { $0.last }.enumerated().min(by: { $0.1.length < $1.1.length })
124-
125-
while let value = minValue {
126-
// Get minimum length off the end of each array
127-
let minRunIdx = value.offset
128-
var minRun = value.element
129-
130-
for idx in (0..<allRuns.count).reversed() where idx != minRunIdx {
131-
guard let last = allRuns[idx].last else { continue }
132-
if idx < minRunIdx {
133-
combineHigherPriority(&minRun, last)
134-
} else {
135-
combineLowerPriority(&minRun, last)
136-
}
137-
138-
if last.length == minRun.length {
139-
allRuns[idx].removeLast()
140-
} else {
141-
// safe due to guard a few lines above.
142-
allRuns[idx][allRuns[idx].count - 1].subtractLength(minRun)
143-
}
144-
}
145-
146-
if !allRuns[minRunIdx].isEmpty {
147-
allRuns[minRunIdx].removeLast()
148-
}
149-
150-
runs.append(minRun)
151-
minValue = allRuns.compactMap { $0.last }.enumerated().min(by: { $0.1.length < $1.1.length })
152-
}
153-
154-
return runs.reversed()
155-
}
156-
157106
func storageUpdated(editedRange: NSRange, changeInLength delta: Int) {
158107
for key in _storage.keys {
159-
_storage[key]?.storageUpdated(editedRange: editedRange, changeInLength: delta)
108+
_storage[key]?.store.storageUpdated(editedRange: editedRange, changeInLength: delta)
160109
}
161110
}
162111
}
@@ -171,7 +120,7 @@ extension StyledRangeContainer: HighlightProviderStateDelegate {
171120
/// - rangeToHighlight: The range to apply the highlights to.
172121
func applyHighlightResult(provider: ProviderID, highlights: [HighlightRange], rangeToHighlight: NSRange) {
173122
assert(rangeToHighlight != .notFound, "NSNotFound is an invalid highlight range")
174-
guard var storage = _storage[provider] else {
123+
guard var storage = _storage[provider]?.store else {
175124
assertionFailure("No storage found for the given provider: \(provider)")
176125
return
177126
}
@@ -193,12 +142,12 @@ extension StyledRangeContainer: HighlightProviderStateDelegate {
193142
lastIndex = highlight.range.max
194143
}
195144

196-
if lastIndex != rangeToHighlight.upperBound {
145+
if lastIndex < rangeToHighlight.upperBound {
197146
runs.append(.empty(length: rangeToHighlight.upperBound - lastIndex))
198147
}
199148

200149
storage.set(runs: runs, for: rangeToHighlight.intRange)
201-
_storage[provider] = storage
150+
_storage[provider]?.store = storage
202151
delegate?.styleContainerDidUpdate(in: rangeToHighlight)
203152
}
204153
}

‎Sources/CodeEditSourceEditor/RangeStore/RangeStore.swift‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,23 +39,31 @@ struct RangeStore<Element: RangeStoreElement>: Sendable {
3939
/// - Parameter range: The range to query.
4040
/// - Returns: A continuous array of runs representing the queried range.
4141
func runs(in range: Range<Int>) -> [Run] {
42+
let length = _guts.count(in: OffsetMetric())
4243
assert(range.lowerBound >= 0, "Negative lowerBound")
43-
assert(range.upperBound <= _guts.count(in: OffsetMetric()), "upperBound outside valid range")
44+
assert(range.upperBound <= length, "upperBound outside valid range")
4445
if let cache, cache.range == range {
4546
return cache.runs
4647
}
4748

4849
var runs = [Run]()
49-
5050
var index = findIndex(at: range.lowerBound).index
51-
var offset: Int? = range.lowerBound - _guts.offset(of: index, in: OffsetMetric())
51+
var offset: Int = range.lowerBound - _guts.offset(of: index, in: OffsetMetric())
52+
var remainingLength = range.upperBound - range.lowerBound
5253

53-
while index < _guts.endIndex, _guts.offset(of: index, in: OffsetMetric()) < range.upperBound {
54+
while index < _guts.endIndex,
55+
_guts.offset(of: index, in: OffsetMetric()) < range.upperBound,
56+
remainingLength > 0 {
5457
let run = _guts[index]
55-
runs.append(Run(length: run.length - (offset ?? 0), value: run.value))
58+
let runLength = min(run.length - offset, remainingLength)
59+
runs.append(Run(length: runLength, value: run.value))
5660

61+
remainingLength -= runLength
62+
if remainingLength <= 0 {
63+
break // Avoid even checking the storage for the next index
64+
}
5765
index = _guts.index(after: index)
58-
offset = nil
66+
offset = 0
5967
}
6068

6169
return runs
@@ -83,6 +91,9 @@ struct RangeStore<Element: RangeStoreElement>: Sendable {
8391
storageUpdated(replacedCharactersIn: upperBound..<upperBound, withCount: missingCharacters)
8492
}
8593

94+
// This is quite slow in debug builds but is a *really* important assertion for internal state.
95+
assert(!runs.contains(where: { $0.length < 0 }), "Runs cannot have negative length.")
96+
8697
_guts.replaceSubrange(
8798
range,
8899
in: OffsetMetric(),

‎Tests/CodeEditSourceEditorTests/Highlighting/StyledRangeContainerTests.swift‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ final class StyledRangeContainerTests: XCTestCase {
1111

1212
// Have to do string conversion due to missing Comparable conformance pre-macOS 14
1313
XCTAssertEqual(store._storage.keys.sorted(), providers)
14-
XCTAssert(store._storage.values.allSatisfy({ $0.length == 100 }), "One or more providers have incorrect length")
14+
XCTAssert(
15+
store._storage.values.allSatisfy({ $0.store.length == 100 }),
16+
"One or more providers have incorrect length"
17+
)
1518
}
1619

1720
@MainActor
@@ -26,10 +29,10 @@ final class StyledRangeContainerTests: XCTestCase {
2629
)
2730

2831
XCTAssertNotNil(store._storage[providers[0]])
29-
XCTAssertEqual(store._storage[providers[0]]!.count, 3)
30-
XCTAssertNil(store._storage[providers[0]]!.runs(in: 0..<100)[0].value?.capture)
31-
XCTAssertEqual(store._storage[providers[0]]!.runs(in: 0..<100)[1].value?.capture, .comment)
32-
XCTAssertNil(store._storage[providers[0]]!.runs(in: 0..<100)[2].value?.capture)
32+
XCTAssertEqual(store._storage[providers[0]]!.store.count, 3)
33+
XCTAssertNil(store._storage[providers[0]]!.store.runs(in: 0..<100)[0].value?.capture)
34+
XCTAssertEqual(store._storage[providers[0]]!.store.runs(in: 0..<100)[1].value?.capture, .comment)
35+
XCTAssertNil(store._storage[providers[0]]!.store.runs(in: 0..<100)[2].value?.capture)
3336

3437
XCTAssertEqual(
3538
store.runsIn(range: NSRange(location: 0, length: 100)),

0 commit comments

Comments
 (0)