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

ugras-ergun-sonarsource · 2023-11-24T09:53:39Z

ugras-ergun-sonarsource · 2023-11-24T09:55:11Z

Functional logic will be added in another ticket.

georgii-borovinskikh-sonarsource · 2023-11-24T09:58:50Z

src/Integration/Integration.csproj

@@ -59,11 +59,13 @@
    <None Remove="Connection\UI\OrganizationSelectionWindow.xaml" />
    <None Remove="Notifications\NotificationIndicator.xaml" />
    <None Remove="Progress\ProgressControl.xaml" />
+    <None Remove="Muting\MuteWindowDialog.xaml" />


I don't think we should use the term Muting. Transition/Change status should be more appropriate

georgii-borovinskikh-sonarsource · 2023-11-24T10:00:52Z

src/Integration/Muting/MuteWindowDialog.xaml

@@ -0,0 +1,60 @@
+<vsui:DialogWindow  x:Class="SonarLint.VisualStudio.Integration.Muting.MuteWindowDialog"


A screenshot would be nice to attach

georgii-borovinskikh-sonarsource · 2023-11-24T10:01:58Z

src/Integration/Muting/MuteWindowDialog.xaml

+
+        <Border BorderBrush="Black" Name="BorderAccept" BorderThickness="1" HorizontalAlignment="Left" Height="75" Margin="25,25,0,0" VerticalAlignment="Top" Width="450" Visibility="{Binding AcceptVisibility}">
+            <Grid>
+                <RadioButton Name="rbAccept" GroupName="Transitions" Content="Accept" VerticalAlignment="Top" Margin="10,16,-10,0" FontWeight="Bold" Grid.ColumnSpan="2" Checked="RadioButton_Checked"/>


Changing the text of the button, instead of creating hidden buttons is less confusing and more maintainable.

I disagree, I believe this way it's more obvious what's happening. We create a logical separation of different transitions. Using the same control for two different conditions would be more confusing IMHO.

georgii-borovinskikh-sonarsource · 2023-11-24T10:02:17Z

src/Integration/Muting/MuteWindowDialog.xaml

+        <Border BorderBrush="Black" Name="BorderAccept" BorderThickness="1" HorizontalAlignment="Left" Height="75" Margin="25,25,0,0" VerticalAlignment="Top" Width="450" Visibility="{Binding AcceptVisibility}">
+            <Grid>
+                <RadioButton Name="rbAccept" GroupName="Transitions" Content="Accept" VerticalAlignment="Top" Margin="10,16,-10,0" FontWeight="Bold" Grid.ColumnSpan="2" Checked="RadioButton_Checked"/>
+                <Label Content="The issue is valid but does not need fixing. It represents accepted technical debt." HorizontalAlignment="Left" Margin="10,36,0,0" VerticalAlignment="Top" Grid.ColumnSpan="2"/>


The content should be in the resource file, IMO

georgii-borovinskikh-sonarsource · 2023-11-24T10:04:22Z

src/Integration/Muting/MuteWindowDialog.xaml

+                <Label Content="The issue is raised unxpectedly on code that should not trigger an issue." HorizontalAlignment="Left" Margin="10,36,0,0" VerticalAlignment="Top" Grid.ColumnSpan="2"/>
+            </Grid>
+        </Border>
+        <Label Content="Add a comment (optional):" HorizontalAlignment="Left" Margin="25,200,0,0" VerticalAlignment="Top"/>


SLI window had some formatting info in below the text box. We should at least add a ticket for that feature.

georgii-borovinskikh-sonarsource · 2023-11-24T10:05:10Z

src/Integration/Muting/MuteWindowDialog.xaml.cs

+
+        private void NewMethod(bool showAccept)
+        {
+            BorderWontFix.Visibility = showAccept ? Visibility.Hidden : Visibility.Visible;


See my point about hidden buttons above

georgii-borovinskikh-sonarsource · 2023-11-24T10:05:42Z

src/Integration/Muting/MuteWindowDialog.xaml.cs

+            NewMethod(showAccept);
+        }
+
+        private void NewMethod(bool showAccept)


ugras-ergun-sonarsource · 2023-11-24T11:23:54Z

src/Integration/Transition/MuteWindowDialog.xaml.cs

+    /// Interaction logic for UserControl1.xaml
+    /// </summary>
+    [ContentProperty(nameof(MuteWindowDialog))]
+    [ExcludeFromCodeCoverage]


UI Code not testable

ugras-ergun-sonarsource · 2023-11-24T11:25:55Z

georgii-borovinskikh-sonarsource · 2023-11-24T11:44:50Z

src/Integration/Resources/Strings.resx

+    <value>Mark Issue as Resolved on SonarQube</value>
+  </data>
+  <data name="MuteWindow_WontFixContent" xml:space="preserve">
+    <value>The issue is valid but does not need fixing. It represents accepted technical debt.</value>


Same content as Accept 🤔

I know I'll talk about it with Marco during the meet

georgii-borovinskikh-sonarsource · 2023-11-24T11:45:22Z

src/Integration/Transition/MuteWindowDialog.xaml

+
+        <Border BorderBrush="Black" Name="BorderAccept" BorderThickness="1" HorizontalAlignment="Left" Height="75" Margin="25,25,0,0" VerticalAlignment="Top" Width="450" Visibility="{Binding AcceptVisibility}">
+            <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"/>


Radio button stye (and possibly others) is duplicated.

georgii-borovinskikh-sonarsource · 2023-11-24T11:45:41Z

src/Integration/Transition/MuteWindowDialog.xaml

