Fix some visualizer crashes and misbehavior #734
Conversation
Happy to improve it and build on your original work. If you ever get a few minutes, I'd enjoy brainstorming some ideas as to why nested property visualization doesn't seem functional, and what the proper syntax is to visualize IInspectable (aka System.Object) property types. |
arkrumbe
left a comment
arkrumbe
left a comment
There was a problem hiding this comment.
Not my codebase, so I won't block on feedback or anything, but just had a few questions.
| [&](ElementType type) | ||
| { | ||
| if ((type < ElementType::Boolean) || (type > ElementType::String)) | ||
| if ((ElementType::Boolean <= type) && (type <= ElementType::String)) |
There was a problem hiding this comment.
I figure you're about to merge this anyway, but 3 questions:
- I'm assuming ElementType is an enum or something that supports std::less, so you can do this comparison safely?
- Why are you flipping the operands and putting type as the second operand in the first comparison. That seems smelly.
- Can you explain the logic here: is it "if it's between these types"?
There was a problem hiding this comment.
Yes, ElementType is an enum.
I'm doing the operands that way, because it most closely mimics the visuals of "min <= x <= max". I've found this to be the least error-prone and most intuitive way to compare a value against a range.
The logic of "between this types" is that this is the range of "fundamental" types, as defined in the winmd. Unfortunately, treating it this way led to the bug that I had to fix, as it inadvertently ignored ElementType::Object, which is another possible type that is represented via a simple ElementType. Honestly, I think this would be more intuitive and less brittle as a switch-case statement, mapping the various simple ElementType values to their corresponding 'PropertyValue' values. I'm likely to make that change in the near future.
| { | ||
| //propDisplayType = L"winrt::Windows::Foundation::IInspectable"; | ||
| //propCategory = PropertyCategory::Class; | ||
| //propAbiType = L"winrt::impl::inspectable_abi*"; |
There was a problem hiding this comment.
I don't know about your codebase: do you tend to put TODOs when you know future work needs to happen (i.e. The object visualization issue that you have to fix in future)
There was a problem hiding this comment.
Yes, this should be a TODO - ideally one that also includes an issue #.
| { | ||
| return; | ||
| propCategory = (PropertyCategory)(static_cast<std::underlying_type<ElementType>::type>(type) - | ||
| static_cast<std::underlying_type<ElementType>::type>(ElementType::Boolean)); |
There was a problem hiding this comment.
and then just a C-style cast back? Confused.
There was a problem hiding this comment.
The two enums have corresponding ranges. Think of it as analogous to performing an upper-to-lower case conversion like this:
char upper_to_lower(char input)
{
return (input - 'A') + 'a';
}| propertyData.push_back({ propIid, propIndex, propCategory, propAbiType, propDisplayType, propDisplayName }); | ||
| if (propCategory) | ||
| { | ||
| auto propName = method.Name().substr(4); |
There was a problem hiding this comment.
I believe because this is stripping off the leading "get_" in the method name.
If I was being pedantic, the metadata spec doesn't actually mandate that the getter for a property Foo be named "get_Foo". It's just a convention used by all the tooling that we're aware of. The slightly more robust way (and slightly more expensive way) of doing this would be to utilize the PropertyMap and MethodSemantics tables to lookup each property getter for a type, and extract the name directly from the Property table.
| for (auto&& impl : impls) | ||
| { | ||
| GetInterfaceData(impl.Interface(), m_propertyData, m_isStringable); | ||
| GetInterfaceData(process, impl.Interface(), m_propertyData, m_isStringable); |
There was a problem hiding this comment.
Why the inclusion of this process as a param?
There was a problem hiding this comment.
So it can be supplied to the call to NatvisDiagnostic for logging properties we couldn't visualize.
|
I was getting the VS debugger to crash when using the visualizer in the past. do you think this change could have addressed that? also, want to point out #571 as another debugging related issue |
Users trying to debug Xaml apps were having issues that rendered the debugging experience unusable. Namely:
In this change:
I tested this by excercising scenarios that were previously blowing up the visualizer. Specifically, setting a breakpoint in a Xaml Button click handler, and trying to inspect the "sender" object. This sender is a Xaml Button, and so its property list satisfies all three conditions: it has generics, IInspectables, and a long list of properties that requires the debugger to make multiple requests.