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

Add a few more fixes for environments #2892

Merged
merged 3 commits into from
May 14, 2024
Merged

Conversation

bbonaby
Copy link
Contributor

@bbonaby bbonaby commented May 13, 2024

Summary of the pull request

After testing environments more for any random crashes or things that may have been missed before release I found the following:

  1. If you repeatedly perform operations on multiple environments, there can be a random COM exception due to using the "Clear()" method in the observable collection that we hold properties in here: this can happen randomly and upon looking online there are people who have ran into the same issue when using the clear method. Found an issue related to this clearing observable collection bound to ListView crashes with ArgumentOutOfRangeException microsoft-ui-xaml#8684. To work around this, I found that removing the properties from the end of the collection to the beginning throws no exception no matter how many operations I attempt. Interestingly I found other places online like in WPF that also have issues with clearing directly: https://stackoverflow.com/questions/47347273/clear-observablecollection-throws-exception. So I updated this in the two place we use it.
  2. Updated the logging context in ComputeSystemHelpers so its name shows up in the logs.
  3. Noticed that the operation names that appear in the menu flyouts in the environments pages were localized but we werent using them from the resource file. Updated the DataExtractor to use them
  4. Noticed we were missing the Save operation in the DataExtractor so I added it in
  5. Noticed in the Set up environments flow we were showing "Version: Unknown" when the user gets to the review page for environments where we don't know the operating system. For these we shouldn't show any version. We should only show the version if we know it. So updated the SetupTargetReviewView to only show the version when an operating system property from the environment exists
  6. Noticed we still showed environments that were being created and deleted in the set up environments page so added a method to check for these so we don't show them as they won't be in a configurable state.

Video of me performing multiple operations with no crashes in environments page, also shows save button:

Showing.multiple.operations.being.initiated.without.crashing.mp4

Showing version not showing up when the environment does not have the operating system as a property:

Showing.version.not.showing.up.when.the.environment.does.not.have.the.operating.system.as.a.property.mp4

Showing me creating a Dev Box which shows up in the environments page and now does not show up in set up environments page:

Showing.me.creating.a.Dev.Box.which.shows.up.in.the.environments.page.and.now.does.not.show.up.in.set.up.environments.page.mp4

References and relevant issues

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

@krschau krschau added this to the Dev Home v0.14 milestone May 13, 2024
@@ -83,24 +84,16 @@
Grid.Column="2"
Margin="0 0 5 0"
x:Uid="SetupTargetReviewPageComputeSystemVersion"
Visibility="{x:Bind ViewModel.OsName, Mode=OneWay, Converter={StaticResource EmptyObjectToObjectConverter}}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I could definitely be misremembering, but have you tested this with both scenarios? I have a vague memory of trying to do something like this and realizing that Collapsed still resolved to "true" or something like that.

Copy link
Contributor Author

@bbonaby bbonaby May 13, 2024

Choose a reason for hiding this comment

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

Hey yes, I tested both scenarios. The key here is making the OsName = null in the SetOsNameAsync method here. Only when the object is null will the UI element be collasped. In the second video at timestamp 0:13 seconds that shows what happens when OsName is null. The version text box and the string next to it does not appear. Then in timestamp 0:27 seconds is what happens when OsName is not null

collection.RemoveAt(i);
}
}
catch (Exception ex)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we get an exception, would we still want to try to clear the rest of the collection, or is it not worth it?

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 believe it's not worth it, as it may result in another exception

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to figure out the root cause of the issue. May be open a lower priority bug. Then we could understand what's the best workaround is. For example, if it's caused by a race then using a lock around Clear() could be a better solution.
One side effect of removing items one by one could be sending too many/often notifications to the subscribers, which can affect UX responsiveness or cause some issues within the subscribers if they didn't expect that.

<value>Stop</value>
<comment>Text for the stop operation shown for the environment.</comment>
<data name="Operations_Shutdown" xml:space="preserve">
<value>Shutdown</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't "Stop" the value that the designers wanted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually they decided to change it #2665 to match hyper-v

Copy link
Contributor

Choose a reason for hiding this comment

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

So don't you want to link and close the issue with this PR?

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.

Map strings for actions for Hyper-V to align with Hyper-V manager
4 participants