+                ,115,0,0" VerticalAlignment="Top" Width="450">
+            <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"/>


Style duplication

georgii-borovinskikh-sonarsource · 2023-11-24T11:48:51Z

src/Integration/Transition/MuteWindowDialog.xaml.cs

+        {
+            switch (sender)
+            {
+                case var value when value == rbWontFix:


Non-blocker: Could be replaced with a dictionary that would look nicer

georgii-borovinskikh-sonarsource · 2023-11-24T11:50:07Z

src/Integration/Transition/MuteWindowDialog.xaml

+        <Label Content="{x:Static resx:Strings.MuteWindow_CommentLabel}" HorizontalAlignment="Left" Margin="25,200,0,0" VerticalAlignment="Top"/>
+        <TextBox Name="txtComment" HorizontalAlignment="Center" Height="60" Margin="0,225,0,0" TextWrapping="Wrap" VerticalAlignment="Top" Width="450"/>
+        <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">


This style is not duplicated, however, it's hard to read

Ah, actually it is duplicated from the Cancel button

georgii-borovinskikh-sonarsource · 2023-11-24T13:00:11Z

src/Integration/Transition/MuteWindowDialog.xaml

        <TextBox Name="txtComment" HorizontalAlignment="Center" Height="60" Margin="0,225,0,0" TextWrapping="Wrap" VerticalAlignment="Top" Width="450"/>
-        <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"/>


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

margins are different in each component

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" }

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

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

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.

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

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

georgii-borovinskikh-sonarsource · 2023-11-24T15:13:32Z

src/Integration/Transition/MuteWindowDialog.xaml

+                <Label Content="{x:Static resx:Strings.MuteWindow_FalsePositiveContent}" Style="{StaticResource TransitionLabelStyle}" />
+            </Grid>
+        </Border>
+        <Label Content="{x:Static resx:Strings.MuteWindow_CommentLabel}" Style="{StaticResource LabelStyle}" Margin="25,200,0,0"/>


LabelStyle is only used once, margin can be moved there

georgii-borovinskikh-sonarsource · 2023-11-24T15:13:48Z

src/Integration/Transition/MuteWindowDialog.xaml

+            </Grid>
+        </Border>
+        <Label Content="{x:Static resx:Strings.MuteWindow_CommentLabel}" Style="{StaticResource LabelStyle}" Margin="25,200,0,0"/>
+        <TextBox Name="txtComment" HorizontalAlignment="Center" Height="60" Margin="0,225,0,0" TextWrapping="Wrap" VerticalAlignment="Top" Width="450"/>


Styles in the layout

georgii-borovinskikh-sonarsource · 2023-11-24T15:15:18Z

src/Integration/Transition/MuteWindowDialog.xaml

+            </Grid>
+        </Border>
+
+        <Border Name="BorderAccept" Style="{StaticResource BorderStyle}" Margin="25,25,0,0" Visibility="{Binding AcceptVisibility}">


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

sonarqubecloud · 2023-11-24T15:44:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

Fixes #5032

added window

65bd3ed

ugras-ergun-sonarsource marked this pull request as ready for review November 24, 2023 09:54

ugras-ergun-sonarsource requested a review from georgii-borovinskikh-sonarsource November 24, 2023 09:55

georgii-borovinskikh-sonarsource changed the base branch from master to feature/mute-server-issues November 24, 2023 09:58

georgii-borovinskikh-sonarsource requested changes Nov 24, 2023

View reviewed changes

ugras-ergun-sonarsource added 2 commits November 24, 2023 12:05

PR feedback

cb43f7d

exclude tests

d8f6200

ugras-ergun-sonarsource commented Nov 24, 2023

View reviewed changes

ugras-ergun-sonarsource requested a review from georgii-borovinskikh-sonarsource November 24, 2023 11:26

georgii-borovinskikh-sonarsource requested changes Nov 24, 2023

View reviewed changes

PR feedback

07bbfb4

georgii-borovinskikh-sonarsource reviewed Nov 24, 2023

View reviewed changes

georgii-borovinskikh-sonarsource approved these changes Nov 24, 2023

View reviewed changes

pr feedback

fe6ad37

georgii-borovinskikh-sonarsource reviewed Nov 24, 2023

View reviewed changes

moved styles

4e25fd8

ugras-ergun-sonarsource merged commit 45a093a into feature/mute-server-issues Nov 24, 2023
4 checks passed

ugras-ergun-sonarsource deleted the ue/add-window branch November 24, 2023 15:47

ugras-ergun-sonarsource added a commit that referenced this pull request Dec 11, 2023

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

7894f7c

Fixes #5032

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ugras-ergun-sonarsource commented Nov 24, 2023

ugras-ergun-sonarsource commented Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

ugras-ergun-sonarsource Nov 24, 2023 •

edited

Loading

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

ugras-ergun-sonarsource Nov 24, 2023

ugras-ergun-sonarsource commented Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

ugras-ergun-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

ugras-ergun-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

ugras-ergun-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

ugras-ergun-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

ugras-ergun-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

georgii-borovinskikh-sonarsource Nov 24, 2023

sonarqubecloud bot commented Nov 24, 2023

		@@ -0,0 +1,60 @@
		<vsui:DialogWindow x:Class="SonarLint.VisualStudio.Integration.Muting.MuteWindowDialog"

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

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

Conversation

ugras-ergun-sonarsource commented Nov 24, 2023

ugras-ergun-sonarsource commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ugras-ergun-sonarsource Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ugras-ergun-sonarsource commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 24, 2023

ugras-ergun-sonarsource Nov 24, 2023 •

edited

Loading