Skip to content

Commit

Permalink
Fix Loading on pages (microsoft#290)
Browse files Browse the repository at this point in the history
This subtly regressed in microsoft#244, so subtly that I didn't really notice it till I fixed it.

In that PR, we replaced the `visibility` binding of the `ProgressBar` with a binding to `IsInitialized`. For most pages, this actually just works fine - they're only initialized once they return some results. And most pages are only loading until they first return something.  

But the trick is with pages that do some `Loading` _after_ they're initialized. For them, the progress bar never shows up, because `Initialized` is already true.

I actually only stumbled upon this because of a totally different bug (which this also fixes). 

In rare cases we'd load a little out of order, and the ShellPage would try to determine if it should hide the `ErrorMessage` or not. About 25% of the time, it would evaluate `{x:Bind ViewModel.CurrentPage.ErrorMessage, Converter={StaticResource StringNotEmptyToVisibilityConverter}, Mode=OneWay}` _before_ the `CurrentPage` got set, so it would show it initially. That looked real janky.
  • Loading branch information
zadjii-msft authored Jan 9, 2025
1 parent 0bd356c commit 4b9b15f
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 7 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright (c) Microsoft Corporation
// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CmdPal.Extensions;

namespace Microsoft.CmdPal.UI.ViewModels;

public partial class LoadingPageViewModel : PageViewModel
{
public LoadingPageViewModel(IPage? model, TaskScheduler scheduler)
: base(model, scheduler)
{
ModelIsLoading = true;
IsInitialized = false;
}
}
13 changes: 10 additions & 3 deletions src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/PageViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ public partial class PageViewModel : ExtensionObjectViewModel, IPageContext

private readonly ExtensionObject<IPage> _pageModel;

public bool IsLoading => ModelIsLoading || (!IsInitialized);

[ObservableProperty]
[NotifyPropertyChangedFor(nameof(IsLoading))]
public partial bool IsInitialized { get; protected set; }

[ObservableProperty]
Expand All @@ -38,7 +41,9 @@ public partial class PageViewModel : ExtensionObjectViewModel, IPageContext

public string Title { get => string.IsNullOrEmpty(field) ? Name : field; protected set; } = string.Empty;

public bool IsLoading { get; protected set; } = true;
// This property maps to `IPage.IsLoading`, but we want to expose our own
// `IsLoading` property as a combo of this value and `IsInitialized`
public bool ModelIsLoading { get; protected set; } = true;

public IconDataType Icon { get; protected set; } = new(string.Empty);

Expand Down Expand Up @@ -91,13 +96,14 @@ public override void InitializeProperties()
}

Name = page.Name;
IsLoading = page.IsLoading;
ModelIsLoading = page.IsLoading;
Title = page.Title;
Icon = page.Icon;

// Let the UI know about our initial properties too.
UpdateProperty(nameof(Name));
UpdateProperty(nameof(Title));
UpdateProperty(nameof(ModelIsLoading));
UpdateProperty(nameof(IsLoading));
UpdateProperty(nameof(Icon));

Expand Down Expand Up @@ -143,7 +149,8 @@ protected virtual void FetchProperty(string propertyName)
this.Title = model.Title ?? string.Empty;
break;
case nameof(IsLoading):
this.IsLoading = model.IsLoading;
this.ModelIsLoading = model.IsLoading;
UpdateProperty(nameof(ModelIsLoading));
break;
case nameof(Icon):
this.Icon = model.Icon;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public SettingsViewModel(SettingsModel settings, IServiceProvider serviceProvide

Icon = new("\uE713");
IsInitialized = true;
IsLoading = false;
ModelIsLoading = false;
Title = "Settings";

var activeProviders = GetCommandProviders();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public partial class ShellViewModel(IServiceProvider _serviceProvider, TaskSched
public partial bool IsDetailsVisible { get; set; }

[ObservableProperty]
public partial PageViewModel? CurrentPage { get; set; }
public partial PageViewModel CurrentPage { get; set; } = new LoadingPageViewModel(null, _scheduler);

[RelayCommand]
public async Task<bool> LoadAsync()
Expand Down Expand Up @@ -98,7 +98,7 @@ public void LoadPageViewModel(PageViewModel viewModel)
{
var result = (bool)viewModel.InitializeCommand.ExecutionTask.GetResultOrDefault()!;

CurrentPage = result ? viewModel : null;
CurrentPage = viewModel; // result ? viewModel : null;
////LoadedState = result ? ViewModelLoadedState.Loaded : ViewModelLoadedState.Error;
},
CancellationToken.None,
Expand Down
2 changes: 1 addition & 1 deletion src/modules/cmdpal/Microsoft.CmdPal.UI/ShellPage.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@
Grid.ColumnSpan="2"
VerticalAlignment="Bottom"
IsIndeterminate="True"
Visibility="{x:Bind ViewModel.CurrentPage.IsInitialized, Converter={StaticResource BoolToInvertedVisibilityConverter}, Mode=OneWay}" />
Visibility="{x:Bind ViewModel.CurrentPage.IsLoading, Mode=OneWay}" />

<Grid
x:Name="ContentGrid"
Expand Down

0 comments on commit 4b9b15f

Please sign in to comment.