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

Fixed Multiple-Nag Bug by Refactoring in-app New Campaign Handling #5357

Merged
merged 2 commits into from
Dec 15, 2024

Conversation

IllianiCBT
Copy link
Collaborator

For a good long while there has been a weird bug where sometimes advancing day would cause nag dialogs to trigger multiple times. This became more noticeable with the introduction of the Turnover module, as that nags users on a monthly basis increasing the likelihood users will interact with this bug.

It's taken me literal months to track down and identify the cause of this bug. It turns out that when a user creates a new campaign, from within an existing campaign, event listeners are not unregistered causing all events to be triggered multiple times. For example, it would mean that any New Day event would be triggered twice. Technically, it would trigger once for each new in-app campaign created. Create 20 new campaigns, one after the other, without exiting mhq? Your new day events would trigger 20 times.

However, if we unregister these event listeners and the user happened to cancel out of the in-app new campaign dialog then no events would trigger until the campaign was reloaded or mhq exited.

Previously we've papered over in-app new campaign issues by having a dialog pop-up advising the user not to cancel once they pass a certain point. That worked well enough for the old 'preset' bug, but that wouldn't work here. At least, not without a really poor play experience.

Instead, what I've done is passed some logic down the chain so that a few things happen when a player triggers an in-app new campaign:

  • We ask the user to confirm they want to start a new campaign. If they fail to confirm, we stop here.
  • We ask the user if they would like to save their existing campaign. If they cancel the save prompt, or a save fails, we stop here.
  • The point of no return is reached. We unregister all event listeners and trigger the new campaign preset picker dialog.
  • If the preset picker dialog is canceled, we exit MekHQ entirely.
  • If the customize preset dialog is canceled, we exit MekHQ entirely.

Streamlined the process for starting a new campaign in-app by introducing `handleInAppNewCampaign` and modifying related classes. Removed the `NewCampaignConfirmationDialog` as its functionality is now integrated into the workflow. Updated exit behavior and campaign options handling to improve consistency and clarity.
Introduced a NewCampaignConfirmationDialog to prompt users before starting a new campaign, enhancing user decision control. Updated relevant GUI logic in CampaignGUI and added corresponding copyright updates for 2024.
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.47%. Comparing base (7ddb24c) to head (974f563).
Report is 64 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5357      +/-   ##
============================================
- Coverage     10.47%   10.47%   -0.01%     
+ Complexity     6069     6065       -4     
============================================
  Files           959      959              
  Lines        135559   135602      +43     
  Branches      19750    19760      +10     
============================================
- Hits          14204    14200       -4     
- Misses       119997   120046      +49     
+ Partials       1358     1356       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@IllianiCBT
Copy link
Collaborator Author

A couple of addendums:

  • I could have sworn there were a couple of bug reports related to this, but I couldn't find them.
  • The duplicate events issue isn't just restricted to New Day events, but effectively any gui-based event. I anticipate this fix having some performance improvements for users who start a lot of campaigns back-to-back.

@psikomonkie
Copy link
Collaborator

Do you think this would fix #4767 ?

@IllianiCBT
Copy link
Collaborator Author

Potentially. iirc we were never able to track down the root cause, but multiple event triggers could cause that, if paid recruitment is a part of the functionality triggered by a new day event.

@HammerGS HammerGS merged commit da490da into MegaMek:master Dec 15, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants