Add "UI Filter" for main window #145
Conversation
- Shift hue, saturation, and brightness of editor without themes - Real "Dynamic Reload" for settings just when settings change - Add support for float values in preferences
📝 WalkthroughWalkthroughPreferences window no longer emits SettingsChanged on close; SettingOption guards empty keys and refactors float handling; Settings.set_setting gains a force_silent option to suppress signals; a new HSV UI filter shader and overlay ColorRect were added and Core updates its shader uniforms on settings reload. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (UI)
participant Pref as Preferences Window
participant Settings as Settings (autoload)
participant Core as Core/Main
participant Overlay as OverlayShader (ColorRect)
User->>Pref: change setting or close window
Pref->>Settings: set_setting(section,key,value,force_silent?)
alt value unchanged
Settings-->>Pref: return (no write, no emit)
else save failed
Settings-->>Pref: return (save failed)
else save succeeded
alt force_silent == false
Settings->>Settings: emit Signals.settings_changed
Settings->>Core: settings_changed triggers reload handler
Core->>Core: reload mode, read UI presets
Core->>Overlay: update shader uniforms (hue_shift,saturation,brightness)
else force_silent == true
Settings-->>Core: no settings_changed emitted (silent)
end
end
Note right of Pref: Closing preferences now calls queue_free() directly (no manual settings_changed emit)
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/README.md (1)
28-57: Fix markdown formatting issues in the code example.The test generation guidance code block has two formatting issues:
- Line 29: Missing language identifier for the fenced code block
- Lines 43-53: Hard tabs should be replaced with spaces for consistency
Apply this diff to fix the formatting:
-- Don't use local variables in lambda functions, use like this (`test_neme__var_name`): -``` +- Don't use local variables in lambda functions, use like this (`test_name__var_name`): +```gdscript # GdUnit generated TestSuite class_name UIFilterIntegrationTestSuite extends GdUnitTestSuite @@ -41,13 +41,13 @@ ... func test_complete_ui_filter_workflow() -> void: - # Test complete workflow: setting -> signal -> reload - var connection := func(): test_complete_ui_filter_workflow__signal_emitted = true - Signals.settings_changed.connect(connection) - - # Change hue shift - Settings.set_setting(test_section, "filter_hue_shift", 0.5) - await get_tree().process_frame - assert_bool(test_complete_ui_filter_workflow__signal_emitted).is_true() - assert_float(Settings.get_setting(test_section, "filter_hue_shift")).is_equal(0.5) - - Signals.settings_changed.disconnect(connection) + # Test complete workflow: setting -> signal -> reload + var connection := func(): test_complete_ui_filter_workflow__signal_emitted = true + Signals.settings_changed.connect(connection) + + # Change hue shift + Settings.set_setting(test_section, "filter_hue_shift", 0.5) + await get_tree().process_frame + assert_bool(test_complete_ui_filter_workflow__signal_emitted).is_true() + assert_float(Settings.get_setting(test_section, "filter_hue_shift")).is_equal(0.5) + + Signals.settings_changed.disconnect(connection) ...Note: Also fixed typo "test_neme__var_name" → "test_name__var_name" on line 28.
tests/action_scripts/scenes/marketplace_test.gd (2)
17-25: Marketplace tests currently exercise onlySettings, notmarketplace.gdbehaviorAll tests here operate directly on
Settings.set_setting/Signals.settings_changedand never instantiateres://action_scripts/scenes/marketplace.gdor call its_change_theme()helper. That means these tests won’t catch regressions where_change_theme()starts emitting the signal manually again or stops delegating toSettings.set_setting.If you want this suite to guard marketplace-specific behavior, consider:
- Instantiating the marketplace scene/script, and
- Calling
_change_theme()(or the public equivalent) in these tests, asserting:
- The theme setting is updated via
Settings, and- Only one
settings_changedemission occurs per theme change.Otherwise, you may want to rename or relocate these tests to make it clear they’re exercising the Settings API contract rather than the marketplace scene itself.
Also applies to: 26-99
17-25: Clarify or implement theme restoration in hooks
before_test()is currently a stub with a comment about storing the original theme, whileafter_test()always callsSettings.restore_default("editor_ui", "theme_name")when a preset exists. That can permanently reset the editor’s theme to its preset default when tests run against a real user profile.Two options:
- Implement actual save/restore in the hooks (capture the original theme and set it back), or
- If the test environment always uses an isolated config, simplify by removing the “Store original theme” comment to avoid implying behavior that doesn’t exist.
tests/core/autoload/settings_test.gd (1)
13-22: Settings signal/early‑return behavior is well covered; consider improving the save‑error testThe added fields and tests accurately exercise the updated
Settings.set_settingcontract:
- Early return when setting the same value (string, numeric, boolean, float).
- Signal emission on actual value changes.
force_silentsuppressingSignals.settings_changed, including the explicitfalsecase.restore_defaultemitting when it changes a value and remaining silent when already at default.This suite should catch most regressions around the new
force_silentparameter and early‑return logic.One gap is
test_set_setting_does_not_emit_on_save_error(), which currently justpasses with a comment. As written, it can’t fail and gives the impression that the save‑error path is tested when it isn’t. Consider either:
- Implementing a small seam (e.g., injectable file API or temporary invalid path) to force a save failure and assert that no signal is emitted, or
- Removing the test (or marking it explicitly as a documentation‑only placeholder) to avoid a false sense of coverage.
Also applies to: 139-269
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
action_scripts/scenes/setting_option.gd(1 hunks)action_scripts/scenes/setting_option.tscn(1 hunks)core/autoload/settings.gd(2 hunks)tests/INDEX.md(1 hunks)tests/README.md(2 hunks)tests/TEST_SUMMARY.md(1 hunks)tests/action_scripts/scenes/marketplace_test.gd(1 hunks)tests/action_scripts/scenes/marketplace_test.gd.uid(1 hunks)tests/action_scripts/scenes/preferences_test.gd(1 hunks)tests/action_scripts/scenes/preferences_test.gd.uid(1 hunks)tests/action_scripts/scenes/setting_option_test.gd(1 hunks)tests/action_scripts/scenes/setting_option_test.gd.uid(1 hunks)tests/core/autoload/backup_core_test.gd(1 hunks)tests/core/autoload/settings_test.gd(2 hunks)tests/core/main_test.gd(1 hunks)tests/core/main_test.gd.uid(1 hunks)tests/core/shader_validation_test.gd(1 hunks)tests/core/shader_validation_test.gd.uid(1 hunks)tests/core/ui_filter_integration_test.gd(1 hunks)tests/core/ui_filter_integration_test.gd.uid(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- tests/action_scripts/scenes/setting_option_test.gd.uid
- tests/core/main_test.gd.uid
- tests/core/ui_filter_integration_test.gd.uid
- tests/action_scripts/scenes/marketplace_test.gd.uid
- tests/action_scripts/scenes/preferences_test.gd.uid
- tests/core/shader_validation_test.gd.uid
🚧 Files skipped from review as they are similar to previous changes (1)
- action_scripts/scenes/setting_option.gd
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-04T06:30:54.607Z
Learnt from: mkh-user
Repo: text-forge/text-forge PR: 145
File: core/main.gd:149-152
Timestamp: 2025-12-04T06:30:54.607Z
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/main_test.gdtests/core/shader_validation_test.gd
🪛 markdownlint-cli2 (0.18.1)
tests/README.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
51-51: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (15)
tests/INDEX.md (1)
18-37: LGTM! Documentation accurately reflects test expansion.The updated test counts align with the expanded test coverage across Action Scripts, Autoloads, and Core test suites introduced in this PR.
tests/TEST_SUMMARY.md (1)
9-43: LGTM! Test summary accurately documents expanded coverage.The updated metrics and new test file entries properly reflect the comprehensive test expansion introduced in this PR, including the new UI Filter and Settings API test coverage.
tests/action_scripts/scenes/preferences_test.gd (3)
14-16: LGTM! Clean test setup.The before_test hook properly instantiates the Preferences window from the scene file and adds it to the test tree.
30-38: LGTM! Validates the new close behavior.This test correctly verifies that the close_requested signal is now connected to queue_free, aligning with the PR change to remove the manual signal emission handler.
40-52: Excellent test for the key behavioral change.This test validates that closing the Preferences window no longer manually emits settings_changed, which is the core change in this PR. The signal should now only be emitted by Settings.set_setting().
action_scripts/scenes/setting_option.tscn (1)
67-67: LGTM! Enables float precision in preferences.The step value of 0.01 provides appropriate granularity for float settings, aligning with the PR's goal to add float value support in preferences.
tests/core/autoload/backup_core_test.gd (1)
81-81: LGTM! Improves type safety.The explicit type annotation enhances code clarity and leverages GDScript's static typing.
core/autoload/settings.gd (1)
98-112: LGTM! Well-designed Settings API improvements.The changes introduce three valuable enhancements:
- force_silent parameter: Enables suppression of settings_changed signal for batch operations or internal updates
- Early return on unchanged value (lines 99-101): Prevents unnecessary file I/O and signal emission when the value hasn't changed
- Early return on save error (line 110): Correctly prevents signal emission when persistence fails
The logic correctly handles both existing keys (with change detection) and new keys (which always proceed to save).
tests/action_scripts/scenes/setting_option_test.gd (4)
14-30: LGTM! Proper test isolation with setup/teardown.The before_test and after_test hooks correctly clean up test settings and presets, ensuring test independence.
44-110: Excellent coverage of numeric type handling.The test suite comprehensively validates both integer and float settings, including important edge cases:
- Boolean toggle (true/false with UI label changes)
- Standard integer values
- Float values with various edge cases (zero, negative, large values)
This ensures robust float support introduced in this PR.
139-197: LGTM! Thorough UI interaction validation.The tests correctly verify that UI interactions (checkbox presses, spinbox changes, text submissions) properly update the underlying Settings backend, including array parsing with whitespace trimming.
199-207: Good defensive test for unsupported types.This test verifies graceful degradation when an unsupported type (dictionary) is used, confirming the component properly queues itself for deletion rather than failing silently or crashing.
tests/core/ui_filter_integration_test.gd (1)
18-27: UI filter integration tests look consistent with Settings + signal semanticsThe suite does a good job exercising:
- Signal emission vs.
force_silentbehavior,- Multiple sequential updates,
- Default restoration and edge ranges for the filter settings, and
- Independence of UI filter values from theme changes.
The only minor thing to watch is that
after_test()callsSettings.restore_defaultfor the UI filter keys without checking whether presets exist; ifrestore_defaultever starts asserting on missing presets, these tests will need corresponding guards (as you already do inMainTestSuite.after_test). Otherwise, the structure and coverage here look solid.Also applies to: 28-183
tests/core/main_test.gd (1)
16-24: Main UI filter tests match the intended behavior and project testing styleThis suite nicely documents:
- Preset presence and default values for
filter_hue_shift,filter_saturation,filter_brightness, andtheme_name,- Accepted value ranges (including extremes), and
- That UI filter settings are independent and resettable via
restore_default.The guarded
restore_defaultcalls inafter_test()also avoid issues when presets are absent. Based on learnings, these tests align well with the project’s preference to validate configuration and nodes (e.g., overlay/filter) via tests rather than runtime null checks incore/main.gd.One thing to keep in mind: tests like
test_theme_default_value()hard‑code"dark"as the expected default. If you ever introduce “system” or OS‑driven defaults, you’ll need to update these assertions accordingly.Also applies to: 25-170
tests/core/shader_validation_test.gd (1)
10-138: Shader validation tests are thorough; string checks may need maintenance over timeThese tests give strong structural guarantees around
res://core/main.gdshaderand its UID: existence, load success, uniforms, helper functions, fragment entry point, shader type, and parameter wiring throughShaderMaterialandColorRect. This fits well with the repo’s pattern of asserting required rendering pieces via tests instead of adding runtime null/defensive checks incore/main.gd. Based on learnings, this is the right place to encode those assumptions.Be aware that tests using
code.contains("...")on the shader source are somewhat brittle with respect to formatting or refactors to function signatures/uniform declarations. If you later reformat or restructure the shader, remember to adjust these substrings accordingly to keep the suite green.
Summary of Changes
Related Items
Themes can change original view of editor, but this UI Filter changes whole colors (just like a filter)
Technical Details & Testing
Signals.settings_changedemissions removed, currentlySettingsAPI emits this signal inset_setting().floatsetting values added as SpinBox.force_silentproperty added toSettings.set_setting()to avoidSignals.settings_changedemissions when needed.overlay_filteradded to main window of editor to apply UI Filter.hue_shift,saturation, andbrightnessadded:hsbvalues from settings.HSV.RGB.alphachannel`).Checklist
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.