Mainwindow Breakdown#118
Conversation
adc3547 to
5645b78
Compare
|
You might know the linux builds fail. I just force-pushed a linux fix. |
5645b78 to
e730181
Compare
|
So further fiddling with this, I got mainwindow.cpp down to 9,500 lines of code. LOL! Like a drop in the ocean, although offloading 3,000 lines of code is about 1/4 of it. I had to move some stuff to the class header. I mean, there's inline declarations in there and stuff you wouldn't even believe. It's like, if there was any doubt throw 'er in mainwindow - it'll all shake out in the end. I've never seen anything like it 👎 But the upside is, it still builds and runs, and can't find anything wrong with it, after offloading 3,000 lines of code from it :-) |
|
The good news is; I got mainwindow.cpp below 9,000 lines of code! The bad news is there's still 8,994 lines of code in that file. Started out with 12,482 lines. But I'm not working on this branch right now. Forked it off into a different one. I might just cherry-pick into this one, or I might scrap this one and submit a new bigger one. |
JS8_Mainwindow: - buildQueryMap.cpp - checkVersion.cpp - dataSink.cpp - displayBandActivity.cpp - displayCallActivity.cpp - initializeDummyDate.cpp - initializeGroupMessage.cpp - networkMessage.cpp - processCommandActivity.cpp - processDecodeEvent.cpp - processRxActivity.cpp reduces mainwindow.cpp from 12,482 to 8,284 lines of code
e730181 to
2c72dc2
Compare
|
OK, so reached a milestone. Removed over 4,000 lines of code from mainwindow.cpp. The exact data is noted in the commit message. There's more that can be done. Just that I reached my goal for one sitting. So I'm shutting down Xcode for the weekend before it pops a fuse or something. It still builds and runs fine on MacOS 26. No memory page faults. Threads look good. It sent 132 packets to PSK Reporter already totaling 19KB. Can't find anything broken in the application. @Joe-K0OG this would be a good one to build and test run on Windows and some flavor of linux. Although it passed all the CI checks here, clang and gcc are not the same animal as gcc will let some things slide that clang won't. |
|
@Chris-AC9KH since you are creating a bunch of new files, I'm going to request that you stick a little blob of code at the top of each file for documentation purposes. No need to add anything after the |
|
Yeah, I can add doxy hooks especially for the Fortran conversion stuff too, since that's now pulled out of mainwindow and is in dataSink.cpp. Breaking that monster down was a little more involved than I planned on. Hard to believe there was over 4,000 lines of code in 11 class member functions! |
316483c to
71a3386
Compare
Joe-K0OG
left a comment
There was a problem hiding this comment.
@Chris-AC9KH Chris, excellent work! I built https://github.com/Chris-AC9KH/JS8Call-improved.git the pr/mainwindow branch on Linux Mint 22.2 and Windows 11 (JTSDK64-Tools + Inno Installer), and all appears to work perfectly on both platforms.
One thing I noticed is this: Before, I would often need to press SHIFT-F4 twice to get the waterfall controls panel to display or close. Now it works on a single SHIFT-F4 press. Something "got" fixed I guess. I never reported it before because it was such a minor issue I forgot about it. This improvement was apparent on both Windows and Linux.
73,
-Joe-
K0OG
|
@Joe-K0OG I've never seen that. There's been nothing done in the code that changes how or what the F4 function key does, no matter what key combination you have to use on your particular keyboard to access it. |
|
There's more that can be done with mainwindow to break it down further. There's still several class member functions in there that are fairly big that can be pulled out into separate files. There's tons of little 5-line or so functions that aren't worth moving outside the mainwindow file. But I figured this was a big enough change for this go-around and it reduced the size of mainwindow considerably. Unfortunately, mainwindow still powers the UI and the backend "engine" is integrated with it. And it has a Qt .ui file associated with it which is why it remains in the JS8_UI folder, along with the rest of the source files that have a .ui file associated with them. |
Yeah, I've not seen any other report of that. This is something that I noticed going back to when Allan and I were working on 2.3. Chris, you're so good that you fix things you didn't know were broken! Ha! 73, |
|
Well, that's not saying it didn't inadvertently change something as to how that works. It is compiled and linked differently now after the source overhaul. The class member functions() that have been pulled out of mainwindow are now compiled as a separate object and linked into mainwindow by the linker, instead of compiling mainwindow in one big glob. The execution order of the mainwindow monster was rather questionable. Who ever wrote that file was either a genius, or had a terrible nightmare and put it into typing. So I suppose it is possible. |
|
And I guess I did fix a couple issues too during this overhaul. There was a function call in the Transceiver class that overrode a member function in TransceiverBase, but it wasn't marked override. So I fixed that. And there was an unused variable in the Modulator that I removed, but that was just good for compiler spam. |
|
Is your plan to land this on 2.5 or 2.6? |
|
This goes into master (2.6.0) only. It will not land on the release/2.5.0 branch. |
|
I specifically locked down the code in release/2.5.0 before starting this. As there is more that can be done here, this was just a big first step since it creates a new commit history from the point where the release/2.5.0 branch was forked. |
|
I've been running the build on this branch for 22 hours continuous uptime. Haven't found any issues with it. So I'm going to say nothing got broke. I'm not going to make any more commits on this branch. |
I've also had it running all day today with no issues, on Linux Mint 22.2. I ran it briefly on Windows this morning and observed no issues. It seems to be very stable. 73, |
|
I'll give it a go here in a few minutes. Are keyboard shortcuts actually mention in the docs anywhere? 🙀 I didn't even know about shift-F4 🤦♂️ |
I don't think they are mentioned in the documentation, but they are not hidden in any way. When you click on any of the menu items, the keyboard shortcuts for several functions are shown. Click on View and you will see that "Show Waterfall Controls" is associated with "Shift+F4". 73, |
|
I'm going to withdraw the documentation tag for this PR, because it has broken with all the files getting moved around. I'll have a related docs PR when I get it all fixed up. Question: Would it be beneficial from the code documentation side of things, to have a previous version available? Ie 2.5.0 and master. |
|
I'll merge this to prevent broken PR's. I don't really think a code documentation run from earlier versions is going to do much good? Developers will want to look at the current version to get an idea of the class structure and execution tree. 2.5.0 and master are very similar except master will have mainwindow partially broken down. I wanted to tackle this over the New Year when PR activity was at a minimum. There's still some things that can be done to break down mainwindow further. But let's do it one member function at a time as work gets done to the affected member function that's still in mainwindow.cpp. This was a big one stripping out over 4,000 lines of code from mainwindow. But I wouldn't want to do it on a regular basis so as to prevent disruption of normal development in the future. |
That varies by operating environment. On MacOS it is Function+F4. But since it is a Qt menu widget, and Qt is built slightly differently for each platform, it should display the correct key combination for whatever platform you are running. |
So everybody knows mainwindow.cpp is a monster. It is all one huge MainWindow class. Which in itself is not a problem. You can create a whole bunch of new classes and break it down that way. But the design muddy's this up. It is a monolithic design, the engine that powers everything is in mainwindow.cpp and it also powers the UI.
So what I'm doing here is moving the includes in the source to the class header. Those should've never been there in the first place, except for a couple that are necessary. Developers see a long list of includes in the source file, oh - let's just bolt on another one (or two, or three).
So then I moved three member functions of the MainWindow class to separate files placed in JS8_Mainwindow, all of these use the same class header so they are just source files for the function implementation. So then you have CMake tell clang to compile the object for these function source files, and the linker knows how to link that object with a one-line function() call in mainwindow.cpp.
So this got rid of 550 lines of code out of mainwindow and put it into separate files. So now if you want to work on the software updater, you can find the code for it in the file that's the same name as the function, without groping around in mainwindow.cpp to find it.
This all works fine on MacOS 26 with clang 17. Submitting the PR here to see how the CI build system handles it.