Skip to content

Conversation

@hamza-usmani
Copy link
Collaborator

@hamza-usmani hamza-usmani commented Nov 1, 2025

Experimental GUI - this is something we want the community's feedback on (the UI-based approach) and is highly experimental in the functionality/code!

@hamza-usmani hamza-usmani requested a review from azchohfi November 1, 2025 07:39
Copy link

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Bit hard to follow the app logic code, definitely room for some revisions to better separate logical concerns. Provided some suggestions here to get things started. Excited to see how folks use these tools!

xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
mc:Ignorable="d"
Title="Windows Identity App">

Choose a reason for hiding this comment

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

Could be good to make sure to Mica backdrop and update Titlebar etc... @niels9001 any other guidance/suggestions?

</StackPanel>

<!-- Pivot Control for App Type -->
<Pivot x:Name="AppTypePivot" Margin="0,0,0,8" SelectionChanged="AppTypePivot_SelectionChanged">

Choose a reason for hiding this comment

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

Pivot is sort of out-dated. Could be more organized to make separate pages for each mode anyway (plus not great to have all the code in MainWindow anyway). So, could update to a SelectorBar or other navigation control controlling a frame or content element (I hear SwitchPresenter from the Toolkit is nice, but I'm biased... 😅).


<!-- Python PivotItem -->
<PivotItem Header="Python">
<StackPanel>

Choose a reason for hiding this comment

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

You can use Spacing on panels to minimize custom margin use

<StackPanel Orientation="Horizontal" Spacing="8" HorizontalAlignment="Center" Margin="0,4,0,0">
<TextBlock x:Name="FileNameText" FontSize="16" FontWeight="SemiBold" VerticalAlignment="Center"/>
<Button x:Name="CancelButton" Width="32" Height="32" Padding="0" FontSize="20" VerticalAlignment="Center" HorizontalAlignment="Center" Background="Transparent" BorderThickness="0" ToolTipService.ToolTip="Remove file">
<TextBlock FontFamily="Segoe MDL2 Assets" Text="&#xE10A;" VerticalAlignment="Center" HorizontalAlignment="Center"/>

Choose a reason for hiding this comment

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

Wrong fontfamily, should use the resource SymbolThemeFontFamily.


private void BindExeName()
{
DotNetFileNamePanel.Visibility = SelectedExeName != null ? Visibility.Visible : Visibility.Collapsed;

Choose a reason for hiding this comment

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

Please use binding for all these over code-behind, will make things a lot easier, I promise!

<ScrollViewer x:Name="ProgressScrollViewer" VerticalScrollBarVisibility="Auto">
<ListView x:Name="ProgressItemsControl">
<ListView.ItemTemplate>
<DataTemplate>

Choose a reason for hiding this comment

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

Make sure to give data type and use x:Bind

Comment on lines +64 to +66
DotNetProgressPanel.ProgressCards = progressCards;
PythonProgressPanel.ProgressCards = progressCards;
MsixProgressPanel.ProgressCards = progressCards;

Choose a reason for hiding this comment

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

This progresscard pattern was very hard to follow. I think breaking things out into separate pages and trying to better encapsulate the data with MVVM principles and binding would go a long way to make things more readable/maintainable.

@hamza-usmani hamza-usmani requested a review from nmetulev November 3, 2025 18:17
@nmetulev nmetulev enabled auto-merge November 4, 2025 18:19
@nmetulev nmetulev merged commit ea0cc48 into main Nov 4, 2025
7 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.

4 participants