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.

Sign up for GitHub

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

Jump to bottom

Improve Summary screen by adding more details about applied resources #912

Merged
merged 6 commits into from
Oct 2, 2023
Merged

Improve Summary screen by adding more details about applied resources #912

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
af531d4
Get the configuration unit Id and description
betanzos May 28, 2023
635beac
Add new string resource keys for the each summary variant
betanzos May 28, 2023
5cc4d38
Build the summary title according to the available info on the result…
betanzos May 28, 2023
29d9c81
Update constructor
betanzos May 28, 2023
120d285
refactor: Var declaration in method call
betanzos Sep 19, 2023
49bbf9d
Improves localized strings doc for configuration unit summaries
betanzos Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .../SetupFlow/DevHome.SetupFlow.ElevatedComponent/Helpers/ElevatedConfigureUnitTaskResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ public sealed class ElevatedConfigureUnitTaskResult
{
public string? UnitName { get; set; }

public string? Id { get; set; }

public string? Description { get; set; }

public string? Intent { get; set; }

public bool IsSkipped { get; set; }
Expand Down
17 changes: 12 additions & 5 deletions tools/SetupFlow/DevHome.SetupFlow.ElevatedComponent/Tasks/ElevatedConfigurationTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,19 @@ public IAsyncOperation<ElevatedConfigureTaskResult> ApplyConfiguration(StorageFi
taskResult.TaskAttempted = true;
taskResult.TaskSucceeded = result.Succeeded;
taskResult.RebootRequired = result.RequiresReboot;
taskResult.UnitResults = result.Result.UnitResults.Select(unitResult => new ElevatedConfigureUnitTaskResult
taskResult.UnitResults = result.Result.UnitResults.Select(unitResult =>
{
UnitName = unitResult.Unit.UnitName,
Intent = unitResult.Unit.Intent.ToString(),
IsSkipped = unitResult.State == ConfigurationUnitState.Skipped,
HResult = unitResult.ResultInformation?.ResultCode?.HResult ?? HRESULT.S_OK,
object descriptionObj;
unitResult.Unit.Directives.TryGetValue("description", out descriptionObj);
betanzos marked this conversation as resolved.
Show resolved Hide resolved
florelis marked this conversation as resolved.
Show resolved Hide resolved
return new ElevatedConfigureUnitTaskResult
{
UnitName = unitResult.Unit.UnitName,
Id = unitResult.Unit.Identifier,
Description = descriptionObj?.ToString() ?? string.Empty,
Intent = unitResult.Unit.Intent.ToString(),
IsSkipped = unitResult.State == ConfigurationUnitState.Skipped,
HResult = unitResult.ResultInformation?.ResultCode?.HResult ?? HRESULT.S_OK,
};
}).ToList();

if (result.ResultException != null)
Expand Down
10 changes: 10 additions & 0 deletions tools/SetupFlow/DevHome.SetupFlow/Models/ConfigurationUnitResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ public class ConfigurationUnitResult
public ConfigurationUnitResult(ApplyConfigurationUnitResult result)
{
UnitName = result.Unit.UnitName;
Id = result.Unit.Identifier;
object descriptionObj;
result.Unit.Directives.TryGetValue("description", out descriptionObj);
betanzos marked this conversation as resolved.
Show resolved Hide resolved
Description = descriptionObj?.ToString() ?? string.Empty;
Intent = result.Unit.Intent.ToString();
IsSkipped = result.State == ConfigurationUnitState.Skipped;
HResult = result.ResultInformation?.ResultCode?.HResult ?? HRESULT.S_OK;
Expand All @@ -21,13 +25,19 @@ public ConfigurationUnitResult(ApplyConfigurationUnitResult result)
public ConfigurationUnitResult(ElevatedConfigureUnitTaskResult result)
{
UnitName = result.UnitName;
Id = result.Id;
Description = result.Description;
Intent = result.Intent;
IsSkipped = result.IsSkipped;
HResult = result.HResult;
}

public string UnitName { get; }

public string Id { get; }

public string Description { get; }

public string Intent { get; }

public bool IsSkipped { get; }
Expand Down
5 changes: 4 additions & 1 deletion tools/SetupFlow/DevHome.SetupFlow/Services/StringResourceKey.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ public static class StringResourceKey
public static readonly string ConfigurationUnitFailed = nameof(ConfigurationUnitFailed);
public static readonly string ConfigurationUnitSkipped = nameof(ConfigurationUnitSkipped);
public static readonly string ConfigurationUnitSuccess = nameof(ConfigurationUnitSuccess);
public static readonly string ConfigurationUnitSummary = nameof(ConfigurationUnitSummary);
public static readonly string ConfigurationUnitSummaryFull = nameof(ConfigurationUnitSummaryFull);
public static readonly string ConfigurationUnitSummaryMinimal = nameof(ConfigurationUnitSummaryMinimal);
public static readonly string ConfigurationUnitSummaryNoDescription = nameof(ConfigurationUnitSummaryNoDescription);
public static readonly string ConfigurationUnitSummaryNoId = nameof(ConfigurationUnitSummaryNoId);
public static readonly string ConfigurationUnitStats = nameof(ConfigurationUnitStats);
public static readonly string ConfigurationViewTitle = nameof(ConfigurationViewTitle);
public static readonly string DevDriveReviewTitle = nameof(DevDriveReviewTitle);
Expand Down
14 changes: 13 additions & 1 deletion tools/SetupFlow/DevHome.SetupFlow/Strings/en-us/Resources.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,10 +221,22 @@
<value>Configuration successfully applied</value>
<comment>Message displayed when applying a configuration unit succeeds.</comment>
</data>
<data name="ConfigurationUnitSummary" xml:space="preserve">
<data name="ConfigurationUnitSummaryFull" xml:space="preserve">
<value>{0} : {3} [{1} {2}]</value>
<comment>{Locked="{0}","{1}","{2}","{3}"}Summary text for a configuration unit. {0} is replaced by the configuration unit intent (e.g. Assert, Inform, Apply). {1} is replaced by the configuration unit name. {2} is replaced by the configuration unit id. {3} is replaced by the configuration unit description.</comment>
betanzos marked this conversation as resolved.
Show resolved Hide resolved
</data>
<data name="ConfigurationUnitSummaryMinimal" xml:space="preserve">
<value>{0} : {1}</value>
<comment>{Locked="{0}","{1}"}Summary text for a configuration unit. {0} is replaced by the configuration unit intent (e.g. Assert, Inform, Apply). {1} is replaced by the configuration unit name.</comment>
</data>
<data name="ConfigurationUnitSummaryNoDescription" xml:space="preserve">
<value>{0} : {1} [{2}]</value>
Comment on lines +232 to +233
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<comment>{Locked="{0}","{1}","{2}"}Summary text for a configuration unit. {0} is replaced by the configuration unit intent (e.g. Assert, Inform, Apply). {1} is replaced by the configuration unit name. {2} is replaced by the configuration unit id.</comment>
</data>
<data name="ConfigurationUnitSummaryNoId" xml:space="preserve">
<value>{0} : {2} [{1}]</value>
<comment>{Locked="{0}","{1}","{2}"}Summary text for a configuration unit. {0} is replaced by the configuration unit intent (e.g. Assert, Inform, Apply). {1} is replaced by the configuration unit name. {2} is replaced by the configuration unit description.</comment>
</data>
<data name="ConfigurationUnitStats" xml:space="preserve">
<value>Succeeded: {0} | Failed: {1} | Skipped: {2}</value>
<comment>{Locked="{0}","{1}","{2}"}Text showing the number of configuration units that succeeded, failed and skipped. {0}, {1} and {2} are replaced with numeric values.</comment>
Expand Down
22 changes: 21 additions & 1 deletion tools/SetupFlow/DevHome.SetupFlow/ViewModels/ConfigurationUnitResultViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public ConfigurationUnitResultViewModel(ISetupFlowStringResource stringResource,
_unitResult = unitResult;
}

public string Title => _stringResource.GetLocalized(StringResourceKey.ConfigurationUnitSummary, _unitResult.Intent, _unitResult.UnitName);
public string Title => BuildTitle();

public string ApplyResult => GetApplyResult();

Expand All @@ -54,4 +54,24 @@ private string GetApplyResult()

return _stringResource.GetLocalized(StringResourceKey.ConfigurationUnitSuccess);
}

private string BuildTitle()
{
if (string.IsNullOrEmpty(_unitResult.Id) && string.IsNullOrEmpty(_unitResult.Description))
{
return _stringResource.GetLocalized(StringResourceKey.ConfigurationUnitSummaryMinimal, _unitResult.Intent, _unitResult.UnitName);
}

if (string.IsNullOrEmpty(_unitResult.Id))
{
return _stringResource.GetLocalized(StringResourceKey.ConfigurationUnitSummaryNoId, _unitResult.Intent, _unitResult.UnitName, _unitResult.Description);
}

if (string.IsNullOrEmpty(_unitResult.Description))
{
return _stringResource.GetLocalized(StringResourceKey.ConfigurationUnitSummaryNoDescription, _unitResult.Intent, _unitResult.UnitName, _unitResult.Id);
}

return _stringResource.GetLocalized(StringResourceKey.ConfigurationUnitSummaryFull, _unitResult.Intent, _unitResult.UnitName, _unitResult.Id, _unitResult.Description);
Comment on lines +60 to +75
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}