Restore GRPC #90
Restore GRPC #90
Conversation
Fix the file pattern to whitelist only emake and emake.exe so it stops picking up emake-tests.exe as the server. Set a timer delay larger than 0 so the server plugin stops hogging the CPU.
|
Really I didn't need to do cc34f3d because you were already indiscriminately building the server plugin into RGM (and really who's going to use it without the plugin) but I did anyway and it's analogous to the one I added to the pro file. In both cases it is off by default. |
|
@fundies Shouldn't we honestly turn it on by default though? And you can just disable it until you get your shit sorted out? |
| @@ -240,11 +240,6 @@ void CompilerClient::UpdateLoop() { | |||
| } | |||
|
|
|||
| ServerPlugin::ServerPlugin(MainWindow& mainWindow) : RGMPlugin(mainWindow) { | |||
| /* | |||
There was a problem hiding this comment.
You don't seem to be checking the new macros you created. This code's just always uncommented. Are you just relying on it not being called?
There was a problem hiding this comment.
I am checking the define RGM_SERVER_ENABLED in MainWindow.cpp which calls this constructor. The plugin interface I designed was meant to be pluggable into RGM, so fundies kind of went commenting it out in the wrong place.
| connect(_ui->menuWindow, &QMenu::aboutToShow, this, &MainWindow::updateWindowMenu); | ||
|
|
||
| auto settingsButton = static_cast<QToolButton *>(_ui->mainToolBar->widgetForAction(_ui->actionSettings)); | ||
| settingsButton->setPopupMode(QToolButton::ToolButtonPopupMode::MenuButtonPopup); | ||
| settingsButton->setToolButtonStyle(Qt::ToolButtonStyle::ToolButtonTextBesideIcon); | ||
| _ui->actionSettings->setMenu(_ui->menuChangeGameSettings); | ||
|
|
||
| #ifdef RGM_SERVER_ENABLED |
There was a problem hiding this comment.
Since you're linking in all the code regardless of whether you intend to use it, this could as well be a runtime check.
Also, we're leaking that pluginServer ref. It should probably be a unique_ptr owned by this class (can be left null when disabled).
There was a problem hiding this comment.
Are you suggesting making it a dll? Yeah that's how Qt Creator has its own plugin interface, I just wasn't ready to set it up yet.
And no, we're not, Qt has its own ownership policy. The server plugin reference is owned by the main window. This is because the server plugin inherits from my generic RGM plugin abstraction which itself is based on QObject. All RGM plugins are informed of the main window during construction in the current implementation.
Line 31 in 6d626d7
There was a problem hiding this comment.
No, I'm just suggesting being consistent about whether we're eliminating code at compile time or branching over it at runtime. My complaint above is related.
I am checking the define
RGM_SERVER_ENABLEDinMainWindow.cppwhich calls this constructor.
You are building an entire unused class into RGM in this case. I suggest either making it a runtime check (e.g, a flag), or removing the class from the codegen.
There was a problem hiding this comment.
Which class is the one you are saying is unused? Are you saying RGMPlugin is unused or ServerPlugin is unused? The former is clearly true, but the latter is not always true. I am in fact filtering out the ServerPlugin class entirely in both the pro qmake and the CMakeLists.
https://github.com/enigma-dev/RadialGM/pull/90/files#diff-af3b638bc2a3e6c650974192a53c7291R162-R166
https://github.com/enigma-dev/RadialGM/pull/90/files#diff-bad885b9516edcb9eac2d0c8c57a7ebdR33-R40
There was a problem hiding this comment.
So if you remove the #if from just MainWindow, it will generate link errors because the Qt project file excludes the class?
There was a problem hiding this comment.
Yes, assuming you haven't uncommented rgm_enable_grpc_server in the pro or CMakeLists.
There was a problem hiding this comment.
A bit awkward, but okay. Proceed.
There was a problem hiding this comment.
Silly me though I forgot to wrap the include in the main window.
There was a problem hiding this comment.
It's ok though, as I said later we'll move plugins to dlls.
This is just the start of my GRPC fixes that I've been discussing. First, I am enabling the server plugin again. Then I fixed the file search pattern to whitelist "emake" and "emake.exe" so that "emake-tests.exe" is no longer matched. I went with this for now because I see Qt only accepts globs, not actual regexes, so the regex Josh gave me won't work yet. Next, I changed the timer delay to 1ms to prevent the plugin from hogging 100% of a CPU core. Finally, I added
rgm_enable_grpc_serverconfig option (disabled by default) in both the pro file and the CMakeLists to make it easier to turn the GRPC server on and off, and so I can keep working on it without undoing fundies commenting out my code.