Fix: checkable menu options not toggling correctly #146

mkh-user · 2025-12-06T14:20:53Z

Summary of Changes

After #145, Settings.set_setting() emits Signals.settings_changed. This leads to duble change in CheckableActionScripts because this class calls set_setting and then _set_value to update status but emission of Signals.settings_changed calls _set_value in first call, with this duplicate call all ChechableActionScripts shows an unexpected behavior.

Related Items

Generated from Add "UI Filter" for main window #145

Technical Details & Testing

Old logic:

User click -> _run_action() -> Settings.set_settings() -> Signals.settings_changed -> _set_value() (#1)
                            -> _set_value() (#2)

New logic:

User click -> _run_action() -> Settings.set_settings() -> Signals.settings_changed -> _set_value()

Checklist

Linked all relevant issues/PRs/discussions
Code follows project style and guidelines
Self-reviewed and tested thoroughly
Documentation updated (if applicable)
No new warnings or errors introduced
Relevant changes added to CHANGELOG

Summary by CodeRabbit

Bug Fixes
- Fixed checkable menu toggles; corrected issues causing toggles to mark files as unsaved and reset-font-size not loading configuration.
Documentation
- Updated changelog: corrected PR references and added new "Fixed" entries.
Tests
- Expanded test suite with new class-level and changelog validation tests, increasing test counts and total runtime.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-06T14:21:03Z

📝 Walkthrough

Walkthrough

Removed immediate internal state update in checkable action handling, updated CHANGELOG.md Unreleased entries, and added comprehensive tests: CheckableActionScript unit tests and a changelog validation test suite; test indices and summaries updated.

Changes

Cohort / File(s)	Change Summary
Core change `core/classes/action_script_extension/checkable_action_script.gd`	Removed the direct `_set_value(not _get_value())` call from `_run_action`, leaving only the settings update and deferring internal state synchronization to other mechanisms (no public API changes).
Changelog `CHANGELOG.md`	Edited Unreleased section: updated API FileDatabase PR reference number and added several Fixed entries referencing PR 139 (Move Lines, Reset Font Size, Toggle Bookmark, checkable menu toggles).
Tests — indexes & summaries `tests/INDEX.md`, `tests/README.md`, `tests/TEST_SUMMARY.md`	Updated test counts, execution time estimates, and added new test categories ("Classes Tests" and "Docs Tests"); adjusted totals and timings.
Tests — CheckableActionScript `tests/core/classes/action_script_extension/checkable_action_script_test.gd`, `tests/core/classes/action_script_extension/checkable_action_script_test.gd.uid`	Added `CheckableActionScriptTestSuite` and `TestCheckableActionScript` with extensive tests covering setup, config loading, toggling behavior, signals, menu-state synchronization, edge cases, and cross-instance propagation; added UID file.
Tests — changelog validation `tests/docs/changelog_validation_test.gd`, `tests/docs/changelog_validation_test.gd.uid`	Added `ChangelogValidationTestSuite` validating `CHANGELOG.md` structure, formatting, PR references, links, dates, duplicates, and content quality; added UID file.

Possibly related PRs

text-forge/text-forge — related PR 136 (touches CheckableActionScript-based action scripts).

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main bug fix: checkable menu options not toggling correctly, which is directly addressed by the code changes removing the duplicate _set_value call.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch mkh-user/fix-chechable-action-scripts

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b91612 and 82fa26f.

📒 Files selected for processing (1)

tests/core/classes/action_script_extension/checkable_action_script_test.gd (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-04T06:30:54.628Z

Learnt from: mkh-user
Repo: text-forge/text-forge PR: 145
File: core/main.gd:149-152
Timestamp: 2025-12-04T06:30:54.628Z
Learning: In the text-forge codebase (core/main.gd), the project prefers to use unit test assertions to verify that required scene nodes like overlay_shader are properly configured, rather than adding runtime null checks in the code.

Applied to files:

tests/core/classes/action_script_extension/checkable_action_script_test.gd

🔇 Additional comments (6)

tests/core/classes/action_script_extension/checkable_action_script_test.gd (6)

17-43: LGTM! Test class implementation is well-designed.

The TestCheckableActionScript class properly extends CheckableActionScript and implements comprehensive tracking for test validation. The previous concern about custom_default being typed as bool but compared to null has been correctly addressed by making it an untyped Variant (line 26), which allows null as a valid value.

Based on learnings, the text-forge codebase prefers to use unit test assertions rather than runtime null checks, which aligns with this test-focused approach.

45-66: LGTM! Proper test isolation and cleanup.

The before_test and after_test methods correctly ensure test isolation by cleaning up Settings data before and after each test. The use of auto_free() for proper memory management and adding the test menu as a child for scene tree integration are appropriate practices.

190-209: LGTM! Critical test validates the PR #146 fix.

This test correctly validates the core fix for PR #146: _run_action() should not directly call _set_value(), but should instead rely on the settings_changed signal to trigger _load_config(), which then calls _set_value() once. The test properly resets the counter after initialization and asserts exactly one call, preventing the duplicate invocation that caused the toggle bug.

153-223: LGTM! Comprehensive coverage of run action behavior.

The run action tests thoroughly validate the expected behavior:

Toggle operations in both directions

_get_value() is called to determine current state

_set_value() is called exactly once via signal (not directly)

settings_changed signal is properly emitted

This test section provides strong validation of the corrected toggle flow introduced in PR #146.

315-436: LGTM! Excellent integration and edge case coverage.

The integration tests validate complete workflows including full toggle cycles and cross-instance signal propagation. The rapid toggle test (lines 400-414) is particularly valuable for ensuring that under stress conditions, each toggle still triggers exactly one _set_value() call—a key regression check for the PR #146 fix. Edge cases appropriately cover uninitialized settings, missing configuration, and null menu scenarios.

1-452: LGTM! Comprehensive and well-structured test suite.

This is an excellent test suite that provides thorough coverage of CheckableActionScript functionality:

Clear organization: Tests are logically grouped into sections (initialization, setup, run action, load config, virtual methods, integration, edge cases, properties)

Strong PR Fix: checkable menu options not toggling correctly #146 validation: Multiple tests specifically validate the fix, ensuring _run_action() no longer directly calls _set_value(), preventing duplicate updates

Comprehensive coverage: Tests span initialization, configuration loading, signal handling, toggle operations, and edge cases

Proper test isolation: Setup and teardown ensure tests don't interfere with each other

Stress testing: Rapid toggle tests help catch potential race conditions or duplicate call issues

The test suite provides confidence that the checkable action toggle behavior is now correct and will remain correct through future changes.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Fix: checkable menu options not toggling correctly

3d7cf10

github-project-automation bot added this to Release: Text Forge 1.0 Dec 6, 2025

github-project-automation bot moved this to Needs Review in Release: Text Forge 1.0 Dec 6, 2025

mkh-user self-assigned this Dec 6, 2025

mkh-user added action scripts Official action scripts or action scripts features bug A confirmed issue causing incorrect behavior or unexpected results labels Dec 6, 2025

mkh-user moved this from Needs Review to In Progress in Release: Text Forge 1.0 Dec 6, 2025

This comment was marked as resolved.

Sign in to view

Add Unit Tests

801ab78

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

mkh-user added this to the Text Forge 0.2 milestone Dec 6, 2025

This comment was marked as resolved.

Sign in to view

Fix some issues for #146

4b91612

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

This comment was marked as resolved.

Sign in to view

Fix some issues for #146

82fa26f

Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

mkh-user merged commit f81512e into Main Dec 6, 2025
2 checks passed

mkh-user deleted the mkh-user/fix-chechable-action-scripts branch December 6, 2025 19:52

github-project-automation bot moved this from In Progress to Completed in Release: Text Forge 1.0 Dec 6, 2025

coderabbitai bot mentioned this pull request Dec 22, 2025

Add support for normalizing line endings and related features #171

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix: checkable menu options not toggling correctly #146

Fix: checkable menu options not toggling correctly #146

Uh oh!

mkh-user commented Dec 6, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 6, 2025 •

edited

Loading

Walkthrough

Changes

Possibly related PRs

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix: checkable menu options not toggling correctly #146

Fix: checkable menu options not toggling correctly #146

Uh oh!

Conversation

mkh-user commented Dec 6, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Related Items

Technical Details & Testing

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mkh-user commented Dec 6, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 6, 2025 •

edited

Loading