Skip to content

Logo Subsystem – Global State Reduction Refactor#5593

Merged
walterbender merged 1 commit intosugarlabs:masterfrom
vanshika2720:refactor/logo-reduce-global-state
Feb 22, 2026
Merged

Logo Subsystem – Global State Reduction Refactor#5593
walterbender merged 1 commit intosugarlabs:masterfrom
vanshika2720:refactor/logo-reduce-global-state

Conversation

@vanshika2720
Copy link
Contributor

Overview

This refactor improves the Logo subsystem by reducing its reliance on implicit global state through explicit dependency injection. The work is a scoped architectural improvement aligned with Identify and/or fix high-level inconsistencies (#2767).

The goal is to improve maintainability, testability, and clarity without changing existing behavior.

What This Refactor Does

  • Introduces a LogoDependencies abstraction to encapsulate external dependencies:
    • Shared mutable state (instrument registries, effects, filters)
    • Global singletons (e.g. window.widgetWindows)
    • Global utility functions
    • External classes and libraries (Singer, Tone.js, Notation, Synth, StatusMatrix)
  • Updates the Logo constructor to accept injected dependencies
    • Preserves backward compatibility via LogoDependencies.fromGlobals(activity)
  • Replaces direct global access inside Logo with explicit dependency access
  • Refactors affected methods to follow the new dependency model
  • Documents the updated architecture and remaining (encapsulated) globals

Why This Is Necessary (#2767)

Issue #2767 identifies systemic architectural inconsistencies caused by tight coupling, implicit globals, and monolithic design.
This change addresses one focused sub-problem—implicit global state in the Logo subsystem—without attempting a broad rewrite.

Verification

  • Existing tests (js/tests/logo.test.js) pass with no regressions
  • Added new tests (js/tests/logo_dependencies.test.js) to verify initialization with mocked dependencies

Scope and Impact

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2026

✅ All Jest tests passed! This PR is ready to merge.

2 similar comments
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2026

✅ All Jest tests passed! This PR is ready to merge.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2026

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720 vanshika2720 force-pushed the refactor/logo-reduce-global-state branch from ea77cc9 to a78e71e Compare February 9, 2026 04:56
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2026

✅ All Jest tests passed! This PR is ready to merge.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2026

❌ Some Jest tests failed. Please check the logs and fix the issues before merging.

Failed Tests:

logo.test.js

@vanshika2720 vanshika2720 force-pushed the refactor/logo-reduce-global-state branch from f8afd16 to 5a1ebc0 Compare February 9, 2026 05:06
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2026

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720 vanshika2720 force-pushed the refactor/logo-reduce-global-state branch from 5a1ebc0 to 290460f Compare February 9, 2026 05:10
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2026

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720
Copy link
Contributor Author

@walterbender Please have a review on this

@walterbender
Copy link
Member

Maybe you need to rebase? This won't open for me.

@vanshika2720 vanshika2720 force-pushed the refactor/logo-reduce-global-state branch from 290460f to 9a56bb0 Compare February 14, 2026 16:18
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720
Copy link
Contributor Author

Maybe you need to rebase? This won't open for me.

@walterbender Rebased

@walterbender
Copy link
Member

Some other changes crept back in. mespeak was recently removed.

@vanshika2720 vanshika2720 force-pushed the refactor/logo-reduce-global-state branch from 9a56bb0 to 469f44e Compare February 14, 2026 19:04
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720
Copy link
Contributor Author

@walterbender

I’ve rebased onto latest master and removed all mespeak references that had crept back in.
The PR is now strictly limited to the Logo refactor, with a clean diff and all tests passing.

Introduce LogoDependencies for explicit dependency injection in Logo.
Remove meSpeak references following its removal from master.
Aligned with latest master and applied Prettier formatting.
@vanshika2720 vanshika2720 force-pushed the refactor/logo-reduce-global-state branch from 469f44e to ca46d2e Compare February 16, 2026 19:30
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@vanshika2720
Copy link
Contributor Author

@walterbender

I’ve rebased onto latest master and removed all mespeak references that had crept back in.
The PR is now strictly limited to the Logo refactor, with a clean diff and all tests passing.

@walterbender walterbender merged commit aec58f0 into sugarlabs:master Feb 22, 2026
7 checks passed
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