Media first click interact group affiliation check fix

[DarlCat]

Description

This PR fixes an error with how group affiliation was computed for media first click interact purposes.

Related Issues

Issue Link: closes #4405

[TommyTheTerrible]

This changes the behavior from requiring an active group to be set to requiring only a group to be joined for a media web source be run.

The issue associated seems to intentionally target when group is not active, but it is very obvious in the comments in the code that this is the intentional behavior.

I feel like this is a very dangerous change in behavior that should not be the default and require opt-in from the user.

[DarlCat]

@TommyTheTerrible If you review the test plan I wrote for this feature, what you describe and as this PR adjusts the code to reflect, is actually the intended operation for the feature. The prior behavior before this PR was a defect and did not match my specification.

This is not yet a released feature, only developers / testers have used it so far, so it's my opinion that changes to how it functions are very hard to justifiably call dangerous.

I believe you have misunderstood that this feature allows media initialization, it does not. Media first click interact strictly only allows already started media instances to receive mouse hover information, and get clicks on them passed through without a "focus" click.

[TommyTheTerrible]

The CEF browser is the most dangerous part of the viewer, allowing remote code execution and possibly library exploitation from any source defined by any user, while the CEF itself is not being updated regularly across the entire user base to avoid the later.

The fact that the viewer has the option to allow any website to be run automatically from any in-world object is a huge risk in itself.

If this is an effort to change the behavior, it should be labeled as such and not being passed off as a bug or "error" in your words. And if it is a behavior users have relied on, it should be continued to be allowed as an option.

The exclusion of the established behavior in your "testplan" is also worrying. You could have continued it's availability through a "MEDIA_FIRST_CLICK_GROUP_ACTIVE" option.

But to change it's behavior by avoiding mention or discussion through hiding the change in behavior through labeling the old behavior as a bug is unnerving to me.

Mentioning the overall effort in the PR, explaining the change more thoroughly or explaining that it is a deviation from precedent would have greatly changed the perception of the change and the tone of my response.

It is unfortunately common in code bases for people to report bugs and then fix them as a way to insert their personal preferences into the code without review and a defensive part of me turns on whenever I see something similar happen.

The fact the bug report itself seems to have an error as well is setting off more red flags for me. The referenced issue states assigning an 8 to enable the group filter but the lltoolpie.h shows that to be 4?

ea75bfd#diff-6cddc956d98d622e0c9cd053b4a4a1e5cde88e94f09446821b1ae118f86d58cbR97

So, technically this PR does not fix the "bug".

Also, reviewing your changes to the "panel_preferences_sound.xml" does not seem to be conforming to the settings either? It has group labeled as friends and friends labeled as group while the values are wrong for several?

ea75bfd#diff-e7771266f6df854254138a040f9a5fe48d47448661893a0ca0790130c66993a3R410

Personally, I am sorry if this all seems harsh because you and I are minor contributors to the code base but the more time I look at examining all of this the more worried I get.

And any change to the use of CEF makes hairs on my neck stand on end so when I notice them, which is not often because I cannot read every PR and commit, I will get incredibly critical.

As such, I apologize if this seen as a slight but I really think this work needs to be reviewed more thoroughly.

[DarlCat]

@TommyTheTerrible I understand your strong feelings about CEF in the viewer and the security implications it brings, and I absolutely share them!

I want to emphasize that this feature and the latest PRs I have filed do not make media play when it otherwise would not. The media first click interact has no impact on when media is allowed to be initialized and connections to websites be established.

There is a sister feature to this, HUD media autoplay that does modify how autoplay functions, but that is a very clear preference to the user and only impacts attached HUDs worn by the user. That is not the subject of any of these open PRs.

The values in the XUI are computed correctly, please refer to this tooltip which should hopefully explain why the values do not match the enum in the header file exactly:
tool_tip="This setting controls which media (once loaded) does not require a first click to focus before interaction can begin. This allows clicks to be passed directly to media bypassing the focus click requirement. Each option also inherits the previous ones."

Also, when discussing the behavior of the code please cite current commits as values in the enum HAVE changed during development. The initial version of this functionality is not comparable to the current state as at least one other developer has had their hands in it in the interim and documentation has only recently been updated in #4429 along with forced changes to avoid UI precision issues.

[TommyTheTerrible]

Makes sense. Using a mask that I overlooked. I'm sorry for the knee-jerk reaction. The commit was so small and did not seem to match the bug so I got very paranoid.

[TommyTheTerrible]

I want to thoroughly apologize for my panicked and hurried responses to this PR. CEF integration scares me after seeing it being used maliciously in-world and I did not have enough information nor take the proper time to understand the initiative associated with this code submission.

Darl did not deserve to be challenged like this and I am sincerely sorry.