Skip to content

Clean up naming & optimize theme#1288

Open
GeneralProtectionFault wants to merge 2 commits into
Redot-Engine:masterfrom
GeneralProtectionFault:theme_naming_cleanup
Open

Clean up naming & optimize theme#1288
GeneralProtectionFault wants to merge 2 commits into
Redot-Engine:masterfrom
GeneralProtectionFault:theme_naming_cleanup

Conversation

@GeneralProtectionFault

@GeneralProtectionFault GeneralProtectionFault commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

This is sorta basic, but when switching to at least one of the themes, we'd get this error:

  ERROR: editor/themes/editor_theme_manager.cpp:2665 - Condition "EditorSettings::is_default_text_editor_theme(theme_name.get_file().to_lower())" is true.

This is because one of the theme names was left out of the logic, and they were just hard-coded strings.
This PR fixes that bug and puts them in objects so it's not so fragile (basically, puts the actual theme names in 1 place), and addresses a couple of the todos that were in there.

#668 - I'm not sure if this issue is caused by the same thing, perhaps the text theme not switching correctly if the error was thrown, but I'm mentioning it here since it's in the same ballpark.

Summary by CodeRabbit

  • New Features
    • Improved consistency for editor theme presets and built-in text editor themes, including handling and import rules for custom .tet themes.
  • Bug Fixes
    • Theme generation now more accurately detects when fonts and icons need regeneration, reducing stale or unnecessary updates.
    • Editor theme styling can refresh only the changed subsystems instead of rebuilding everything.
    • Unknown theme preset names now reliably fall back to the default preset.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f2bb756b-2dc1-4c0d-b6b6-f8552b326c27

📥 Commits

Reviewing files that changed from the base of the PR and between 15c5368 and 34796cd.

📒 Files selected for processing (2)
  • editor/script/script_editor_plugin.cpp
  • editor/themes/editor_theme_manager.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • editor/themes/editor_theme_manager.cpp

Walkthrough

Replaces hardcoded text editor theme names and interface/spacing preset if/else chains with static data tables. Adds subsystem-level outdated tracking and hash-based font comparison to support partial theme regeneration.

Changes

Editor theme table-driven refactor and partial regeneration

Layer / File(s) Summary
Theme and outdated-state contracts
editor/settings/editor_settings.h, editor/themes/editor_theme_manager.h
Adds BuiltinTextEditorTheme with a color-provider function pointer and manual override flag, declares BUILTIN_TEXT_EDITOR_THEMES[], and adds OutdatedSubsystems with a static inline instance.
Builtin text editor theme table usage
editor/settings/editor_settings.cpp
is_default_text_editor_theme() and update_text_editor_themes_list() iterate BUILTIN_TEXT_EDITOR_THEMES instead of matching hardcoded strings.
Interface and spacing preset tables
editor/themes/editor_theme_manager.cpp
Introduces InterfaceThemePreset and SpacingPreset tables, and _create_theme_config() now looks up presets by name with fallback to Default.
Hash-based icon and font regeneration
editor/themes/editor_theme_manager.cpp
hash_fonts() includes editor font settings, _create_base_theme() merges the old theme first, and icon/font regeneration now depends on hash matches.
Builtin text editor style application
editor/themes/editor_theme_manager.cpp
_populate_text_editor_styles() resolves builtin themes through BUILTIN_TEXT_EDITOR_THEMES, always sets initial values, and conditionally calls set_manually().
Subsystem-level outdated detection
editor/themes/editor_theme_manager.cpp
is_generated_theme_outdated() computes per-subsystem outdated flags and derives outdated_cache from those flags plus font/icon changes.
Builtin theme import validation
editor/script/script_editor_plugin.cpp
Import rejection messages now list builtin theme names from BUILTIN_TEXT_EDITOR_THEMES instead of using a hardcoded forbidden-name string.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% 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 is broadly related, but it is too vague to clearly describe the main theme-editor changes. Use a specific title such as "Centralize built-in text editor themes" or "Refactor theme regeneration and built-in theme handling".
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@editor/settings/editor_settings.cpp`:
- Around line 1795-1809: The reserved-name validation message is outdated
relative to is_default_text_editor_theme() and the BUILTIN_TEXT_EDITOR_THEMES
list, which now also treats “Godot” as reserved. Update the error text in
editor/script/script_editor_plugin.cpp to list all current builtin text editor
theme names that cannot be used, so saving or importing Godot.tet shows the
correct restriction.

In `@editor/themes/editor_theme_manager.cpp`:
- Around line 223-253: Broaden the text-editor theme regeneration check so
_populate_text_editor_styles() runs whenever its inputs change, not only when
outdated_subsystems.text_editor_styles is set. Update regen_text_editor in
EditorThemeManager::generate_theme (or the surrounding regeneration logic) to
also consider interface-theme-derived p_config changes such as UI theme, font,
and icon updates, since this method is responsible for rebinding CodeEdit fonts
and icons and can otherwise leave stale merged editor references behind.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 17bfc1f6-e8dc-4ba4-9c44-3780fae8c29a

📥 Commits

Reviewing files that changed from the base of the PR and between 5702c54 and 15c5368.

📒 Files selected for processing (4)
  • editor/settings/editor_settings.cpp
  • editor/settings/editor_settings.h
  • editor/themes/editor_theme_manager.cpp
  • editor/themes/editor_theme_manager.h

Comment thread editor/settings/editor_settings.cpp
Comment thread editor/themes/editor_theme_manager.cpp
@GeneralProtectionFault

Copy link
Copy Markdown
Contributor Author

@Shakai-Dev This is a bug fix for that error message, and the one CodeRabbit actually called out 😋 that might solve the issue I referred to. I'll fix that as soon as I can and squash it in here.

@GeneralProtectionFault

Copy link
Copy Markdown
Contributor Author

Ok, this is updated to address the hare's rather good points. If this is good to merge, I'd recommend we ask the OP of #668 to test for the behavior whenever it gets into a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Open

Development

Successfully merging this pull request may close these issues.

2 participants