Add to SystemInformation class #1533
Conversation
For CommunityToolkit#1505 Porting of some logic from WTS + extra useful app usage and analytical information Adds Properties: IsFirstRun; IsAppUpdated; FirstVersionInstalled; FirstUseTime; LastLaunchTime; LaunchTime; LaunchCount; AppUptime Adds Methods: TrackAppUse; LaunchStoreForReviewAsync
| } | ||
| } | ||
|
|
||
| private static DateTime _sessionStart; |
There was a problem hiding this comment.
Need to move it to the top
There was a problem hiding this comment.
Thanks. This one is necessary
| } | ||
| else | ||
| { | ||
| return TimeSpan.MinValue; |
There was a problem hiding this comment.
We could just use TimeSpan.Zero though MinValue works too
There was a problem hiding this comment.
Zero has the possibility that it might be valid if checked as soon as the app starts. MinValue will never be valid and so can always be used as a comparison and indicates the value has not been set without the overhead of considering nullability.
| /// <param name="args">Details about the launch request and process.</param> | ||
| public static void TrackAppUse(LaunchActivatedEventArgs args) | ||
| { | ||
| if (new[] { ApplicationExecutionState.ClosedByUser, ApplicationExecutionState.NotRunning }.Contains(args.PreviousExecutionState)) |
There was a problem hiding this comment.
Wouldn't it be simpler to just use a standard if statement rather than creating an array to only do contains ?
There was a problem hiding this comment.
I prefer this as it's shorter than if (args.PreviousExecutionState == ApplicationExecutionState.ClosedByUser || args.PreviousExecutionState == ApplicationExecutionState.NotRunning) as everything is only mentioned once. I also use it as a general approach as it makes it easier to add additional comparisons against the same value in the future. Shouldn't be relevant here, but just trying to explain my reasoning.
Simpler is subjective. Must this be changed to be accepted?
There was a problem hiding this comment.
If I felt sufficiently strong about these then I would have requested changes. Thoughts just based on a quick glance. I will let others go through this.
There was a problem hiding this comment.
I think Hermit is right, creating new array will consume some memory that we don't need :) I know an array of 2 doesn't add much to memory but it is just a practice.
There was a problem hiding this comment.
Changed this to better meet expectations of how it would normally be done.
| public static string FirstVersionInstalled { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the DateTime (in UTC) that the app as first used. |
There was a problem hiding this comment.
This wording feels confusing. How about
Gets the DateTime (in UTC) when the app was launched for the first time
|
|
||
| // To get if the app is being used for the first time since it was installed. | ||
| public string IsFirstUse => SystemInformation.IsFirstRun.ToString(); |
There was a problem hiding this comment.
I don't think we need the ToString on all the properties in the bind file. It hides the return type of the property/function.
| /// Gets the first version of the app that was installed. | ||
| /// This will be the current version if a previous verison of the app was installed before accessing this property. | ||
| /// </summary> | ||
| public static string FirstVersionInstalled { get; } |
There was a problem hiding this comment.
Should this match the ApplicationVersion return type of PackageVersion?
There was a problem hiding this comment.
Changed but had to add a PackageVersionHelper as PackageVersion isn't serializable.
Should the methods in this new helper class have extra specific error handling? or happy to just let any formatting errors bubble up from usage?
There was a problem hiding this comment.
I'm ok with the error bubbling up as you already have the format in the param description
| public static DateTime FirstUseTime { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the DateTime (in UTC) that this was previously launched. |
There was a problem hiding this comment.
I'd reword this to
Gets the DateTime (in UTC) when the app was previously launched, not including this instance
…irstRunOrUpdatedHelper
…irstRunOrUpdatedHelper
For #1505
Porting of some logic from WTS + extra useful app usage and analytical information
Adds Properties: IsFirstRun; IsAppUpdated; FirstVersionInstalled; FirstUseTime; LastLaunchTime; LaunchTime; LaunchCount; AppUptime
Adds Methods: TrackAppUse(); LaunchStoreForReviewAsync()