Skip to content

Commit ac47208

Browse files
committed
Apply pending styles before active styles
This is a tricky one, since the original reasoning for applying active styles first is still correct, but an edge case disrupts it. When parsing an nested expression, when the start of an annotation is green it becomes active, and when it is closed it becomes pending. Take the following string for example: {red:hello {green:there {blue:you} and} good} day The list of active (and pending) styles would look like so: - red - red, green - red, green, blue - red, green (blue pending) - red (green pending) - (red pending) As such, the pending styles are the most recently applied, and so should be applied last to have a higher priority, as is appropriate. However, this situation becomes complicated when interpolation occurs, as when an expression is interpolated we need to jump out of the usual processing, and active styles can be more specific than pending. Consider this example {red:hello} {green:there$(interpolation)} Now the active/pending styles are: - red, - (red pending) - green (red pending) - interpolation happens! In this situation, note that the pending style should be applied first. If this was the only code responsible for annotation ordering was in addpart!, we'd be in a bit of a conundrum, because fixing the second example would break the first. However, thanks to other recent efforts to handle nested annotations better (e.g. dfef96d) we can adjust the code in addpart! to handle the linear/interpolated case correctly, and the nested behaviour isn't broken. To be completely frank, I'm not entirely sure that this is exactly and everything going on. This is arguably the most complicated part of the code base, and (re)building a mental model of the entire parser intricate enough for me to be able to declare this with confidence would be the work of a morning/afternoon, at least. However, it is well-tested, and with this change none of the tests break (and one is fixed). As such, I feel there's sufficient justification to run with the theory I outlined above until such a time that further complications pop up (if they ever do). We should add some more tests around annotation order, but for now we seem better off with this change than without it. Hopefully in the future, further tests will be written for this part of the codebase, and the behaviour entirely ironed down. Writing intelligent tests does take some effort though, so for now I'll take better observed behaviour and potential future bug-fixes. Ideally some other contributors will be attracted to this project and somebody other than me will write these tests :)
1 parent 85dd300 commit ac47208

File tree

1 file changed

+7
-7
lines changed

1 file changed

+7
-7
lines changed

src/styledmarkup.jl

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -207,20 +207,20 @@ function addpart!(state::State, stop::Int)
207207
str
208208
else
209209
styles = sty_type[]
210-
relevant_styles = Iterators.filter(
211-
(_, start, _)::Tuple -> start <= stop + state.offset[] + 1,
212-
Iterators.flatten(map(reverse, state.active_styles)))
213-
for (_, start, annot) in relevant_styles
214-
range = (start - state.point[]):(stop - state.point[] + state.offset[] + 1)
215-
push!(styles, tupl(range, annot))
216-
end
217210
sort!(state.pending_styles, by = (r -> (first(r), -last(r))) first) # Prioritise the most specific styles
218211
for (range, annot) in state.pending_styles
219212
if !isempty(range)
220213
push!(styles, tupl(range .- state.point[], annot))
221214
end
222215
end
223216
empty!(state.pending_styles)
217+
relevant_styles = Iterators.filter(
218+
(_, start, _)::Tuple -> start <= stop + state.offset[] + 1,
219+
Iterators.flatten(map(reverse, state.active_styles)))
220+
for (_, start, annot) in relevant_styles
221+
range = (start - state.point[]):(stop - state.point[] + state.offset[] + 1)
222+
push!(styles, tupl(range, annot))
223+
end
224224
if isempty(styles)
225225
str
226226
elseif !ismacro(state)

0 commit comments

Comments
 (0)