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.

Sign up for GitHub

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 popup with choice of Accept or Won't Fix #5046

Merged
merged 6 commits into from
Nov 24, 2023
Merged

Add popup with choice of Accept or Won't Fix #5046

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
65bd3ed
added window
ugras-ergun-sonarsource Nov 24, 2023
cb43f7d
PR feedback
ugras-ergun-sonarsource Nov 24, 2023
d8f6200
exclude tests
ugras-ergun-sonarsource Nov 24, 2023
07bbfb4
PR feedback
ugras-ergun-sonarsource Nov 24, 2023
fe6ad37
pr feedback
ugras-ergun-sonarsource Nov 24, 2023
4e25fd8
moved styles
ugras-ergun-sonarsource Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 42 additions & 18 deletions src/Integration/Transition/MuteWindowDialog.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,39 +9,64 @@
mc:Ignorable="d"
SizeToContent="WidthAndHeight"
Title="{x:Static resx:Strings.MuteWindow_Title}">
<vsui:DialogWindow.Resources>
<Style x:Key="RadioButtonStyle" TargetType="RadioButton">
<Setter Property="GroupName" Value="Transitions" />
<Setter Property="VerticalAlignment" Value="Top" />
<Setter Property="Margin" Value="10,16,-10,0" />
<Setter Property="FontWeight" Value="Bold" />
</Style>
<Style x:Key="BorderStyle" TargetType="Border">
<Setter Property="BorderBrush" Value="Black" />
<Setter Property="BorderThickness" Value="1" />
<Setter Property="HorizontalAlignment" Value="Left" />
<Setter Property="Height" Value="75" />
<Setter Property="VerticalAlignment" Value="Top" />
<Setter Property="Width" Value="450" />
</Style>
<Style x:Key="LabelStyle" TargetType="Label">
<Setter Property="HorizontalAlignment" Value="Left" />
<Setter Property="VerticalAlignment" Value="Top" />
</Style>
<Style x:Key="TransitionLabelStyle" BasedOn="{StaticResource LabelStyle}" TargetType="Label">
<Setter Property="Margin" Value="10,36,0,0" />
</Style>
<Style x:Key="ButtonStyle" TargetType="Button">
<Setter Property="Padding" Value="5,1"/>
<Setter Property="HorizontalAlignment" Value="Left"/>
<Setter Property="VerticalAlignment" Value="Top"/>
</Style>
</vsui:DialogWindow.Resources>
<Grid Height="350" Width="500">
<Border BorderBrush="Black" Name="BorderWontFix" BorderThickness="1" HorizontalAlignment="Left" Height="75" Margin="25,25,0,0" VerticalAlignment="Top" Width="450" Visibility="{Binding WontFixVisibility}">
<Border Name="BorderWontFix" Style="{StaticResource BorderStyle}" Margin="25,25,0,0" Visibility="{Binding WontFixVisibility}">
<Grid>
<RadioButton Name="rbWontFix" GroupName="Transitions" Content="{x:Static resx:Strings.MuteWindow_WontFixTitle}" VerticalAlignment="Top" Margin="10,16,-10,0" FontWeight="Bold" Grid.ColumnSpan="2" Checked="RadioButton_Checked"/>
<Label Content="{x:Static resx:Strings.MuteWindow_WontFixContent}" HorizontalAlignment="Left" Margin="10,36,0,0" VerticalAlignment="Top" Grid.ColumnSpan="2"/>
<RadioButton Name="rbWontFix" Content="{x:Static resx:Strings.MuteWindow_WontFixTitle}" Style="{StaticResource RadioButtonStyle}" Checked="RadioButton_Checked"/>
<Label Content="{x:Static resx:Strings.MuteWindow_WontFixContent}" Style="{StaticResource TransitionLabelStyle}" />
</Grid>
</Border>

<Border BorderBrush="Black" Name="BorderAccept" BorderThickness="1" HorizontalAlignment="Left" Height="75" Margin="25,25,0,0" VerticalAlignment="Top" Width="450" Visibility="{Binding AcceptVisibility}">
<Border Name="BorderAccept" Style="{StaticResource BorderStyle}" Margin="25,25,0,0" Visibility="{Binding AcceptVisibility}">
Copy link
Contributor

Choose a reason for hiding this comment

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

This margin value is used twice. There's only one BorderStyle instance with a different Mragin value. You can set this one as the default, and override it once at line 75

<Grid>
<RadioButton Name="rbAccept" GroupName="Transitions" Content="{x:Static resx:Strings.MuteWindow_AcceptTitle}" VerticalAlignment="Top" Margin="10,16,-10,0" FontWeight="Bold" Grid.ColumnSpan="2" Checked="RadioButton_Checked"/>
<Label Content="{x:Static resx:Strings.MuteWindow_AcceptContent}" HorizontalAlignment="Left" Margin="10,36,0,0" VerticalAlignment="Top" Grid.ColumnSpan="2"/>
<RadioButton Name="rbAccept" Content="{x:Static resx:Strings.MuteWindow_AcceptTitle}" Style="{StaticResource RadioButtonStyle}" Checked="RadioButton_Checked"/>
<Label Content="{x:Static resx:Strings.MuteWindow_AcceptContent}" Style="{StaticResource TransitionLabelStyle}" />
</Grid>
</Border>

