gah, I was not reactive enough to your message on IRC. I don't think the check for an existing instance of the component prevents you from decoupling construction and attachment of the component. Instead, I suggest you move the check to the component attachment. Maybe another check can be added somewhere else to reduce the risks of memory leaks in future modifications of the code.

I had a voice call with phosit in order to move forward with this review. We chose to work with a unique_ptr. phosit will write a couple incremental patches for eased review, without interface removal to avoid mocking issues, and with minimal boilerplate changes in callers.

(actually this request doesn't warrant a blocking ❌)

Very nice patch. It works on Windows. I agree with the changes proposed 👍🏻

This fixes the issue 👍🏻

I took a first look at this and gave some feedback to phosit on IRC (starting at 14:34).

In D4996#213057, @Stan wrote:

@Itms any opinion on this?

Great, consensus reached! You can perform the changes and I'll commit 🙂

In D4743#212516, @vladislavbelov wrote:

@Itms I think the the warning messages are too specific, as the checked error codes can happen not only for a dangling symbolic link.

This also works well on Windows. EDIT: and also on Linux 👍🏻

I could not reproduce the Jenkins bug 🤷🏻‍♂️

In D4967#212124, @Vulcan wrote:

Debug:
    23>network_dbg.lib(NetEnet.obj) : fatal error LNK1236: corrupt or invalid COFF sections [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\test.vcxproj]

Thanks for the patch! I'm waiting for the Windows build and I'm committing it with minor style fixes.

In D4967#212059, @sera wrote:

I thought about adding it to default.cfg but I feel users really shouldn't touch it unless really really required. So I'm on the fence whether it's a good idea or not to "tell" users it exist instead of it being an emergency workaround. So I let you make the call here.

Hello, this looks nice and clean, thanks for the work!

Here is my work-in-progress rewrite of this feature: https://gitlab.com/na-Itms/0ad/-/commit/b51f29d9e1ff5f2e14e271de9155c9f3a31728cd

Would you plan to move the m_MutedUsers and m_HelperUsers [...]
Would you plan to move the validity testing of the moderation commands [...]

No, those should be in the server indeed. Sorry if I wasn't clear.

Thank you for your answer!

In D4753#210384, @Stan wrote:

Refs https://wildfiregames.com/forum/topic/105716-why-build-with-xp-toolset/?do=findComment&comment=543290

Hi Norse_Harold, I went through this very big patch. First of all thank you again for working on this essential feature. 👍🏻

I went with the const-correct fix and sent it upstream.

Ah shoot, the fix was in D4910, but it got lost when I copied all the headers again before committing. I am actually hesitating between two fixes:

rP27490

In D4910#209709, @Stan wrote:

On what Windows OS(es) did you test this?

Obviously, testers will be smarter than Jenkins, and will rebuild with --build-shared-glooxwrapper.

Note that a disruption in this method has been planned for quite some time and it would be good to perform both changes at once.

This looks good to me and works.

🙃

This is a complete fix for #6676, written by @s0600204, including previous cleanups and a part of the upstream fix to Bug 1739486.

Forgot a file
Fix indentation
Don't patch this issue outside of Linux, it conflicts with wraitii's fix for the virtualenv on Mac and is not needed on Windows

Extra cleanup (correct EOLs and move a build environment variable introduced in D4776 to a better place).

I have no idea what failed!

Here is a key for signing A26 mods.

I'm OK with removing JSHint. Originally the linters were supposed to be complementary, one for the style, one for the logic. But nowadays both detect both kinds of issues, and there is a risk of conflict.

Superseded by SM upgrades.

This change needs D3470 🙂

The SVN change is wrong. If SVN is broken, the differential pipeline should try to build upon a working version, so you must not run svn up. If you want to change the paradigm and always build upon the latest SVN, the pipelines should be entirely rewritten.

Very nice improvement, some major comments though ??

Thanks a lot for the quick review!

Address Freagarach's comments.

The code change is correct and, apparently, works under SM45 and SM52.

Yo Stan, I don't see the need for MesonBuild support, I would recommend creating a Trac ticket with "If Time Permits" urgency, a link to this diff in the Patch field, and then abandon this diff for the time being.

Hi @wraitii! I will need this to perform the SM 52->60 migration (see #1389510 on Bugzilla). The ideal route would be to include this under SM52, but if SM60 adds nice code to handle proxy privates, I am fine with doing this as part of the 52->60 code fixes.