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

Warnings on Campaign Load #5757

Merged
merged 8 commits into from
Jan 16, 2025
Merged

Conversation

IllianiCBT
Copy link
Collaborator

@IllianiCBT IllianiCBT commented Jan 11, 2025

Requires: MegaMek/megamek#6414

  • Added a warning that requests the user complete all active contracts when updating to a new client version.
  • This warning is triggered if the campaign being saved was last saved in an older version and has an active contract.
  • Active contracts are defined as any contract which has already started, or has been accepted but not yet started (i.e. the user is still en route to the contract).
  • The warning does not stop the campaign from loading, to ensure we can still load older saves when investigating bugs and what have you.

Added logic to block campaign updates if active contracts exist, ensuring users complete them before proceeding. Displayed an error dialog to inform users of the issue and maintain version integrity.
Extracted the logic for displaying a warning for active or future contracts into a dedicated method, improving readability and maintainability. Updated corresponding code to use the new method for better encapsulation and reduced duplication.
@IllianiCBT IllianiCBT self-assigned this Jan 11, 2025
Reorganized the active contract logic to directly use `getActiveAtBContracts()` for clarity and efficiency. Moved the contracts list retrieval to conditional logic to avoid redundant calls.
@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.33%. Comparing base (b602ee8) to head (edd5b66).
Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5757      +/-   ##
============================================
- Coverage     10.33%   10.33%   -0.01%     
- Complexity     6124     6127       +3     
============================================
  Files          1036     1037       +1     
  Lines        138533   138592      +59     
  Branches      20482    20495      +13     
============================================
- Hits          14321    14318       -3     
- Misses       122824   122884      +60     
- Partials       1388     1390       +2     

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

Refactored logic to display a warning for active or future contracts into a new `ActiveContractWarning` class. Integrated the new warning mechanism into `CampaignXmlParser` and revised resource properties to include localized messages. Removed redundant code handling contract warnings directly in `CampaignXmlParser`.
@IllianiCBT
Copy link
Collaborator Author

  • Updated to include feedback from Luana

Comment on lines 586 to 591
if (MHQConstants.VERSION.isHigherThan(version)) {
if (retVal.hasActiveAtBContract(true)) {
new ActiveContractWarning();
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

My suggestion would be something like the following, but inside the CampaignFactory file, with the CampaignXMLParser returning a container with the campaign and the relevant diagnostics:

Somewhere add this:

    // Somewhere
    public enum CampaignProblemType {
        NONE,
        ACTIVE_CONTRACT,
        ACTIVE_FUTURE_CONTRACT,
        CANT_LOAD_FROM_VERSION,
        CANT_PARSE_FILE,
    }

    public record ParsedCampaign(Campaign campaign, String currentMekHQVersion, String fileMekHQVersion, CampaignProblemType problem, String diagnostics) { 
    public boolean hasProblems() {
            return problem != null;
        }
    }

In the CampaignFactory


    public ParsedCampaign createCampaign(InputStream is)
        throws CampaignXmlParseException, IOException, NullEntityException {
        if (!is.markSupported()) {
            is = new BufferedInputStream(is);
        }

        byte[] header = readHeader(is);

        // Check if the first two bytes are the GZIP magic bytes...
        if ((header.length >= 2) && (header[0] == (byte) 0x1f) && (header[1] == (byte) 0x8b)) {
            is = new GZIPInputStream(is);
        }
        // ...otherwise, assume we're an XML file.

        CampaignXmlParser parser = new CampaignXmlParser(is, this.app);
        ParsedCampaign parsedCampaign = parser.parse();
        return parsedCampaign;
    }

This way, the place that receives the file will be able to handle it accordingly.

DataLoadingDialog.java

                // And then load the campaign object from it.
                try (FileInputStream fis = new FileInputStream(getCampaignFile())) {
                    ParsedCampaign parsedCampaign = CampaignFactory.newInstance(getApplication()).createCampaign(fis);
                    if (parsedCampaign.hasProblems()) {
                        // TODO: CALL ERROR HANDLING FUNCTION FOR EACH POSSIBLE PROBLEM
                    }
                    campaign.restore();
                    campaign.cleanUp();
                }
                // endregion Progress 6

Otherwise we keep with the problem where we have being imported stuff inserted inside a parser class.

Sorry for not being clear the first time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The "parse()" and the diagnostics can run in two different steps, this way not disrupting any tests or old files and just adding new functionality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I had to wait until my brain had the cycles to process this, but now that it does what you're saying makes perfect sense and will be super maintainable. I would have never thought to use Enums like this. Thanks for the feedback! :)

@HammerGS HammerGS marked this pull request as draft January 15, 2025 01:55
Replaced ActiveContractWarning with a more robust version compatibility check during campaign loading. Unified dialog functionality into CampaignHasProblemOnLoad to handle load issues more effectively. Refactored redundant code, such as getSpeakerIcon, into a common utility to improve maintainability.
Updated error dialogs to adapt button options based on problem type, added detailed handling for unsupported version issues, and refactored code for cleaner logic. This improves user guidance and streamlines the campaign loading process.
@IllianiCBT IllianiCBT marked this pull request as ready for review January 15, 2025 20:31
@IllianiCBT
Copy link
Collaborator Author

  • incorporated Luana's suggestions

Updated copyright headers across multiple files to extend coverage to 2025. This change ensures compliance with proper attribution and copyright practices for the MegaMek and MekHQ project files.
@IllianiCBT IllianiCBT merged commit e4f9b03 into MegaMek:master Jan 16, 2025
4 checks passed
@IllianiCBT IllianiCBT changed the title Added Active Contract Warning on Campaign Load Warnings on Campaign Load Jan 16, 2025
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.

3 participants