Update to .NET Framework 4.6.2 and add No Games Found text

.NET Framework 4.6.1 is no longer available for download, while 4.6.2 is.

msedge_2Ry7BEyUnc

Fix https://github.com/thpatch/thcrap/issues/235

This is also highly recommended, if we want to package the app with MSIX: https://learn.microsoft.com/en-us/windows/msix/packaging-tool/know-your-installer

Quick answer because it's been a while since you opened this pull request:

.NET Framework update makes sense.

IMO, it would be better if the "no games found" message was also displayed if you already have 10 known games, try to add an 11th one (for example because th20 just got released), try to select your whole Touhou games folder, and thcrap doesn't find anything new because your th20 copy that you totally acquired legally was in your Downloads folder instead - so, if the search doesn't find any new game.

I was thinking about quickly implementing that myself, but I didn't do it yet. So, you can either try to argue about why your version is better, implement my idea, or wait until I take the time to implement it.

Apparently the email notifications were in my spam and I haven't been checking Github that frequently. I will take a look at implementing your suggestion.

Check it now @brliron

I have a comment for you, and many others for ReSharper.

First, when it comes to making a commit with many changes to the code style of a file, I think it really helps to split into 2 commits: one for the code style changes, and one for the functional change. Most of the changes in there are just ReSharper arranging the code without any visible change, and a small part is about updating the no new games found error message. But if I want to quickly review how the change to the error message have been implemented and if it looks correct, I need to read and analyze all the ReSharper changes, because they're mixed with the functional change. And the same problem arises if, in the future, I want to browse the history for this file, for example if I'm trying to track down a bug, or to understand why something have been done in some way. I want to be able to quickly identify and skip over the formatting commits. This is a problem that I've encountered several times in my professional life.

Now, on to ReSharper. I'll start by saying that I'm mostly a C and C++ programmer, not a C# programmer, and that some of the following thoughts may be biased towards that.

I don't like C# trying to impose its own coding style. Because people will endlessly fight over which style is better - I've even worked at a place that used 3 spaces for indentation because there was a huge fight between people wanting 2 spaces and people wanting 4 spaces, and they settled this by taking the thing in the middle. And if Microsoft thinks they know which one is objectively better, they're wrong, they're just one more developer group with their own subjective view on that question.

if (_contextMenu != null) return _contextMenu;: this one is funny, because I tried to commit something like that earlier today, and the git pre-commit hook refused my commit because it didn't adhere to the coding style of the project I was working on. The perfect proof that people have many opinions. Anyway, I would lend more towards having it on 4 lines, like this:

if (_contextMenu != null)
{
    return _contextMenu;
}

I don't like the one-line syntax because seeing branches is vital to understand the control flow of a function, and a one-line if hides that if. This is even more important if that if has a return, because return is even more important than if to understand the control flow of a function. Like, when I read the changes to the visibility code around 180-189 (old file) / 191-194 (new file), I think ReSharper decided to unconditionally call PropertyChanged?.Invoke, and I have to really pay attention to notice the early exit. Adding an empty line below the if ... return would help, but the C# syntax doesn't mandate it (otherwise I guess your PR would have it). I don't like the two-lines syntax without braces either, because it makes adding / removing things harder than it needs to be, and more importantly, dangerous. "warning: this 'if' clause does not guard this statement, but the latter is misleadingly indented as if it were guarded by the 'if' " may be my favorite warning in gcc, because I definitely tried to add a new line to a two-lines if more than once. My favorite if syntax is the 3-lines one used in thcrap, with the opening bracket on the same line and the closing bracket on a new line, but I'm fine with the 4-lines one used by C# with braces.

GameJs.Title is a reference to the "title" field in the thxx.js files. As such, it should stay fully lowercase. game_id is also a reference to the game_id global variable from thcrap_tsa and thcrap_tasofro, and for thcrap developers, I think it makes more sense to keep it lowercase.

I like being explicit on this because it makes the variable ownership explicit. Maybe someone who spent 10 years working with C# and nothing else will know that a variable that starts with an uppercase letter is always a member variable from this, but not many of our developers fit this description. This is especially important on a line like WasAlreadyPresent = wasAlreadyPresent.

A variable starting with an underscore irks me as if I wasn't allowed to name my variable like that, because C defines identifiers in the global scope starting with an underscore as reserved. This is fine for this PR, but this is another example of Microsoft being wrong when they think they know what is objectively the best code style, I can't take that code style and bring it with me in C land.

