Improve Summary screen by adding more details about applied resources

[betanzos]

Summary of the pull request

Add new variants for the applied resource summary.

References and relevant issues

#873

Detailed description of the pull request / Additional comments

The current applied resource summary only includes the Intent and the Unit Name and this PR adds three new variants for the applied resource summary, resulting in four available variants as shown below.

New variants will be choosen according to the presence of the unit Identifier (id) and the Description directive.

• FULL: Used when all the info is available

  Format: {Intent} : {Description} [{UnitName} {Identifier}]

  

• MISSING ID: Used when only the Identifier is missing

  Format: {Intent} : {Description} [{UnitName}]

  

• MISSING DESCRIPTION: Used when only the Description is missing

  Format: {Intent} : {UnitName} [{Identifier}]

  

• MINIMAL: Used when both Identifier and Description are missing. This is the current format

  Format: {Intent} : {UnitName}

  

Validation steps performed

PR checklist

• Closes Machine Configuration Summary needs more details #873
• Tests added/passed
• Documentation updated

[betanzos]

Contributor

@microsoft-github-policy-service agree

[AmelBawa-msft]

@betanzos Thank you for creating this PR! Today the configuration output in the summary page is missing the [identifier] value to align with the output from WinGet-CLI source code, which you covered in the "MISSING DESCRIPTION" output variant 🎉 Adding PMs @denelon and @shakersMSFT to help with the proposed changes in the other variants and future plan for presenting the configuration summary results.

[denelon]

I see these as a great improvement for the output!

I know @shakersMSFT has some other ideas for future plans on further enhancements.

I'm going to go ahead and cross-link this with the Issue over at WinGet-CLI so we can make sure we're aware of any future potential for breaking changes as we work towards WinGet configuration becoming a stable feature.

• WinGet configure  winget-cli#2845

[shakersMSFT]

This looks great to me! We definitely want to make sure we are keeping parity with the WinGet CLI!

[dhoehna]

@AmelBawa-msft @shakersMSFT is there a reason why the PR is still here and not merged?

[AmelBawa-msft]

/azp run

[azure-pipelines]

No pipelines are associated with this pull request.

[EricJohnson327]

Closing and reopening to enable pipelines added since this was opened.

[florelis]

I'm not a fan of having each of the components jump around depending on the format. For example, sometimes name leads everything and sometimes it's in brackets, and the ID can be on its own or with the name.

How about this format: Intent : Name [Id] - Description? And we can build it depending on what we have. Like

summary = "Intent : Name"
if (Id) summary += " [Id]"
if (Description) summary += " - Description"

[florelis]

Do we need all of these variants to be localized? I'm a bit concerned that we may have more combinations we want to be able to handle and that's going to grow too much.

[betanzos]

I just followed the current style because I have no much context on why we are using a localized string for this, but folks, if you want I'm good with removing this strings.

[florelis]

I don't remember getting guidance specifically about this. When I've added strings like these to localization it is because I'm not sure if other locales would use the same characters.
For example, I know for quotes some languages use 「these」 and some others use „these“. Not sure if that applies in this case, particularly considering the audience here is developers.

If we're keeping your current formatting, I think it's okay to localize these. IIRC my comment was thinking of adding the components to the string one by one always in the same order, instead of the current formatting where things change places depending on which ones are available.

[florelis]

Nit: It bothers me a bit to have all the repetition of GetLocalized. I wonder if it could make sense to do something like

var resourceKey = StringResourceKey.ConfigurationUnitSummaryFull;
if (condition1) resourceKey = [another string]
else if (condition2) resourceKey = [another string]

return _stringResource.GetLocalized(resourceKey, _unitResult.Intent, _unitResult.UnitName, ...);

Though this may be abusing the GetLocalized function since it may not be intended to get more replacements than placeholders there are

[betanzos]

There is a corner case that break the simplicity of this solution: if the unit ID is missing the description should be the thrid param. Because of this the resulting code looks worse (even assuming the repetition of return _stringResource.GetLocalized(...)) to me because hide its intention:

private string BuildTitle()
{
    var missingId = string.IsNullOrEmpty(_unitResult.Id);
    var missingDescription = string.IsNullOrEmpty(_unitResult.Description);
    var summaryResourceKey = StringResourceKey.ConfigurationUnitSummaryFull;
    object[] localizedParams = { _unitResult.Intent, _unitResult.UnitName, _unitResult.Id, _unitResult.Description };

    if (missingId && missingDescription)
    {
        summaryResourceKey = StringResourceKey.ConfigurationUnitSummaryMinimal;
    }
    else if (missingId)
    {
        summaryResourceKey = StringResourceKey.ConfigurationUnitSummaryNoId;

        // for this variant of the string template the description is the thrid param
        localizedParams[2] = _unitResult.Description;
    }
    else if (missingDescription)
    {
        summaryResourceKey = StringResourceKey.ConfigurationUnitSummaryNoDescription;
    }

    return _stringResource.GetLocalized(summaryResourceKey, localizedParams);
}

[florelis]

if the unit ID is missing the description should be the thrid param

This may be a Bad Idea® but... This could probably be avoided by having that string skip the replacement {2} so it would be like {0} : {3} [{1}]
Unless something is checking that the numbers used are contiguous.

Anyways, what I'm suggesting would be adding complexity in the strings to get some simplicity here, so it may not be worth it.

[betanzos]

I'm not a fan of having each of the components jump around depending on the format. For example, sometimes name leads everything and sometimes it's in brackets, and the ID can be on its own or with the name.

How about this format: Intent : Name [Id] - Description? And we can build it depending on what we have. Like

summary = "Intent : Name"
if (Id) summary += " [Id]"
if (Description) summary += " - Description"

To me the elements of the summary string has the following precedence:

1. Description
2. Unit name
3. Unit identifier

So, I would like to see in first place those elements with the higher precendence and the rest withing the brackets. If an element is missing the next one (if any) takes it place. If there are no elements to be added to the brackets it will be hidden. And that's it.

I'm open to hear your comments but the format introduced by this PR looks better to me.

[florelis]

I'm not a fan of having each of the components jump around depending on the format. For example, sometimes name leads everything and sometimes it's in brackets, and the ID can be on its own or with the name.
How about this format: Intent : Name [Id] - Description? And we can build it depending on what we have. Like

summary = "Intent : Name"
if (Id) summary += " [Id]"
if (Description) summary += " - Description"

To me the elements of the summary string has the following precedence:

1. Description
2. Unit name
3. Unit identifier

So, I would like to see in first place those elements with the higher precendence and the rest withing the brackets. If an element is missing the next one (if any) takes it place. If there are no elements to be added to the brackets it will be hidden. And that's it.

I'm open to hear your comments but the format introduced by this PR looks better to me.

I see. This should be fine, and we can change the specific strings easily if we get any feedback.
Sorry it took me this long to re-review the changes.