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

Added MHQDialogImmersive for Immersive Dialog Functionality that Immersively Improves Immersion #5709

Merged
merged 7 commits into from
Jan 9, 2025

Conversation

IllianiCBT
Copy link
Collaborator

  • Introduced a new class, MHQDialogImmersive, to provide an immersive dialog interface with support for customizable speakers, messages, and buttons.
  • Renamed AbstractMHQDialog to AbstractMHQDialogBasic to clarify its purpose and use-cases.

This new dialog is designed to allow developers to easily generate dialogs that conform with the new 'incoming transmission' style debuted in 50.02.

I wrote this class in a way that encourages developers to keep functionality code out of dialogs, which makes the functions a lot easier to write Unit Tests for, as we test in a headless environment, so hiding code inside dialogs makes that code effectively untestable.

I've also ensured that any interaction with the dialog will cause it to be dismissed, rather than simply hidden. This ensures we're releasing memory when the dialog is closed, rather than keeping it in memory until the client restarts or such a time GC decides to finally eat it.

At time of writing this class is not used, as I wanted to be kind to whoever reviews this and allow them to focus on reviewing the new class, rather than needing to worry about the 101 implementations of said class.

Updated references and class declarations across the codebase to reflect the new name. This improves clarity by better describing the purpose of the base class within the GUI structure.
Replaced references to AbstractMHQNagDialog.getSpeakerDescription with MHQDialogImmersive.getSpeakerDescription across several classes. Removed the getSpeakerDescription implementation from AbstractMHQNagDialog, streamlining the code with a more centralized utility approach.
Introduced a new class, MHQDialogImmersive, to provide an immersive dialog interface with support for customizable speakers, messages, and buttons. This new dialog is designed to improve user interaction and flexibility by allowing detailed configurations like speaker roles, images, and out-of-character messages. The implementation includes dynamic panel scaling and resource localization to ensure seamless integration into the UI.
Replaced the ButtonLabelTooltipPair class with a Java record for improved readability and conciseness. Updated method calls to directly use record accessors, reducing boilerplate code.
Updated the copyright years in several source files to include 2025. This maintains licensing compliance and reflects the ongoing development of MekHQ.
import static mekhq.gui.dialog.resupplyAndCaches.ResupplyDialogUtilities.getSpeakerIcon;
import static mekhq.utilities.ImageUtilities.scaleImageIconToWidth;

public class MHQDialogImmersive extends JDialog {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't determine whether this should be called MHQDialogImmersive or AbstractMHQDialogImmersive. Guidance would be appreciated. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

By our standards, Abstract... should only be used in the name of a class that is Abstract.
Only a class that does not implement, or inherit from another class, its own methods should be Abstract, so
in this case MHQDialogImmersive is more correct.

@@ -1,3 +1,21 @@
/*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently, I forgot to include the copyright notice when I wrote this class in 2024, so adding it here.

@@ -0,0 +1,417 @@
/*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note for Reviewer: this is the only class that has had functionality change/be introduced. The rest is just housekeeping.

@IllianiCBT IllianiCBT requested a review from Sleet01 January 8, 2025 21:34
@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.20%. Comparing base (878f61b) to head (339a0a1).
Report is 73 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #5709      +/-   ##
============================================
+ Coverage     10.03%   10.20%   +0.16%     
- Complexity     6069     6073       +4     
============================================
  Files          1074     1010      -64     
  Lines        141525   139327    -2198     
  Branches      20666    20443     -223     
============================================
+ Hits          14209    14220      +11     
+ Misses       125944   123735    -2209     
  Partials       1372     1372              

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

Copy link
Collaborator

@Sleet01 Sleet01 left a comment

Choose a reason for hiding this comment

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

Looks good!

@Sleet01
Copy link
Collaborator

Sleet01 commented Jan 9, 2025

@IllianiCBT Hit a conflict after merging the prior patches.

# Conflicts:
#	MekHQ/src/mekhq/gui/stratcon/StratconScenarioWizard.java
# Conflicts:
#	MekHQ/src/mekhq/gui/dialog/AutoResolveChanceDialog.java
@IllianiCBT IllianiCBT merged commit 6de8158 into MegaMek:master Jan 9, 2025
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.

3 participants