Fix overlay #4282
Fix overlay #4282
Conversation
Krzmbrzl
force-pushed
the
fix-overlay
branch
2 times, most recently
from
dba3669
to
22b5f16
Compare
|
Tested on Win 10 x64 with TF2 (original issue report). With these changes it works again. I plan to take a look at the code changes as well though, including those from the previous PR. |
|
@Kissaki great :) |
There was a problem hiding this comment.
One issue. Otherwise LGTM.
Although not strictly this PR I am going to take the opportunity to also ask about the last PR/this code if you do not mind.
thought: For unix the setActiveInternal is a noop. I was thinking about how activation works here. I guess we not create the named pipe only if the overlay setting is enabled. The library preloading necessary on unix is a secondary precondition. So what effectively changed now is that the preloading alone is not enough anymore? Or maybe the setting has other influences on unix as well? I think on macOS we do not even have a (enable) setting but the overlay has to be installed separately?
thought: I think I would have liked a clearer separation between the terminology of init and active. I think it feels more intertwined to me right now than it actually is though. Changing the condition for init and moving the init logic into a method is probably all that can be done, but should improve clarity.
src/mumble/Overlay.cpp
Outdated
| if (!act && !m_initialized.load()) { | ||
| // Disabling when the Overlay hasn't been initialized yet, doesn't make much sense | ||
| return; | ||
| } |
There was a problem hiding this comment.
suggestion: Personally I think I would make the init if (act && !m_initialized) instead. This block introduces another condition, negated and combined, that was difficult for me to wrap my head around. I think "init if necessary" would be clearer than this "special special" condition.
There was a problem hiding this comment.
I'm also preventing a call to setActiveInternal as imo we shouldn't call internal functions, if the internals have not been initialized yet.
Plus I'm a big fan of putting exit conditions first in a function so that you don't have to wrap your actual code into if/else-Blocks. In this case it'd only be a single block but this can easily lead to obfuscation of the actual code (imo)
There was a problem hiding this comment.
I generally prefer early returns as well. But with the overall state and stuff it was difficult for me to wrap my head around here. It was harder instead of easier to interpret the function.
There was a problem hiding this comment.
Well we could also rewrite the condition to
if (!(act && m_initialized.load()))if you think that helps...
There was a problem hiding this comment.
I didn't like all alternatives I could come up with, so I left it unchanged. I think in combination with the comment, it should be somewhat clear what this is about 🤔
|
About your thoughts:
|
In aac3214 Overlay::createPipe() has been introduced in order to only create the pipe when needed (instead of in the constructor of the Overlay class). This new function was never called though which caused the Overlay to stop working. This commit makes sure the respective function is called when activating the overlay. Fixes mumble-voip#4281
In the old implementation the overlay was initialized as soon as Overlay::setActive got called regardless of whether it was asked to actually activate. This commit makes sure to only initialize the Overlay if it is actually asked to be activated via Overlay::setActive(true).
The initialization code is now moved into its own function (instead of also living inside the setActive function body). This is done in order to better separate initialization and activation.
Backport of #4282 Additionally this PR also fixes the MXE URLs used in the TravisCI script
This PR makes the overlay work again (got broken in aac3214) and furthermore makes sure that it really only is initialized if needed.
I have not tested the changes to actually work yet. Would be great if someone could do so :)