Skip to content

DI PR post-review follow-up 2 - 4 code quality tweaks#5315

Merged
Cervator merged 6 commits into
MovingBlocks:developfrom
SiliconSaga:review/post-di-phase2
Mar 31, 2026
Merged

DI PR post-review follow-up 2 - 4 code quality tweaks#5315
Cervator merged 6 commits into
MovingBlocks:developfrom
SiliconSaga:review/post-di-phase2

Conversation

@Cervator
Copy link
Copy Markdown
Member

Summary

Phase 2 code quality follow-up to #5313, addressing BSA's review comments on #5299.

Changes

  • RegisterRemoteWorldSystems: Remove commented-out world generator code and unused gameManifest field
  • NUIManagerInternal: Rename timedContextForModulesWidgetswidgetContext
  • ConsoleImpl: Inject NetworkSystem directly instead of pulling from Context. Context retained only for lazy PermissionManager lookup (runtime @Share, not available at construction)
  • BlockFamilyLibrary: Replace Context parameter with explicit ReflectFactory and CopyStrategyLibrary in both constructors

Test Plan

  • Engine, engine-tests, and test compilation all pass
  • Local single-player game starts, console works, blocks place/break correctly

🤖 Generated with Claude Code

Cervator and others added 4 commits March 29, 2026 21:43
Remove commented-out world generator initialization code and the now-unused
gameManifest field. The world generator setup was moved to RegisterWorldSystems
during the DI migration.

BSA flagged this on PR MovingBlocks#5299.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rename the nonsensical variable name to something descriptive. This is
a per-module child context used to inject widget-specific dependencies
(TypeWidgetLibrary) during control widget initialization.

BSA flagged this on PR MovingBlocks#5299.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Inject NetworkSystem as an explicit constructor parameter instead of
pulling it from Context. Context is retained only for the lazy
PermissionManager lookup (provided by @share at runtime, not available
at construction time).

BSA flagged this on PR MovingBlocks#5299: "This class does not require a full
context. Inject NetworkSystem and PermissionManager instead."
PermissionManager cannot be fully injected yet due to its lifecycle.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Context parameter with explicit ReflectFactory and
CopyStrategyLibrary parameters in both constructors. The Context was
only used to pull these two dependencies, making it an unnecessary
intermediary.

BSA flagged this on PR MovingBlocks#5299: "We should not be injecting Context
directly unless absolutely necessary."

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 212fac7e-0348-46f7-8966-958033548c3c

📥 Commits

Reviewing files that changed from the base of the PR and between 72d6139 and 63e5595.

📒 Files selected for processing (2)
  • engine/src/main/java/org/terasology/engine/core/modes/StateLoading.java
  • engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterRemoteWorldSystems.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterRemoteWorldSystems.java

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Simplified and clarified startup wiring: reduced hidden dependencies and made constructor/injection paths explicit across world-loading, networking/console integration, UI widget initialization, and block-family setup.
    • Isolated widget initialization context and streamlined remote-world registration to tighten initialization order and reduce implicit context lookups.

Walkthrough

Four classes were refactored to change dependency wiring: RegisterRemoteWorldSystems drops GameManifest, ConsoleImpl now receives NetworkSystem explicitly, BlockFamilyLibrary replaces Context with ReflectFactory and CopyStrategyLibrary, and NUIManagerInternal uses a local widgetContext for widget initialization.

Changes

Cohort / File(s) Summary
Remote world load process
engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/RegisterRemoteWorldSystems.java
Removed GameManifest field and constructor parameter; removed commented seed/WorldGenerator wiring; step() now configures NetworkSystem remote provider, registers BlockEntityRegistry and CelestialSystem, sets renderer camera reflection height, then calls worldRenderer.init().
Console DI update
engine/src/main/java/org/terasology/engine/logic/console/ConsoleImpl.java
Constructor signature changed to ConsoleImpl(NetworkSystem networkSystem, Context context) and assigns networkSystem from the parameter instead of retrieving it from Context; Context remains stored.
Block family DI change
engine/src/main/java/org/terasology/engine/world/block/family/BlockFamilyLibrary.java
Constructors updated to accept ReflectFactory and CopyStrategyLibrary instead of Context; DefaultModuleClassLibrary is instantiated with these explicit dependencies. The @Inject constructor was similarly adjusted.
NUI widget context refactor
engine/src/main/java/org/terasology/engine/rendering/nui/internal/NUIManagerInternal.java
initialiseControlWidget() now creates a local ContextImpl widgetContext, uses it to construct and store TypeWidgetLibraryImpl, and passes widgetContext to InjectionHelper.inject(...) before overlay.initialise(), replacing prior timedContextForModulesWidgets usage.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hopped through constructors, light and keen,
Swapped hidden fetches for a clearer scene.
Contexts pruned and pathways made precise,
Dependencies now neat as a slice.
A little hop — the code feels clean!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'DI PR post-review follow-up 2 - 4 code quality tweaks' is partially related to the changeset, referring to dependency injection refactoring and code quality improvements, but lacks specificity about which components were modified. Consider making the title more specific, such as 'Refactor constructor dependencies for RegisterRemoteWorldSystems, ConsoleImpl, BlockFamilyLibrary, and NUIManagerInternal' to better summarize the primary changes.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description is well-related to the changeset, detailing each of the four code quality improvements with clear explanations of what changed and why, along with verification of testing.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

The GameManifest import was removed along with the dead code, but the
constructor parameter still references the type. Restores the import.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dSystems

BSA review feedback: the GameManifest parameter was left behind after
removing the commented-out code that used it. Remove from constructor
and update the caller in StateLoading.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Cervator Cervator merged commit 2544d5f into MovingBlocks:develop Mar 31, 2026
12 checks passed
@Cervator Cervator deleted the review/post-di-phase2 branch March 31, 2026 22:45
@coderabbitai coderabbitai Bot mentioned this pull request Mar 31, 2026
15 tasks
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.

2 participants