Skip to content

fix: subscribing to a message channel while unsubscribing is pending [MTT-3756] #675

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

Merged
merged 10 commits into from
Jul 13, 2022

Conversation

LPLafontaineB
Copy link
Contributor

@LPLafontaineB LPLafontaineB commented Jun 8, 2022

Description

This PR makes sure that when subscribing to a message channel after having previously subscribed and unsubscribed to it works properly, even if no messages have been published in the mean time. To do so, we keep the list of pending additions and removals from the list of handlers in a dictionary. If we subscribe while a pending unsubscribe is in the dictionary, we simply remove it from the dictionary instead of adding a new pending subscription, and we do the same when unsubscribing if we have a pending subscription.

Issue Number(s)

MTT-3756

Contribution checklist

  • Tests have been added for boss room and/or utilities pack
  • Release notes have been added to the project changelog file and/or package changelog file
  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • JIRA ticket ID is in the PR title or at least one commit message
  • Include the ticket ID number within the body message of the PR to create a hyperlink

@LPLafontaineB LPLafontaineB added 2-Easy This PR is trivial and can be reviewed quickly 1-Needs Review PR needs attention from the assignee and reviewers labels Jun 8, 2022
@LPLafontaineB LPLafontaineB requested a review from pdeschain June 8, 2022 18:23
pdeschain
pdeschain previously approved these changes Jun 8, 2022
Copy link
Contributor

@pdeschain pdeschain left a comment

Choose a reason for hiding this comment

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

We discussed it in Slack, implementation looks good 👍

@pdeschain pdeschain added 2-One More Review One review in, one to go and removed 1-Needs Review PR needs attention from the assignee and reviewers labels Jun 8, 2022
Copy link
Contributor

@SamuelBellomo SamuelBellomo left a comment

Choose a reason for hiding this comment

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

Let's add tests for this case, so there's no regression in the future. This way, even though we don't have a use case in game, we'd have tests to make sure this behaviour doesn't change later.

Instead of auto-injecting in everything inside UICanvas, only include GameObjects that don't already have their components registered via ClientMainMenuState to avoid injecting twice into the same MonoBehaviour
@pdeschain pdeschain added 3-Good to Merge and removed 2-One More Review One review in, one to go labels Jul 12, 2022
@LPLafontaineB LPLafontaineB merged commit a775625 into develop Jul 13, 2022
@LPLafontaineB LPLafontaineB deleted the fix/sub-unsub-resub-msg-channels branch July 13, 2022 13:13
SamuelBellomo added a commit that referenced this pull request Jul 21, 2022
* develop:
  fix: subscribing to a message channel while unsubscribing is pending [MTT-3756] (#675)

# Conflicts:
#	Assets/Scripts/Gameplay/GameState/ServerBossRoomState.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2-Easy This PR is trivial and can be reviewed quickly 3-Good to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants