Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent ignored users from joining games #651

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

GrantBartlett
Copy link
Member

@GrantBartlett GrantBartlett commented Feb 1, 2025

Resolve #649
Resolves the player icons not all showing at once too (because we're doing a who on all users)

Preview:

2025-02-01.20-05-05.mp4

Copy link

github-actions bot commented Feb 1, 2025

Nightly build for this pull request:

@Metadorius Metadorius requested a review from SadPencil February 1, 2025 20:46
DXMainClient/Online/CnCNetManager.cs Show resolved Hide resolved
DXMainClient/Online/IConnectionManager.cs Outdated Show resolved Hide resolved
Comment on lines 982 to 984
foreach (var whoData in whoDataList)
{
(string ident, string host, string userName, string extraInfo) = whoData;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foreach (var whoData in whoDataList)
{
(string ident, string host, string userName, string extraInfo) = whoData;
foreach ((string ident, string host, string userName, string extraInfo) in whoDataList)
{

@@ -618,6 +619,10 @@ private void PerformCommand(string message)
case 311: // Reply to WHOIS NAME query
connectionManager.OnWhoReplyReceived(parameters[2], parameters[3], parameters[1], string.Empty);
break;
case 315: // End of WHO query (RPL_ENDOFWHO)
connectionManager.OnWhoQueryComplete(parameters[1], new List<(string ident, string host, string userName, string extraInfo)>(whoResponseList));
Copy link
Member

@SadPencil SadPencil Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, another review comment.

  • why declaring whoResponseList as a class member? It seems this list is only used here.
  • is whoResponseList always be empty?

@@ -40,6 +40,8 @@ public class CnCNetManager : IConnectionManager
public event EventHandler ReconnectAttempt;
public event EventHandler Disconnected;
public event EventHandler Connected;
public event EventHandler UserListInitialized;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event 'CnCNetManager.UserListInitialized' is never used

@SadPencil SadPencil requested a review from Metadorius February 4, 2025 10:36
@Metadorius
Copy link
Member

@GrantBartlett have you tested this code when an ignored user joins the lobby, but you're not the admin? I don't see any distinction for such cases in code. I don't think ghost banning and silent joining should be done in this case.

@Metadorius
Copy link
Member

I am not sure the changes to Channel and CnCNetManager have to take place there exactly. From a brief look it's not obvious how to make only the host (channel admin) kick/joins silently. It does not seem to be it's responsibility to have such changes?

cc @SadPencil @Rampastring

@SadPencil
Copy link
Member

SadPencil commented Feb 6, 2025

I am not sure the changes to Channel and CnCNetManager have to take place there exactly. From a brief look it's not obvious how to make only the host (channel admin) kick/joins silently. It does not seem to be it's responsibility to have such changes?

cc @SadPencil @Rampastring

I am not sure the changes to Channel and CnCNetManager have to take place there exactly.

Ummm... right, ideally the channel.RequestUserInfo() change in CnCNetManager should be done in another PR? @GrantBartlett

For other changes about silently kicking, maybe it's okay to be done in this PR

From a brief look it's not obvious how to make only the host (channel admin) kick/joins silently.

Yeah. isSilent is determined by IRCUser.IsIgnored where the IgnoreList is a local list.
Suppose user A is not a host and user A banned user B. The host C kicked the user B. Will user A receive such a message, and should they? @GrantBartlett

@Metadorius
Copy link
Member

Ummm... right, ideally the channel.RequestUserInfo() change in CnCNetManager should be done in another PR?

Uh I am not sure what is the point, can you elaborate? I just meant that it felt to me like implementing the feature in the wrong part of the code not responsible for this.

@SadPencil
Copy link
Member

SadPencil commented Feb 6, 2025

Ummm... right, ideally the channel.RequestUserInfo() change in CnCNetManager should be done in another PR?

Uh I am not sure what is the point, can you elaborate? I just meant that it felt to me like implementing the feature in the wrong part of the code not responsible for this.

I mean RequestUserInfo might be an independent enhancement, not related with the ban things?

@Metadorius
Copy link
Member

I mean RequestUserInfo might be an independent enhancement, not related with the ban things?

I don't think splitting that is needed though? Though I guess an explanation would be good as for why it is added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignoring/Blocking a user does not prevent them from joining a game
3 participants