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

Showing exceptions in the loading screen. #2927

Merged
merged 11 commits into from
May 17, 2024

Conversation

dhoehna
Copy link
Contributor

@dhoehna dhoehna commented May 15, 2024

Please do not merge this PR until 05/17

Summary of the pull request

Caught exceptions in the loading screen when running a task, and un-caught exceptions in a task did not crash DevHome. But also DevHome did not notify the user of the failed task.

This PR adds this feature. :) These are reported to the user in the Loading Screen and are treated as a failed task.

References and relevant issues

Detailed description of the pull request / Additional comments

The logic to figure what message to show based on internal information is tightly coupled resulting in some duplicate code.

This PR contains the first steps in de-coupling internal information and the loading screen message.

The logic to keep executing messages at the bottom of the list was fragile. I upgraded this to use two ListViews. One for finished messages, and one for executing messages. This resulted in

  1. Less code to write because two properties were removed.
  2. Easier code to keep executing messages at the bottom of the loading screen.

Validation steps performed

Used Random to cause a crash in the loading screen and the clone task exception.
Specified argument was out of the range of valid values (window) is a crash in the code that runs a task.
`Value cannot be null (parameter 'Blah') is an un-caught crash inside CloneRepoTask.

Please ignore the "Can't clone because the location is not empty" errors when retying. This is because the exception in CloneRepoTask is thrown after the repo cloned.

PR checklist

Movie!
https://github.com/microsoft/devhome/assets/2517139/14b9a23e-e4d1-43a0-8d2d-9502fc67d71a

@dhoehna dhoehna requested review from dkbennett, sshilov7, bbonaby and a team May 15, 2024 18:18
@krschau krschau added the Do-Not-Merge Don't merge this in yet label May 15, 2024
@dhoehna dhoehna removed the Do-Not-Merge Don't merge this in yet label May 17, 2024
@dhoehna dhoehna merged commit 70595ee into main May 17, 2024
4 checks passed
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.

Loading screen UI if a tasks exception isn't caught inside the task
4 participants