I don't like comments in the middle of the code just to tell a tool to shut up. I mean, this may be fine if said code will only be touched by one tool and never by anything else, but is ReSharper the only code styling tool that exist and will ever exist for C#? If not, why don't we include comments for these other tools? Will we end up with even more comments for the tools than we have code? Note that I also have this problem in the C world, with header / footer comments telling the text editor which size a tab should be etc. There is a problem, but this isn't the right solution for this problem.

Quick question about ReSharper adding private on many functions, isn't the default visibility close enough to private?

I was almost about to say that its uses of the null coalescing operator was nice, until I noticed that it really likes to hide assignments in them. For example: return _name ?? (_name = GameId);. I like the assignment to be on its own line, because, an assignment it changing the state of the program, it's something important and it should be noticeable.

Outside of that, there are some good suggestions in that ReSharper pass, like going from System.Windows.RoutedEventArgs to just RoutedEventArgs or using $"..." for string formatting. But there's everything else above.

Oh, and the part of the commit that's actually related to the message looks good. I'll just need to test it to make sure it works properly, but it looks like it should.

First, when it comes to making a commit with many changes to the code style of a file, I think it really helps to split into 2 commits: one for the code style changes, and one for the functional change. Most of the changes in there are just ReSharper arranging the code without any visible change, and a small part is about updating the no new games found error message. But if I want to quickly review how the change to the error message have been implemented and if it looks correct, I need to read and analyze all the ReSharper changes, because they're mixed with the functional change. And the same problem arises if, in the future, I want to browse the history for this file, for example if I'm trying to track down a bug, or to understand why something have been done in some way. I want to be able to quickly identify and skip over the formatting commits. This is a problem that I've encountered several times in my professional life.

In the future, I will try and keep to this. Working in SAP has made me forgot all about managing commits.

I don't like C# trying to impose its own coding style. Because people will endlessly fight over which style is better - I've even worked at a place that used 3 spaces for indentation because there was a huge fight between people wanting 2 spaces and people wanting 4 spaces, and they settled this by taking the thing in the middle. And if Microsoft thinks they know which one is objectively better, they're wrong, they're just one more developer group with their own subjective view on that question.

They aren't just a development group. They're the developer of language. They provide the default. You can deviate, but you should have a good reason to do so. If you don't need to why use something else?

I don't like the one-line syntax because seeing branches is vital to understand the control flow of a function, and a one-line if hides that if. This is even more important if that if has a return, because return is even more important than if to understand the control flow of a function. Like, when I read the changes to the visibility code around 180-189 (old file) / 191-194 (new file), I think ReSharper decided to unconditionally call PropertyChanged?.Invoke, and I have to really pay attention to notice the early exit. Adding an empty line below the if ... return would help, but the C# syntax doesn't mandate it (otherwise I guess your PR would have it). I don't like the two-lines syntax without braces either, because it makes adding / removing things harder than it needs to be, and more importantly, dangerous. "warning: this 'if' clause does not guard this statement, but the latter is misleadingly indented as if it were guarded by the 'if' " may be my favorite warning in gcc, because I definitely tried to add a new line to a two-lines if more than once. My favorite if syntax is the 3-lines one used in thcrap, with the opening bracket on the same line and the closing bracket on a new line, but I'm fine with the 4-lines one used by C# with braces.

One-liner if-returns are popular as guards. Which allow you invert ifs to reduce nesting. I similary dislike the two-liners for the same reason you mention. I also personally like the three-liner. If you're okay with it I can set it as a solution-wide setting (.editorconfig). For example:

GameJs.Title is a reference to the "title" field in the thxx.js files. As such, it should stay fully lowercase. game_id is also a reference to the game_id global variable from thcrap_tsa and thcrap_tasofro, and for thcrap developers, I think it makes more sense to keep it lowercase. I like being explicit on this because it makes the variable ownership explicit. Maybe someone who spent 10 years working with C# and nothing else will know that a variable that starts with an uppercase letter is always a member variable from this, but not many of our developers fit this description. This is especially important on a line like WasAlreadyPresent = wasAlreadyPresent.

It could be possible to separate models for the json files in a completely separate folder, and set a custom or disable naming convention for it. The default for C# is to use UpperCamelCase for most things to do with classes, lowerCamelCase for local variables/constants, _lowerCamelCase for private fields. As such, it's rare to see this being used explicitly.

