toast notifications(+) #17
@@ -86,7 +86,8 @@ namespace ntfysh_client
|
||||
timeoutSeconds: (int)Program.Settings.Timeout,
|
||||
icon: priorityIcon,
|
||||
showTimeOutBar: Program.Settings.CustomTrayNotificationsShowTimeoutBar,
|
||||
showInDarkMode: Program.Settings.CustomTrayNotificationsShowInDarkMode
|
||||
showInDarkMode: Program.Settings.CustomTrayNotificationsShowInDarkMode,
|
||||
playNotificationSound: Program.Settings.CustomTrayNotificationsPlayDefaultWindowsSound
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -156,6 +157,7 @@ namespace ntfysh_client
|
||||
dialog.UseCustomTrayNotifications = Program.Settings.NotificationsMethod == SettingsModel.NotificationsType.CustomTray;
|
||||
dialog.CustomTrayNotificationsShowTimeoutBar = Program.Settings.CustomTrayNotificationsShowTimeoutBar;
|
||||
dialog.CustomTrayNotificationsShowInDarkMode = Program.Settings.CustomTrayNotificationsShowInDarkMode;
|
||||
dialog.CustomTrayNotificationsPlayDefaultWindowsSound = Program.Settings.CustomTrayNotificationsPlayDefaultWindowsSound;
|
||||
dialog.Timeout = Program.Settings.Timeout; // set timeout last so bounds are setup before setting value
|
||||
|
||||
//Show dialog
|
||||
@@ -171,6 +173,7 @@ namespace ntfysh_client
|
||||
Program.Settings.NotificationsMethod = (dialog.UseNativeWindowsNotifications)? SettingsModel.NotificationsType.NativeWindows : SettingsModel.NotificationsType.CustomTray;
|
||||
Program.Settings.CustomTrayNotificationsShowTimeoutBar = dialog.CustomTrayNotificationsShowTimeoutBar;
|
||||
Program.Settings.CustomTrayNotificationsShowInDarkMode = dialog.CustomTrayNotificationsShowInDarkMode;
|
||||
Program.Settings.CustomTrayNotificationsPlayDefaultWindowsSound = dialog.CustomTrayNotificationsPlayDefaultWindowsSound;
|
||||
|
||||
//Save new settings persistently
|
||||
SaveSettingsToFile();
|
||||
@@ -330,6 +333,7 @@ namespace ntfysh_client
|
||||
NotificationsMethod = SettingsModel.NotificationsType.NativeWindows,
|
||||
CustomTrayNotificationsShowTimeoutBar = true,
|
||||
CustomTrayNotificationsShowInDarkMode = false,
|
||||
CustomTrayNotificationsPlayDefaultWindowsSound = true,
|
||||
};
|
||||
|
||||
private void MergeSettingsRevisions(SettingsModel older, SettingsModel newer)
|
||||
@@ -347,6 +351,7 @@ namespace ntfysh_client
|
||||
older.NotificationsMethod = newer.NotificationsMethod;
|
||||
older.CustomTrayNotificationsShowTimeoutBar = newer.CustomTrayNotificationsShowTimeoutBar;
|
||||
older.CustomTrayNotificationsShowInDarkMode = newer.CustomTrayNotificationsShowInDarkMode;
|
||||
older.CustomTrayNotificationsPlayDefaultWindowsSound = newer.CustomTrayNotificationsPlayDefaultWindowsSound;
|
||||
}
|
||||
|
||||
//Update the revision
|
||||
|
||||
@@ -4,6 +4,8 @@ using System.Runtime.InteropServices;
|
||||
|
|
||||
using System.Windows.Forms;
|
||||
using System.Diagnostics;
|
||||
using ntfysh_client.Themes;
|
||||
using Microsoft.Win32;
|
||||
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
using System.Media;
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
|
||||
|
||||
namespace ntfysh_client
|
||||
@@ -33,7 +35,7 @@ namespace ntfysh_client
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
changed changed
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
removed removed
removed removed
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
updated updated
updated updated
swapped swapped
swapped swapped
removed removed
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
InitializeWindowHidden();
|
||||
}
|
||||
|
||||
public void ShowNotification(string title, string message, int timeoutSeconds = 0, ToolTipIcon? icon = null, bool showTimeOutBar = true, bool showInDarkMode = true)
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
public void ShowNotification(string title, string message, int timeoutSeconds = 0, ToolTipIcon? icon = null, bool showTimeOutBar = true, bool showInDarkMode = true, bool playNotificationSound = false)
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
{
|
||||
if (Visible)
|
||||
{
|
||||
@@ -100,6 +102,10 @@ namespace ntfysh_client
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
changed changed
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
removed removed
removed removed
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
updated updated
updated updated
swapped swapped
swapped swapped
removed removed
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
// ok, show the window
|
||||
Show();
|
||||
SetWindowPosition();
|
||||
if(playNotificationSound)
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
{
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
PlayNotificationSound();
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
}
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
}
|
||||
|
||||
private void ApplyTheme()
|
||||
@@ -265,5 +271,28 @@ namespace ntfysh_client
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
changed changed
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
removed removed
removed removed
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
updated updated
updated updated
swapped swapped
swapped swapped
removed removed
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
_shownStopwatch = null;
|
||||
}
|
||||
|
Nitpick: newline Nitpick: newline
I added a blank line before the if statement I added a blank line before the if statement
|
||||
}
|
||||
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
private void PlayNotificationSound()
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
{
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
bool found = false;
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
try
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
{
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
using RegistryKey? key = Registry.CurrentUser.OpenSubKey(@"AppEvents\Schemes\Apps\.Default\Notification.Default\.Current");
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
if (key != null)
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
{
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
Object? o = key.GetValue(null); // pass null to get (Default)
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
if (o != null)
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
{
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
SoundPlayer theSound = new SoundPlayer((String)o);
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
theSound.Play();
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
found = true;
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
I added a blank line before the if statement I added a blank line before the if statement
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
}
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
}
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
}
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
catch
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
{ }
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
if (!found)
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
SystemSounds.Beep.Play(); // consolation prize
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
}
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
I added a blank line before the if statement I added a blank line before the if statement
removed removed
I added a blank line before the if statement I added a blank line before the if statement
updated updated
swapped swapped
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
}
|
||||
}
|
||||
|
||||
|
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: One liner? Nitpick: One liner? `if (this.shownStopwatch is null) return;`
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
What's the purpose of this override? What's the purpose of this override?
What's the purpose of this override? What's the purpose of this override?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
This is a great candidate for the new This is a great candidate for the new `switch expression` syntax, and then it can be turned into a lambda
```csharp
private Image? ConvertToolTipIconToImage(ToolTipIcon icon) => icon switch
{
ToolTipIcon.Info => SystemIcons.Information.ToBitmap(),
ToolTipIcon.Warning => SystemIcons.Warning.ToBitmap(),
ToolTipIcon.Error => SystemIcons.Error.ToBitmap(),
_ => null
}
```
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Lambda one liner Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: Naming convention is capitalised first letter title casing without underscores Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
This appears to be set but never read outside of the This appears to be set but never read outside of the `progress` getter
Could this just read Could this just read `this.progressBar1.Value` instead?
Could this just read Could this just read `this.progressBar1.Value` instead?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Any reason we're wrapping this? Is it protected usually? Any reason we're wrapping this? Is it protected usually?
Does this need Does this need `this`es?
Does this need Does this need `this`es?
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: newline Nitpick: newline
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
Nitpick: Nitpick: `timeout_ms` should be `timeoutMilliseconds`
immediate? immediate?
immediate? immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway. I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style? @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I'll change the comment to better explain the desired intent rationale: I'll change the comment to better explain the desired intent
```suggestion
// don't animate, immediately "close"
```
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I think this is just left over from my first attempts at getting the animation to work. I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment). I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed changed
changed changed
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
removed removed
removed removed
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
updated updated
updated updated
swapped swapped
swapped swapped
removed removed
removed removed
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
yes, I originally thought there would be more to the setter. Removed. yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
Not in the end. Removed. (I had originally though the UI would be Not in the end. Removed. (I had originally though the UI would be `.SetInfo(...); .Show()` or something, but the way it is now has a better expandability and doesn't use this.
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
no, that's just what style I was used to back when I last worked in C# (removed all) no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
This got changed to This got changed to `timeoutSeconds` to deal with corresponding comment in `MainForm.cs`
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
I added a blank line before the if statement I added a blank line before the if statement
heh, wonder where that style came from... heh, wonder where that style came from...
heh, wonder where that style came from... heh, wonder where that style came from...
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
settings dialog is using camelCase for everything.... I think the Pep8 guidance that "consistency within a file is most important" is a good point here. I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase. settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed renamed
renamed renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible. > @alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments). I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
|
||||
19
ntfysh_client/SettingsDialog.Designer.cs
generated
@@ -42,6 +42,7 @@ namespace ntfysh_client
|
||||
useCustomTrayNotifications = new System.Windows.Forms.RadioButton();
|
||||
useNativeWindowsNotifications = new System.Windows.Forms.RadioButton();
|
||||
groupCustomNotificationSettings = new System.Windows.Forms.GroupBox();
|
||||
customNotificationsPlayWindowsNotificationAudio = new System.Windows.Forms.CheckBox();
|
||||
customNotificationsShowInDarkMode = new System.Windows.Forms.CheckBox();
|
||||
customNotificationsShowTimeoutBar = new System.Windows.Forms.CheckBox();
|
||||
label1 = new System.Windows.Forms.Label();
|
||||
@@ -59,7 +60,7 @@ namespace ntfysh_client
|
||||
buttonPanel.Controls.Add(cancelButton);
|
||||
buttonPanel.Controls.Add(saveButton);
|
||||
buttonPanel.Dock = System.Windows.Forms.DockStyle.Bottom;
|
||||
buttonPanel.Location = new System.Drawing.Point(0, 316);
|
||||
buttonPanel.Location = new System.Drawing.Point(0, 336);
|
||||
buttonPanel.Margin = new System.Windows.Forms.Padding(4, 3, 4, 3);
|
||||
buttonPanel.Name = "buttonPanel";
|
||||
buttonPanel.Size = new System.Drawing.Size(531, 51);
|
||||
@@ -174,14 +175,25 @@ namespace ntfysh_client
|
||||
//
|
||||
// groupCustomNotificationSettings
|
||||
//
|
||||
groupCustomNotificationSettings.Controls.Add(customNotificationsPlayWindowsNotificationAudio);
|
||||
groupCustomNotificationSettings.Controls.Add(customNotificationsShowInDarkMode);
|
||||
groupCustomNotificationSettings.Controls.Add(customNotificationsShowTimeoutBar);
|
||||
groupCustomNotificationSettings.Location = new System.Drawing.Point(12, 243);
|
||||
groupCustomNotificationSettings.Name = "groupCustomNotificationSettings";
|
||||
groupCustomNotificationSettings.Size = new System.Drawing.Size(504, 67);
|
||||
groupCustomNotificationSettings.Size = new System.Drawing.Size(504, 87);
|
||||
groupCustomNotificationSettings.TabIndex = 10;
|
||||
groupCustomNotificationSettings.TabStop = false;
|
||||
//
|
||||
// customNotificationsPlayWindowsNotificationAudio
|
||||
//
|
||||
customNotificationsPlayWindowsNotificationAudio.AutoSize = true;
|
||||
customNotificationsPlayWindowsNotificationAudio.Location = new System.Drawing.Point(4, 59);
|
||||
customNotificationsPlayWindowsNotificationAudio.Name = "customNotificationsPlayWindowsNotificationAudio";
|
||||
customNotificationsPlayWindowsNotificationAudio.Size = new System.Drawing.Size(200, 19);
|
||||
customNotificationsPlayWindowsNotificationAudio.TabIndex = 2;
|
||||
customNotificationsPlayWindowsNotificationAudio.Text = "Play Windows notification sound";
|
||||
customNotificationsPlayWindowsNotificationAudio.UseVisualStyleBackColor = true;
|
||||
//
|
||||
// customNotificationsShowInDarkMode
|
||||
//
|
||||
customNotificationsShowInDarkMode.AutoSize = true;
|
||||
@@ -216,7 +228,7 @@ namespace ntfysh_client
|
||||
AutoScaleDimensions = new System.Drawing.SizeF(7F, 15F);
|
||||
AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font;
|
||||
BackColor = System.Drawing.Color.White;
|
||||
ClientSize = new System.Drawing.Size(531, 367);
|
||||
ClientSize = new System.Drawing.Size(531, 387);
|
||||
Controls.Add(label1);
|
||||
Controls.Add(groupCustomNotificationSettings);
|
||||
Controls.Add(nativeVersusCustomNotificationsGroupBox);
|
||||
@@ -266,5 +278,6 @@ namespace ntfysh_client
|
||||
private System.Windows.Forms.CheckBox customNotificationsShowTimeoutBar;
|
||||
private System.Windows.Forms.CheckBox customNotificationsShowInDarkMode;
|
||||
private System.Windows.Forms.Label label1;
|
||||
private System.Windows.Forms.CheckBox customNotificationsPlayWindowsNotificationAudio;
|
||||
}
|
||||
}
|
||||
@@ -61,6 +61,12 @@ namespace ntfysh_client
|
||||
get => customNotificationsShowInDarkMode.Checked;
|
||||
set => customNotificationsShowInDarkMode.Checked = value;
|
||||
}
|
||||
|
||||
public bool CustomTrayNotificationsPlayDefaultWindowsSound
|
||||
{
|
||||
get => customNotificationsPlayWindowsNotificationAudio.Checked;
|
||||
set => customNotificationsPlayWindowsNotificationAudio.Checked = value;
|
||||
}
|
||||
#endregion
|
||||
|
||||
public SettingsDialog()
|
||||
|
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter Nitpick: Naming convention is capitalised first letter
Would it be better to make Would it be better to make `0` no timeout requiring closing notifications instead of `-1`? My logic behind this is `0` is not a valid value anyway, and would result in the immediate closure of the notification, which seems somewhat pointless and a bug. Then we don't end up with an invalid value between two valid values (`-1` and `1`) and can enforce a minimum value of `0` easily.
0 for not timeout makes sense. Although, a 0 was valid before. (but...., If I understand correctly, a default Windows install will ignore anything outside of 1-10 and just use 10 instead, so a 0 before became 10 s, ...., but the way we have it setup now anyone upgrading will have to open the settings dialogue to switch over anyays...) I'm leaning toward using 0. 0 for not timeout makes sense. Although, a 0 was valid before.
(but...., If I understand correctly, a default Windows install will ignore anything outside of 1-10 and just use 10 instead, so a 0 before became 10 s, ...., but the way we have it setup now anyone upgrading will have to open the settings dialogue to switch over anyays...)
I'm leaning toward using 0.
renamed renamed
Changed Changed
Changed to 0 and dropped -1 handling/updating Changed to 0 and dropped -1 handling/updating
|
||||
|
||||
@@ -15,5 +15,6 @@
|
||||
public NotificationsType NotificationsMethod { get; set; }
|
||||
public bool CustomTrayNotificationsShowTimeoutBar { get; set; }
|
||||
public bool CustomTrayNotificationsShowInDarkMode { get; set; }
|
||||
public bool CustomTrayNotificationsPlayDefaultWindowsSound { get; set; }
|
||||
}
|
||||
}
|
||||
Nitpick: newline
Nitpick: newline
Nitpick: One liner?
if (this.shownStopwatch is null) return;Nitpick: One liner?
if (this.shownStopwatch is null) return;Nitpick: newline
Nitpick: newline
What's the purpose of this override?
What's the purpose of this override?
Nitpick: newline
Nitpick: newline
This is a great candidate for the new
switch expressionsyntax, and then it can be turned into a lambdaThis is a great candidate for the new
switch expressionsyntax, and then it can be turned into a lambdaNitpick: Lambda one liner
Nitpick: Lambda one liner
Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: Naming convention is capitalised first letter title casing without underscores
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
Nitpick: convention is use of _ for private variables to avoid the use of this. Goes for all others in this section
This appears to be set but never read outside of the
progressgetterThis appears to be set but never read outside of the
progressgetterCould this just read
this.progressBar1.Valueinstead?Could this just read
this.progressBar1.Valueinstead?Any reason we're wrapping this? Is it protected usually?
Any reason we're wrapping this? Is it protected usually?
Does this need
thises?Does this need
thises?Nitpick: newline
Nitpick: newline
Nitpick: newline
Nitpick: newline
Nitpick: newline
Nitpick: newline
Nitpick: Naming convention is capitalised first letter
Nitpick: Naming convention is capitalised first letter
Nitpick:
timeout_msshould betimeoutMillisecondsNitpick:
timeout_msshould betimeoutMillisecondsimmediate?
immediate?
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
I try to rename my elements to match the TitleCase naming convention. Not a major one, just a nice-to-have. I appreciate this isn't consistent within the application anyway.
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
@alexhorner, is there a linter tool or editor config file that I can setup to check code against the desired style?
I'll change the comment to better explain the desired intent
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I'll change the comment to better explain the desired intent
rationale:
if a user click the x button, they expect the window to go away "now". My observation is that general Windows users are about 50% split on whether a going away animation in response to telling a window to close is annoying or "fine" (and only a few people "prefer it"). Because of this, I intentionally implemented the button close handler to immediately close the pop up (don't animate, and don't wait for the timeout).
I think this is just left over from my first attempts at getting the animation to work.
I think this is just left over from my first attempts at getting the animation to work.
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
I went through and attempted to add a blank line before every if statement that was not the opening statement of the parent scope (and if preceded by a comment, the blank line went before the comment).
changed
changed
I added a blank line before the if statement
I added a blank line before the if statement
removed
removed
I added a blank line before the if statement
I added a blank line before the if statement
updated
updated
swapped
swapped
removed
removed
yes, I originally thought there would be more to the setter. Removed.
yes, I originally thought there would be more to the setter. Removed.
Not in the end. Removed. (I had originally though the UI would be
.SetInfo(...); .Show()or something, but the way it is now has a better expandability and doesn't use this.Not in the end. Removed. (I had originally though the UI would be
.SetInfo(...); .Show()or something, but the way it is now has a better expandability and doesn't use this.no, that's just what style I was used to back when I last worked in C# (removed all)
no, that's just what style I was used to back when I last worked in C# (removed all)
This got changed to
timeoutSecondsto deal with corresponding comment inMainForm.csThis got changed to
timeoutSecondsto deal with corresponding comment inMainForm.csI added a blank line before the if statement
I added a blank line before the if statement
I added a blank line before the if statement
I added a blank line before the if statement
heh, wonder where that style came from...
heh, wonder where that style came from...
settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
settings dialog is using camelCase for everything....
I think the Pep8 guidance that "consistency within a file is most important" is a good point here.
I opted to make the new things in SettingsDialogue always use camelCase, but changed the NotificationDialogue be TitleCase.
renamed
renamed
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
So typically I use Rider (alternatively Resharper) whilst working and it picks out most of these things. As far as I know, what it works on is the microsoft standard convention, so I just use that as it's pretty universal and sensible.
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).
I installed an ran the Rider ide and looked through for additional style changes (and applied the ones that didn't seem to conflict with the previous comments).