Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing a [DebuggerTypeProxy] to the generated quantities #1390

Merged
merged 3 commits into from
Jun 15, 2024

Conversation

lipchev
Copy link
Collaborator

@lipchev lipchev commented May 21, 2024

/// <summary>
///     Serves as a debug display proxy for a quantity, providing a convenient way to view various components of the
///     quantity during debugging.
/// </summary>
/// <remarks>
///     This struct provides a structured view of a quantity's components such as abbreviation, unit, value, and convertor
///     during debugging.
///     Each component is represented by a nested struct, which can be expanded in the debugger to inspect its properties.
/// </remarks>
internal readonly struct QuantityDisplay(IQuantity quantity)

This is added to all quantities such:

    [DataContract]
    [DebuggerTypeProxy(typeof(QuantityDisplay))]
    public readonly partial struct Mass :

There are 4 properties visible, each of which can be further expanded:

  1. Abbreviation: displays the DefaultAbbreviation and expands with the list all abbreviations for the current unit, also holds a property with the possible unit conversions (displaying the target abbreviation in the ConvertedQuantity[])
  2. Unit: displays the Unit such as Milligram and expands to the list of other options, where every other options expands to the respective ConvertedQuantity (much like a smart enum).
  3. Value: displays the DoubleValue by default and expands with the IsDecimal, DecimalValue and DoubleValue properties
  4. ValueConversions: displays the output of the QuantityToString by default and expands with the list of possible QuantityToString options (currently only has the GeneralFormat and ShortFormat properties) as well as the list of possible unit conversions (ConvertedQuantity[])

Note that I've had to carefully pick the property names such that they are sorted (more or less) how I wanted, as visual studio sorts them alphabetically, not by the order of declaration. Perhaps there (is / would one day be) an option to change that behavior but, yeah- that's it for now.

I've only tested this with Visual Studio 2022 - but i don't see why there should by any issues with the other IDEs. I did observe that not all things are always displayed on net48 (sometimes there is the _"..requires thread to complete..").

image
image
image
image
image

If we open the visualizer view with the list from the QuantityToUnit we get a nice little table:

image

@lipchev
Copy link
Collaborator Author

lipchev commented May 21, 2024

I initially had the Abbreviation put inside the Unit section but that ruined the smart enum effect. However now that I'm looking at it again, I think it might actually be better to have the Abbreviation named UnitAbbreviation (which would place it right underneath the Unit. It's a bit longer (as text) but the expansion using the keyboard (from the top) would be shorter.. I don't know- you decide..

@angularsen
Copy link
Owner

angularsen commented May 24, 2024

I tested in JetBrains Rider.

I like it a lot, the default view is very verbose with all conversion properties so this helps clean it up and you still have access the the raw debug output 👍

image

I like how you can "dig" through conversions and view the converted quantities 👍
image

I like how you can preview all the available conversions for the current selected quantity:
image

I think maybe Abbreviation.Conversions feels redundant or not so useful? At least for me, digging through Units is more intuitive.
image

Can we add QuantityInfo too, for completeness?
Then I don't imagine ever having to expand the raw debug output for anything.
We can skip Dimensions, since QuantityInfo already describes it.

ConvertedQuantity.Value could maybe reuse ValueDisplay to be consistent and avoid the decimal error? Or, can maybe ConvertedQuantity inherit from QuantityDisplay to get the same base output, just extending with whatever extra the converted quantity holds?
image

@lipchev
Copy link
Collaborator Author

lipchev commented May 24, 2024

I think maybe Abbreviation.Conversions feels redundant or not so useful? At least for me, digging through Units is more intuitive.

For the quantities I'm familiar with, I tend to navigate using the Units as well, wasn't sure what's the more natural way for other/ new users. This is also why I've been thinking about changing the name to UnitAbbreviation so that I can get to my preferred unit conversion with just one click (from the top) of the right arrow key. However I do think there is some benefit in keeping the list separate from the version of the ValueConverter (perhaps under a different name, maybe without the conversion-drill-down).
Have a look at this example: (visually) parsing the first half of the image takes a fraction from what it takes for the second half:

image

Here is the renamed version - it's just the Conversions property name that I find somewhat displeasing, but AbbreviationInOtherUnits didn't seem right either..

image

Can we add QuantityInfo too, for completeness? Then I don't imagine ever having to expand the raw debug output for anything. We can skip Dimensions, since QuantityInfo already describes it.

We can have anything, just need a name that would sort correctly in the display: the "Q" in QuantityInfo will have it appear between the Abbreviation and the Unit which is a bit annoying. I was also thinking of exposing the QuantityInfo with the {Name} or {PluralName} display by default, however I couldn't think of the name (or sub-section) to use for it..

ConvertedQuantity.Value could maybe reuse ValueDisplay to be consistent and avoid the decimal error? Or, can maybe ConvertedQuantity inherit from QuantityDisplay to get the same base output, just extending with whatever extra the converted quantity holds?

Yes, I must have missed that (they are both displaying the same way in v6).

@angularsen
Copy link
Owner

it's just the Conversions property name that I find somewhat displeasing

I think it's fine, we can always revisit.

the "Q" in QuantityInfo will have it appear between the Abbreviation and the Unit which is a bit annoying.

For me, that's entirely fine. I'm not so focused on the order, as much as having a fairly short list of properties that is easy to read and dig through.

@lipchev
Copy link
Collaborator Author

lipchev commented Jun 13, 2024

@angularsen I think this should be good (for now): I've added the missing ValueDisplay and renamed the Abbreviation to UnitAbbreviation having the Unit enum appear on top.

@angularsen angularsen merged commit 25b0fcc into angularsen:master Jun 15, 2024
1 check passed
@angularsen
Copy link
Owner

Perfect, can you also port this to v6 branch?

@angularsen
Copy link
Owner

@lipchev
Copy link
Collaborator Author

lipchev commented Jun 16, 2024

@angularsen I ported the modifications to v6, and was just having a closer look at the screenshots you posted from Rider- that thing about the fully.qualified.type.name preceding the actual-value, that's pretty annoying to look at IMO. Is that something that's configurable? I didn't really like how long the namespaces was when looking at it as the "right-side-column" (the VS way)- maybe we could do something to shorten the namespace (that would probably require moving each proxy into its own file)?

@angularsen
Copy link
Owner

I honestly did not notice, but sure, it may shorten the name a bit if you move the types so they are not nested, but it seems Rider shows a kind of breadcrumb in this scenario for the path I have expanded.

As for configurable, I can disable "fully qualified", but it only removes the leading "UnitsNet" part. For me, I would not bother changing this anyway. I'm fine with it as-is.

image

image

@angularsen
Copy link
Owner

Do you want to make any changes before I merge #1395 ?

@lipchev
Copy link
Collaborator Author

lipchev commented Jun 16, 2024

Nah, we can do it later (perhaps even making the UnitsNet.Display namespace public or something).

angularsen pushed a commit that referenced this pull request Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants