fixes background reset - #2900
Conversation
| Some(Severity::Info) => info, | ||
| Some(Severity::Hint) => hint, | ||
| }); | ||
| let style = Style::default().remove_modifier(Modifier::all()).patch( |
There was a problem hiding this comment.
Would it be better if we patch it twice after reset? First with normal text background, then with diagnostic style.
Or another approach is to just fix the styles.
There was a problem hiding this comment.
I'm not sure I understand. This is intended to fix the reported problem by removing the modifiers for the diagnostics (prior to applying the diagnostic style, which can include modifiers) while leaving the other style elements untouched.
What would patching twice buy us here?
There was a problem hiding this comment.
Yeah we could do
Style::reset().patch(background_style).patch(
match {
// ... that existing match expression ...
},
)I'm pretty sure that would be correct - I don't think there are any odd scenarios where we would want inherit a background that isn't the regular background, right 🤔?
There was a problem hiding this comment.
Ah, I understand. OK. One sec.
There was a problem hiding this comment.
reset.patch(background) should be identical to just using background though. It's just a completely blank style
There was a problem hiding this comment.
Looking at Style::reset, it looks like it'll start out with a sub_modifier of Modifier::all() which I think is what we need to clear the italics. We could probably do:
background_style.remove_modifier(Modifier::all())to be more clear that we only want to drop the modifiers from the background
There was a problem hiding this comment.
Did you want me to follow up with this change, or are we all good at this point?
There was a problem hiding this comment.
This looks like it's working well for me now so I say no need for any more changes 👍
There was a problem hiding this comment.
Copy. Thanks, and sorry for the original oversight.
the-mikedavis
left a comment
the-mikedavis
left a comment
There was a problem hiding this comment.
I think this should do the trick 👍
4 participants
Fixes #2856 (comment). Another set of eyes on this would be good, but here's how it looks (before / after; backgrounds deliberately garish):
Before:
After:
