Remove IconElement caching from PaletteItem#19149
Conversation
In #19132, we introduced caching for BasePaletteItem::ResolvedIcon as a late optimization. We actually can't cache those, because they're UI elements, with parents and relationships and all. When you filter a list with icons and the list elements change physical position, they're assigned new templates-- and new parents. Fixes e7939bb
|
This one is for @e82eric :) |
Hi @OrangeDoro. Just reviewed and approved this PR. Here's some feedback:
Yes. It's nice that it calls out the null pointer dereference.
Honestly no, at least not in this instance. The code change in the PR was very small and well summarized by the PR title and body. Reading through the feedback generated by your tool just added to what I had to read/review. Personally, I think this could be more helpful on a more complex PR, especially if it's feature development instead of a bug fix. Pushing for unit tests in that scenario would definitely be helpful. Good luck on your research! |
In #19132, we introduced caching for BasePaletteItem::ResolvedIcon as a late optimization.
We actually can't cache those, because they're UI elements, with parents and relationships and all. When you filter a list with icons and the list elements change physical position, they're assigned new templates-- and new parents.
Fixes e7939bb