I don't like comments in the middle of the code just to tell a tool to shut up. I mean, this may be fine if said code will only be touched by one tool and never by anything else, but is ReSharper the only code styling tool that exist and will ever exist for C#? If not, why don't we include comments for these other tools? Will we end up with even more comments for the tools than we have code? Note that I also have this problem in the C world, with header / footer comments telling the text editor which size a tab should be etc. There is a problem, but this isn't the right solution for this problem.

I don't know specially for comments, but seems ReSharper is compatible with .editorconfig, .clang-format and StyleCop styles. Also if you check the Visual Studio Marketplace sorted by installs ReSharper is at the top. There's also ReSharper for C++ btw. They're also free for leads and core contributors to "active" open-source projects.

A variable starting with an underscore irks me as if I wasn't allowed to name my variable like that, because C defines identifiers in the global scope starting with an underscore as reserved. This is fine for this PR, but this is another example of Microsoft being wrong when they think they know what is objectively the best code style, I can't take that code style and bring it with me in C land.

And what's wrong with different languages having different coding styles? I personally think it's having each own signature allowing me to easily tell them apart and not mix them is nice. But also once again, can be changed of course.

Quick question about ReSharper adding private on many functions, isn't the default visibility close enough to private? Private is the default, ReSharper just likes being explicit. https://www.jetbrains.com/help/resharper/2023.2/ArrangeTypeMemberModifiers.html

I was almost about to say that its uses of the null coalescing operator was nice, until I noticed that it really likes to hide assignments in them. For example: return _name ?? (_name = GameId);. I like the assignment to be on its own line, because, an assignment it changing the state of the program, it's something important and it should be noticeable.

ReSharper also likes to merge declarations and assignments. https://www.jetbrains.com/help/resharper/2023.2/JoinDeclarationAndInitializer.html

@brliron Kind reminder for PR. This discussion is also probably fit for elsewhere.

Sorry, it took a lot longer than I expected, I got busy, I got inspired to work on other things, I got a bit ill, but let's get back to it.

First, on using ReSharper at all. I'm not sold on its utility? From your 2 PRs, it mostly changed how the code looks. Sure, the code may look a bit better (which is subjective), but for the most part, it doesn't run better, and it isn't safer. Also, half of the things it found are things that Visual Studio already told me, and that I ignored because I'm an arrogant boy and I believe I know better than Microsoft (basically everything related to variable names and to this). I don't feel like it really helps us. Maybe we're just good enough to write safe code by ourselves, and ReSharper doesn't find any real issues?

As for the part about ReSharper being free for Open-Source developers, I don't like the idea of being tied to a licence that a company may or may not choose to give us. Like, I didn't make any commit in March, April and July 2023. If the licence renewal happened around any of these months, I wouldn't be eligible. It would also discriminate new developers if our setup process says "get ReSharper" but they can't do it for free.

Anyway, back to the individual points.

One-liner if-returns are popular as guards. Which allow you invert ifs to reduce nesting.

I found 4 places where this was done: Page4.xaml.cs lines 46, 85, 114 / 135 / 141, and 447.

46: In my head, despite the additional indent level, the pre-ReSharper makes more sense. Before ReSharper, we have "If _contextMenu isn't initialized, initialize it. Then, in any case, return _contextMenu". Post-ReSharper, we have "If _contextMenu is initialized, return it. Otherwise, initialize it, and return the new _contextMenu". The 1st version makes slightly more explicit that we're returning the same _contextMenu in every case, with just an additional step when it's null.

85: Not needed, we know that newMenu is a MenuItem. If we want to fix it, we could replace var newMenu = sender as MenuItem; with var newMenu = (MenuItem)sender;, but we don't need an 'if it's null, return'. It it's null, the world is burning and we should crash, the code is working as intended.

