[Pre]Steps to modernize the code - What do you think?

[gman-comicdb]

I've been trying to figure out a way to make the codebase less unwieldy. After several attempts, think I'm starting to get somewhere and wanted to get people's thoughts around how the ComicRackCE codebase should look in the future.
To be clear, "somewhere" is barely scratching the surface, but I still think it can make a significant difference to making the code more approachable.

Note: This discussion differs from #10 (Steps to modernize the code) in that the focus is NOT making functional changes. Instead the aim is to re-organize the codebase so that you don't have to be a surgeon to work on it.

I've listed a few of the threads I'm currently thinking about below, but really the intention is to get an idea of what the community thinks.

---

• How do you think the codebase should be structured?

• What do you think needs to happen to make it more approachable to newcomers?

• Which parts are sorely in need of some TLC? _{(don't say OpenGL)}

---

Program and MainForm

They're the first 2 classes and they're monolithic; they need to be massively cut down. They talk to everything and everything talks to them. I hope this one isn't too controversial.

I've taken a crack at this here for MainForm and here. I'm not raising a pull request for these changes anytime soon; they're POC.

For Program, the idea was to move towards dedicated classes for the (many) roles it's currently playing.
For MainForm, the branch only looked at splitting up the menus and related commands. It needs a similar treatment of breaking way all of the many things it shouldn't be responsible for, as a UI component.

Layers : Split up UI and non-UI

Idk whether MVP/MVC/MVVM or whatever model makes sense for ComicRack, but it's not the current spider(-man)'s web.

This is tied to the points above and below but is wider in scope, so thought it deserved highlighting. I've tried a few different approaches, none of which I'm completely happy with, although they are steps forward.

Restrict problematic references

This one might be a bit more controversial. It's a looking-forward approach. Native Win32 calls are unmanaged, and they're being made everywhere. System.Windows.Forms locks CRCE to Windows only. System.Windows.Drawing has been deprecated for a number of years, and it shows.

The aim isn't to get rid of these things, it's to abstract away from them so that they're easier to replace in future. For Forms, this is partly splitting up the UI and non-UI components. For Drawing and Win32, it's a case of restricting where they are directly referenced.

Build: make betterer

This one is probably more controversial, in the sense that it's probably a pretty opinionated change.
Started with with opinions on whether it's needed or not.

IMO the build process could so with some TLC. Here are some of things I've messed around with and want to happen:

• Move projects to src subdirectory, to clearly distinguish from configuration files
• Review and document project + config files (git, editorconfig, build files, assembly info, app.config etc)
• Build to bin root directory instead of ComicRack project subdirectory
• Move non-CRCE DLLs from root to sub-directory
• Create PowerShell module to do the heavy lifting
• Documentation, documentation, documentation

[maforget]

Most of this is way beyond the scope of this project. As I've inferred previously I don't want to change things for changing things sake. Just because something isn't optional doesn't mean we need to change it, it only adds needless work that will not bring anything to the user and bring more potential bugs. The idea is to enhance an already mature code base, not completely change it.

I don't think the problem with newcomers is that their are big monolithic classes, but more that they aren't programmers and a lot of stuff is behind layers of abstractions/delegates that makes it hard to follow. When you need to go through half a dozen calls to find the actual place to change is probably why a lot of users that have tried to understand the code have not been able.

The way the program is structured is fine like it is, we are working on an existing code base, there is no needs split it up using different model like MVVM, etc. Also the fact that it is a Winforms projects make it simpler for newcomers than adding layers would help.

In fact changes like your MainForm changes would probably do more harm than good in that regards. The fact that it's a Form and that all related calls are in the same place makes it way easier to find than go through a dozen partial class which seems really really off. I've never seen something like that. I'd add some region and live with the code we've been given.

I would be more inclined to accept changes like your Program class split into different class, although I kinda like having a central Program location than having to remember which class was that in again. Adding more friction than removing it.

My main pain point are not the big classes, but the big methods that would help to be cleaned-up. Things like OnPaint/OnDraw methods could probably help being split into separate function calls so trying to understand what is happening is more straight forward by having a couple of calls than a half mile method with different embedded if statements. Refactoring some variables names that came from decompilation (like num) to actual useful variable names, adding some comments to understand the workflow. Something like ItemVIew.CalcItemPositions or CoverViewItem.OnDraw are really egregious in that regards. It's something I try to do when I touch at these calls. I'd remove some braces when it's simple oneline if calls, to add some space instead of having braces would make it more readable.

Ex from CoverViewItem.OnDraw:

if (thumbTileRenderer2.RatingStripRenderer == null || !Comic.EditMode.CanEditProperties())
{
    break;
}

Should be:

if (thumbTileRenderer2.RatingStripRenderer == null || !Comic.EditMode.CanEditProperties())
    break;

About System.Windows.Forms/System.Windows.Drawing

Let's be realistic will we ever go away from using Winforms to something that is cross platform? Having a Linux version would be great, but would require a lot of work, that is especially way way out of scope. I even added it to the Readme, not to expect a UI rewrite and having a Linux version would require that. Would require changing to .NET 9/10 which brings other problems like broken feature and potential breaking of plugins. So even this I am not fully sure it would be worth it, which is why I haven't really attempted it. The fact that 4.8 is preinstalled is also easier for users without requiring to install another dependencies (I know we could add it to the installer/package the framework with it, but this is easier for users). New version would add new language feature that are nice but not must have and better performance.

Also System.Windows.Drawing isn't depreciated it just isn't packaged with the new framework because it uses the Windows API and wouldn't be cross-platform. The replacement they had used some library that MS didn't have control over that was untested and missing functionality without any help from the lib developers, so they stayed with their Windows only versions. It's available as a nuget package. But again this is all comes down to is it really doable without a ton of work?

I would probably try to make the program more compatible/easier to config for Linux user by using WINE. Some users for example have expressed wanting a docker package, not sure how it would work but that might be more of an avenue than rewriting the software.

Same with the Win32 calls, it makes sense to have all Windows calls into a central location, but it doesn't mean that we need to move all the existing calls and adds coupling when it sometimes makes sense to have it all enclosed in a single file. Just for example the HeifAvifImage I've added, as a call to the native library, it would make zero sense to move that call anywhere else since it is only used by that class anyway. Adding only HeifAvifImage is enough to have all the functionality required. A class should be self-enclosed as much as possible.

About the build process

• Sure moving to a src folder could be something we do, not sure it is really required though. Seems fine like it is.
• Not sure what you mean about Review and document project + config files. Things like AssemblyInfo/App.Config are standard msbuild files, I am not sure what you want to change. editorconfig could be updated (by they way can we stop with the sorting using every time).
• How would you go about moving the bin folder to the root directory? Just change the build location? I mean sure why not, but each projects also build it's own bin location so you would also have these to deal with anyway. It's also the standard way of that the framework build things, I don't see the need to change a standard behavior to a non standard one for no reason.
• What heavy lifting would powershell do? I mean I already use it for zipping Resources folders, but most of the build process should be first and foremost done by the csproj and only use a script when required. A lot can be done using custom build targets and using Pre/Post build events.
• Yes more documentation is good.

Move non-CRCE DLLs from root to sub-directory

I've saved this for last. Absolutely NOT. I've seen some of your code in that regard at some point. The big issue is that the program should be a drop-in replacement for the Original or previous versions. The problem would be that someone would replace the file from their existing installation leaving all the original dlls in the root folder. Because Windows has an hardcoded way of looking for Dlls it will start with looking in the root folder always, so in this case it would just use the original libraries and not the one in the subdir. Then you would update some libraries but these updated libraries would never be used since there would be one in the base folder. That function you used (IIRC) only works when the program fails to find the dll in Windows search location (next to folder, Windows dir, PATH, etc.)

Not everyone uses the installer that would prompt to uninstall and it would open ourselves to DLL hijacking attacks.

---

I do not mean to pop your bubble or sound like that work isn't useful in anyway, but I've stated again & again that I do not want to change things for changing things sake. And anything that adds needless work for me or you without visible returns is something I would avoid.

Here is what could be changes to focus on, beside Viewer changes which really need some love & the aforementioned methods cleanup. Some comments in the code are also most welcome.

Proper SQL support
Instead of having the SQL database just store the XML file it could be a proper database with all properties having their own columns. I would use Entity Framework to use as a ORM between the classes & database. It's something I've already had experience with. It can also be used to have SQLite support beside MSSQL/MYSQL support. Something to keep in mind is EF6 is different than EF Core, so if we ever upgrade to .NET 9/10 it would require a redo, which is why I haven't attempted it.

A lot of SQL user I've talked to said they switched to SQL for performance reasons, because the XML was lagging because they had too much books. I don't know where they got that, IMHE it's the contrary, SQL seems less performant, since when using the XML it's all loaded in memory at start. SQL constantly talks to the server while the XML updates once every 10 minutes, so I believe the lag they had was probably another unrelated issue.

I also had at least 2 users report that SQL failed to update and one was even unable to add books to it. It's something hard to diagnose because it could be network problems, configurations, etc. Could be some errors is making the Database Queue crash somehow, IDK. So some investigation in that regards is required.

WINE Support
As I've stated above a better alternative could be just to provide some documentation at least on how to use it with WINE. I have never dealt with it so I don't know if we could provide some configuration to streamline it's use. Someone on Reddit once made a post with some details, like which OS to target and changing DeleteAPI to SHFileOperation.

That Discord users that stated that Dark Mode didn't work on WINE, it could be a good thing for you to investigate.

Testing
This is something I've been thinking about, but it might not be doable easily with some functions, but would be good to start adding for somethings.

Localization
It seems that there was some program available that we can't seem to find that would have helped user translating. If we can't find it, we might have to create our own. Probably using Reflection to find all the items so that it lists the key & English value and creates & reads the zip archive. All forms elements are also in these files so it would need to list them all. This last one is another reason why we shouldn't breakup MainForm because it could break the existing languages.

Some of the new UI elements I've added also didn't use the TR, like others because of that missing program it wasn't something I had in mind, but would need to be reviewed and add in these case.