<Border BorderBrush="Black" BorderThickness="1" HorizontalAlignment="Left" Height="75" Margin="25
,115,0,0" VerticalAlignment="Top" Width="450">
<Border Style="{StaticResource BorderStyle}" Margin="25,115,0,0">
<Grid>
<RadioButton x:Name="rbFalsePositive" GroupName="Transitions" Content="{x:Static resx:Strings.MuteWindow_FalsePositiveTitle}" VerticalAlignment="Top" Margin="10,16,-10,0" FontWeight="Bold" Grid.ColumnSpan="2" Checked="RadioButton_Checked"/>
<Label Content="{x:Static resx:Strings.MuteWindow_FalsePositiveContent}" HorizontalAlignment="Left" Margin="10,36,0,0" VerticalAlignment="Top" Grid.ColumnSpan="2"/>
<RadioButton x:Name="rbFalsePositive" Content="{x:Static resx:Strings.MuteWindow_FalsePositiveTitle}" Style="{StaticResource RadioButtonStyle}" Checked="RadioButton_Checked"/>
<Label Content="{x:Static resx:Strings.MuteWindow_FalsePositiveContent}" Style="{StaticResource TransitionLabelStyle}" />
</Grid>
</Border>
<Label Content="{x:Static resx:Strings.MuteWindow_CommentLabel}" HorizontalAlignment="Left" Margin="25,200,0,0" VerticalAlignment="Top"/>
<Label Content="{x:Static resx:Strings.MuteWindow_CommentLabel}" Style="{StaticResource LabelStyle}" Margin="25,200,0,0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

LabelStyle is only used once, margin can be moved there

<TextBox Name="txtComment" HorizontalAlignment="Center" Height="60" Margin="0,225,0,0" TextWrapping="Wrap" VerticalAlignment="Top" Width="450"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Styles in the layout

<Button Name="Cancel" Content="{x:Static resx:Strings.MuteWindow_CancelButton}" Padding="5,1" HorizontalAlignment="Left" Margin="300,300,0,0" VerticalAlignment="Top" Click="Cancel_Click"/>
<Button Name="Submit" Content="{x:Static resx:Strings.MuteWindow_SubmitButton}" Padding="5,1" HorizontalAlignment="Left" Margin="360,300,0,0" VerticalAlignment="Top" Click="Submit_Click">
<Button Name="Cancel" Content="{x:Static resx:Strings.MuteWindow_CancelButton}" Style="{StaticResource ButtonStyle}" Margin="300,300,0,0" Click="Cancel_Click"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these margins not part of the style definition? Btw you can also inherit styles and override certain properties in style definitions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

margins are different in each component

Copy link
Contributor

Choose a reason for hiding this comment

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

You can define style as (I don't remember wpf syntax, so I will explain this with C# objects :-) )

style AcceptButton : GenericButton
{
override Margin Margin = "1, 2, 3, 4"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting a property in a common place for only one component does not make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does. You separate styling from layout. Easier to read, easier to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a property and overriding it later create more noise IMO. Another solution would be creating a common style and child styles for each specific component based on the common one. Which would be an overkill in this situation just makes reading code harder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but now you have styling split between the style resources and the layout elements, which is IMO more confusing and harder to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussing this takes more time than changing and I do not feel strongly about it so done.

<Button Name="Submit" Content="{x:Static resx:Strings.MuteWindow_SubmitButton}" Click="Submit_Click">
<Button.Style>
<Style TargetType="Button">

<Style TargetType="Button" BasedOn="{StaticResource ButtonStyle}">
<Setter Property="Margin" Value="360,300,0,0" />
<Setter Property="IsEnabled" Value="False"/>

<Style.Triggers>

<DataTrigger Binding="{Binding Path=IsChecked, ElementName=rbWontFix}" Value="True">
<Setter Property="IsEnabled" Value="True"/>
</DataTrigger>
Expand All @@ -53,7 +78,6 @@
<DataTrigger Binding="{Binding Path=IsChecked, ElementName=rbFalsePositive}" Value="True">
<Setter Property="IsEnabled" Value="True"/>
</DataTrigger>

</Style.Triggers>
</Style>
</Button.Style>
Expand Down
33 changes: 18 additions & 15 deletions src/Integration/Transition/MuteWindowDialog.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Markup;
using Microsoft.VisualStudio.PlatformUI;
using SonarQube.Client.Models;
Expand All @@ -33,11 +35,25 @@ namespace SonarLint.VisualStudio.Integration.Transition
[ExcludeFromCodeCoverage]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

UI Code not testable

public partial class MuteWindowDialog : DialogWindow
{
private readonly Dictionary<RadioButton, SonarQubeIssueTransition> Transitions;

public MuteWindowDialog(bool showAccept)
{
InitializeComponent();

SetVisibility(showAccept);

Transitions = InitializeTransitions();
}

private Dictionary<RadioButton, SonarQubeIssueTransition> InitializeTransitions()
{
return new Dictionary<RadioButton, SonarQubeIssueTransition>
{
{ rbWontFix, SonarQubeIssueTransition.WontFix },
{ rbAccept, SonarQubeIssueTransition.Accept },
{ rbFalsePositive, SonarQubeIssueTransition.FalsePositive }
};
}

private void SetVisibility(bool showAccept)
Expand All @@ -56,26 +72,13 @@ private void Submit_Click(object sender, RoutedEventArgs e)
this.DialogResult = true;
}

public SonarQubeIssueTransition SelectedIssueTransition { get; private set; }
public SonarQubeIssueTransition? SelectedIssueTransition { get; private set; }

public string Comment => txtComment.Text;

private void RadioButton_Checked(object sender, RoutedEventArgs e)
{
switch (sender)
{
case var value when value == rbWontFix:
SelectedIssueTransition = SonarQubeIssueTransition.WontFix;
break;

case var value when value == rbAccept:
SelectedIssueTransition = SonarQubeIssueTransition.Accept;
break;

case var value when value == rbFalsePositive:
SelectedIssueTransition = SonarQubeIssueTransition.FalsePositive;
break;
}
SelectedIssueTransition = Transitions[(RadioButton)sender];
}
}
}