114 / 135 / 141: First, I already told you that separating functional commits and code style commits was better, you got my point, and I didn't plan to repeat it, but I spent 5 minutes trying to untangle the logic of what ReSharper did to this code before understanding that it also got a bit of functional change, with the addition of customDisplayName. Anyway, I really like the readability of the pre-ReSharper function. If name is null, do something. If it's still null, try something else. If even after that, it's still null, try a 3rd thing. And return what you got. I easily see that we're trying to initialize name from different sources, trying a 1st one, then a 2nd one, and then a 3rd one. While on the new version, well... I spent 5 minutes trying to untangle the logic of what ReSharper did to this code. The 3 data sources aren't neatly separated, they look mixed together, it looks like _name always depends on all 3 of them. And I need to comment on if (_name != null) return _name ?? (_name = GameId);. First, we're changing global state in the less obvious of places. I don't expect a ?? operator to do almost nothing in one branch and something important in the other, I understand it a bit like a?.b where it's a small syntactic sugar to easily provide a default value if the 1st half is null, I don't expect the expression in there to have any side effect (but I'm not an expert C# developer, I might be wrong on that). But more importantly, the 1st half says if (_name != null), which means that _name isn't null, which means the 2nd half of _name ?? (_name = GameId); is dead code, and that's confusing as hell (this is part of why I spent 5 minutes trying to untangle the logic of the new code, I was wondering how we went from "use game_id as a last option if everything else fails" to "use GameId in every single case"). But my point about how ?? shouldn't have expressions with side effects (in my opinion) still stands because there's return _name ?? (_name = GameId); without the if (_name != null) at the end of this block, and here, that 2nd half may be executed (and oops, I completely forgot that I already complained about that one bit in my previous answer).

447: Not perfect, but ReSharper is pushing this bit of code towards a better direction. Combine both ifs into one and add a newline after the return, and we're good.

I also personally like the three-liner. If you're okay with it I can set it as a solution-wide setting (.editorconfig).

I didn't know this was configurable, this might be something I want (but for ifs, fors and other blocks like that, not for methods). Ideally I would prefer the style used in the rest of thcrap:

if (something) {
    // a
}
else {
    // b
}

but I'm not sure it exists in this setting because it's not really common. And if we can't do that, I'm not sure whether I prefer the variant with } \n else \n { or the variant with } else {. But... Is .editorconfig a project-wide file that should be committed, or a user-local file that each user is expected to customize to their liking? Because if it's the latter, it won't be nice if developers who don't change this setting have to fight with Visual Studio at every single line they change.

It could be possible to separate models for the json files in a completely separate folder, and set a custom or disable naming convention for it

It might work for some things, but not for all of them. For example, as I said, game_id is written like that not because it matches the schema of a JSON file, but because 'thcrap_tsaandthcrap_tasofrohave a commonly-used variable called game_id, and when our experienced developers seegame_id`, they remember it from the C++ code base, and they immediately know what it corresponds to. GameId doesn't have this effect because it's different.

The default for C# is to use UpperCamelCase for most things to do with classes, lowerCamelCase for local variables/constants, _lowerCamelCase for private fields. As such, it's rare to see this being used explicitly.

All our C# developers don't do enough C# to have these conventions ingrained into their brains, and for them, being more explicit and keeping the this helps.

I don't know specially for comments, but seems ReSharper is compatible with .editorconfig, .clang-format and StyleCop styles.

I don't like ReSharper comments in the middle of a file, but I don't mind them in a separate file.

Yeah, no worries. It seems my notifications also have gone to the void this time as well. I guess we can close this PR and I can re-do it without much of the style changes.

Regarding .editorconfig, it is solution/project-wide, C# heavy example, for visual studio C++ usage Microsoft has this page,for C# settings Microsoft has this page, and for general info there's this page.

It might work for some things, but not for all of them. For example, as I said, game_id is written like that not because it matches the schema of a JSON file, but because thcrap_tsa and thcrap_tasofro have a commonly-used variable called game_id, and when our experienced developers see game_id, they remember it from the C++ code base, and they immediately know what it corresponds to. GameId doesn't have this effect because it's different.

This is a bit worrying to me, though. I do not believe using C++ conventions in C# is the best of idea. Especially if it's hotchpotch of both. Sure, we can enable in some style-guide that we should be more explicit. Although, going against the naming conventions used, just because it's a common occurrence doesn't fit well with me. It's not as if you would see 'GameId' you wouldn't know what it is...

@brliron If it's good with you then, I will close this PR an redo this without any formatting. We can then continue style talks in a separate issue.

Sounds good.

Btw, for the .NET version upgrade, we looked a bit into it, and 4.8 seems like a good target.

It has UnmanagedType.LPUTF8Str, which could be very useful if I could get it to work (I couldn't, it was spitting out garbage and out-of-bounds accesses).
It supports the same Windows versions: https://learn.microsoft.com/en-us/dotnet/framework/migration-guide/versions-and-dependencies (the only difference is 4.6.1 being included in older versions of Windows 10, but I think Windows 10 computers tends to be up to date).
On the Wine side, it seems to be the best version: https://appdb.winehq.org/objectManager.php?sClass=application&iId=2586

thpatch / thcrap

Update to .NET Framework 4.6.2 and add No Games Found text #238