Lumacs/documentation/REVIEW.md

Lumacs Architectural Review

Executive Summary

Lumacs is a well-structured C++20 text editor with a Facade/Mediator pattern centered on EditorCore. The architecture demonstrates solid separation of concerns through interface segregation and dependency injection. However, there are several areas where the design could be improved for scalability, testability, and maintainability.

---

1. Separation of Concerns

Strengths

Interface Segregation is Well-Implemented

• Three core interfaces (ICommandTarget, IEditorNotifier, IWindowManager) effectively decouple components
• BufferManager depends on interfaces (i_editor_notifier.hpp:9-20, i_window_manager.hpp:11-24), not concrete EditorCore
• This enables dependency injection and breaks circular dependencies elegantly

Clear Delegation Pattern

• EditorCore (editor_core.hpp:42-121) delegates to specialized managers
• Each operation routes to the appropriate manager (e.g., kill_line() → KillRingManager)

Issues

EditorCore Is Becoming a God Object

• EditorCore implements 3 interfaces with ~80 public methods (editor_core.hpp:32-177)
• It holds 15+ member pointers (editor_core.hpp:190-206)
• Constructor is 119 lines with complex initialization dependencies (editor_core.cpp:19-119)

Circular Dependency Workaround Is Fragile

// editor_core.cpp:105-106
buffer_manager_ = std::make_unique<BufferManager>(*this, *this);
window_manager_ = std::make_unique<WindowManager>(*this, *buffer_manager_);

The extensive comments in editor_core.cpp:36-103 reveal the complexity of this workaround.

Recommendations

1. Split EditorCore into Coordinator + OperationsDelegate

   • EditorCoordinator: Manages lifecycle, initialization, and event routing
   • EditorOperations: Implements ICommandTarget operations

2. Consider a Service Locator for Manager Access

   • Reduce the number of explicit dependencies in constructors
   • Enable lazy initialization

---

2. Manager Proliferation

Current State: 11+ Managers

Manager	Dependencies	Responsibilities
BufferManager	IEditorNotifier, IWindowManager	Buffer lifecycle, file I/O
WindowManager	IEditorNotifier, BufferManager	Window layout tree
KillRingManager	None	Clipboard-like ring buffer
RegisterManager	None	Named text registers
MacroManager	EditorCore	Keyboard macro recording
RectangleManager	EditorCore	Rectangular region operations
CommandSystem	EditorCore, MinibufferManager	Command registration/execution
MinibufferManager	EditorCore, LuaApi, CompletionSystem	Modal input handling
CompletionSystem	EditorCore	Completion candidates
PluginManager	EditorCore, LuaApi	Plugin lifecycle
KeyBindingManager	CommandSystem*	Key sequence handling (Trie)
ThemeManager	None	Theme management
ModelineManager	None	Status line composition

Analysis

Well-Isolated Managers (Good)

• KillRingManager, RegisterManager, ThemeManager, ModelineManager have zero dependencies
• These follow Single Responsibility Principle well

Over-Coupled Managers (Concern)

• MacroManager, RectangleManager take full EditorCore& reference
• MinibufferManager depends on 3 systems (minibuffer_manager.hpp:23)

MinibufferManager Is Overloaded (minibuffer_manager.hpp:21-115)

• Handles multiple modes (Command, FilePath, BufferName, ThemeName, ISearch)
• Manages history, completion, cursor position, search state
• 115 lines of header with mixed responsibilities

Recommendations

1. Extract ISearchManager from MinibufferManager

   • isearch_* members and methods (minibuffer_manager.hpp:100-111) should be separate

2. Create MacroContext/RectangleContext Instead of EditorCore Reference

   • Define narrow interfaces for what these managers actually need

3. Consider Consolidation:

   • Merge KillRingManager and RegisterManager into a unified ClipboardSystem
   • Both deal with text storage/retrieval

---

3. Lua Integration

Architecture

LuaApi (lua_api.hpp:23-101)
  ├── sol::state lua_ (single global state)
  ├── EditorCore* core_ (set via set_core())
  ├── register_types() → exposes C++ usertypes
  └── register_functions() → global Lua functions

Strengths

Comprehensive Type Exposure (lua_api.cpp:390-898)

• Position, Range, Buffer, Window, EditorCore usertypes
• Enums for ThemeElement, FontWeight, MinibufferMode
• Callback support for buffer events (lua_api.cpp:662-670)

Command System Integration

// lua_api.cpp:764-823
"bind_key", [this](EditorCore& core, std::string key, sol::object callback_or_cmd, ...)

• Keys can bind to either command names or Lua functions
• Transparent interop between C++ commands and Lua callbacks

Issues

Single Global Lua State - Plugin Isolation Risk

// lua_api.hpp:89
sol::state lua_;

• All plugins share the same Lua environment
• One misbehaving plugin can corrupt global state
• No sandboxing for untrusted plugins

LuaApi Constructor Before Core Assignment (lua_api.cpp:58-69, lua_api.cpp:71-74)

LuaApi::LuaApi() { lua_.open_libraries(...); }
void LuaApi::set_core(EditorCore& core) { core_ = &core; setup_api(); }

• set_core() must be called before API is usable
• No enforcement at compile time

Error Handling Is Log-Only (lua_api.cpp:83-86)

} catch (const sol::error& e) {
    spdlog::error("Lua error loading {}: {}", path.string(), e.what());
    return false;
}

• Errors are logged but not propagated to UI
• Users may not see why their config failed

Duplicated Callback Wrapper Code

• lua_api.cpp:143-182, lua_api.cpp:777-810, lua_api.cpp:834-868 contain nearly identical callback wrapping logic

Recommendations

1. Plugin Sandboxing with Separate Environments

   sol::environment plugin_env(lua_, sol::create, lua_.globals());
   

2. Compile-Time Core Requirement

   • Make EditorCore& a constructor parameter, or use RAII pattern

3. Extract Callback Wrapper Helper

   CommandResult wrap_lua_callback(sol::function& callback, CommandContext& context);
   

4. User-Visible Error Reporting

   • Push Lua errors to *Messages* buffer or minibuffer

---

4. UI Layer Abstraction

Interface Design (ui_interface.hpp:38-59)

class IEditorView {
    virtual void init() = 0;
    virtual void run() = 0;
    virtual void handle_editor_event(EditorEvent event) = 0;
    virtual void set_core(EditorCore* core) = 0;
};

Strengths

Clean Factory Pattern (gtk_editor.hpp:81, tui_editor.cpp:761-763)

std::unique_ptr<IEditorView> create_gtk_editor();
std::unique_ptr<IEditorView> create_tui_editor();

Conditional Compilation (main.cpp:52-66)

#ifdef LUMACS_WITH_GTK
    if (!force_tui) { view = create_gtk_editor(); }
#endif

Issues

Event Handling Has Redundant Dispatching

Both TuiEditor::handle_editor_event() (tui_editor.cpp:159-256) and GtkEditor::handle_editor_event() have nearly identical switch statements for mode activation:

// tui_editor.cpp:171-183
} else if (event == EditorEvent::CommandMode) {
    core_->minibuffer_manager().activate_minibuffer(
        MinibufferMode::Command, ":",
        [this](const std::string& input) { ... },
        [this]() { core_->set_message("Cancelled"); }
    );
}

This is duplicated for BufferSwitchMode, KillBufferMode, FindFileMode, etc.

Raw Pointer to Core

// gtk_editor.hpp:43, tui_editor.cpp:35
EditorCore* core_ = nullptr;

• No ownership semantics expressed
• set_core() must be called before init()

TUI Implementation In .cpp

• TuiEditor class is defined entirely in tui_editor.cpp:20-60
• Inconsistent with GtkEditor which has a header

Recommendations

1. Extract Common Mode Activation Logic

   class MinibufferModeHandler {
      void handle_mode_event(EditorEvent event, EditorCore& core, ...);
   };
   

2. Use Non-Owning Smart Pointer

   std::reference_wrapper<EditorCore> core_;
   

3. Create TUI Header for Consistency

   • Move class definition to tui_editor.hpp

---

5. Event System

Current Implementation (ui_interface.hpp:12-32, editor_core.hpp:143-145)

enum class EditorEvent {
    BufferModified, CursorMoved, ViewportChanged, WindowLayoutChanged,
    WindowFocused, Message, CommandMode, BufferSwitchMode, ...
};

using EventCallback = std::function<void(EditorEvent)>;
void on_event(EventCallback callback);

Strengths

• Simple, synchronous event dispatch
• Clean separation via callback registration

Issues

No Event Data Payload

void EditorCore::emit_event(EditorEvent event) {
    for (const auto& callback : event_callbacks_) {
        callback(event);  // Only event type, no context
    }
}

• Observers must query EditorCore for related data
• Race conditions possible in multi-threaded scenarios

O(n) Callback Iteration

std::vector<EventCallback> event_callbacks_;

• Fine for current use (typically 1 UI), but doesn't scale

Modal Events Mix Control and Data

• CommandMode, ISearchMode are events that trigger UI state changes
• Different semantic level from BufferModified, CursorMoved

Recommendations

1. Add Event Payload

   struct EditorEventData {
      EditorEvent type;
      std::variant<BufferModifiedData, CursorMovedData, ...> payload;
   };
   

2. Consider Observer Pattern with Typed Handlers

   void on_buffer_modified(std::function<void(const BufferModifiedData&)> handler);
   void on_cursor_moved(std::function<void(Position)> handler);
   

3. Separate Modal Events into Different Mechanism

   • Use direct method calls for mode transitions
   • Reserve events for state change notifications

---

6. Plugin/Extension System

Current Implementation (plugin_manager.hpp:22-59)

class PluginManager {
    void discover_plugins(const std::vector<std::filesystem::path>& plugin_dirs);
    bool load_plugin(const std::string& name);
    bool unload_plugin(const std::string& name);
};

Strengths

• Simple file-based plugin discovery
• Optional on_load()/on_unload() lifecycle hooks

Issues

No Plugin Metadata

struct Plugin {
    std::string name;
    std::filesystem::path path;
    // No: version, dependencies, compatibility info
};

No Plugin Isolation

• All plugins run in shared sol::state
• No capability restrictions
• Plugins can access filesystem, network via Lua stdlib

No Plugin Dependency Resolution

• Plugins loaded in discovery order
• No mechanism for plugin A to require plugin B

Unload Is Incomplete

void execute_on_unload(const Plugin& plugin);

• Calls Lua on_unload() but doesn't clean up:
  • Registered commands
  • Key bindings
  • Event handlers

Recommendations

1. Plugin Manifest

   -- plugin.lua
   return {
      name = "my-plugin",
      version = "1.0.0",
      requires = {"core >= 0.1.0"},
      on_load = function() ... end
   }
   

2. Capability-Based Sandboxing

   sol::environment safe_env(lua_, sol::create);
   safe_env["editor"] = core_subset;  // Limited API
   

3. Plugin Registry for Cleanup

   • Track commands/bindings registered by each plugin
   • Auto-cleanup on unload

---

7. State Management

Current State Distribution

Component	State Owned
BufferManager	std::list<shared_ptr<Buffer>>
WindowManager	shared_ptr<LayoutNode>, shared_ptr<Window>
Buffer	Content, undo stack, mark, styles
Window	Cursor, viewport, buffer reference
KillRingManager	std::deque<std::string>
MinibufferManager	Mode, input, history, completion
MacroManager	Recording state, macro content

Issues

Shared Mutable State via shared_ptr

// buffer_manager.hpp:78
std::list<std::shared_ptr<Buffer>> buffers_;

• Multiple windows can reference same Buffer
• No synchronization primitives
• Buffer modification from any path

Undo Is Buffer-Local Only

• No transaction semantics for multi-manager operations
• Kill-then-yank is two unrelated operations from undo perspective

YankState in EditorCore (editor_core.hpp:187-188)

std::optional<Position> last_yank_start_;
std::optional<Position> last_yank_end_;

• Implementation detail leaking into core class
• Should be managed by KillRingManager or a YankContext

Recommendations

1. Consider Copy-on-Write for Buffer Content

   • Enables efficient snapshotting
   • Safer for potential future threading

2. Centralized Undo Stack

   class UndoManager {
      void begin_transaction();
      void commit_transaction();
      void rollback_transaction();
   };
   

3. Move Yank State to KillRingManager

   • Keep yank position tracking with kill ring logic

---

8. Testability

Current Test Structure (tests/test_editor_core.cpp)

struct EditorCoreTest : public ::testing::Test {
    lumacs::EditorCore core;  // Full system initialization
};

Strengths

• Comprehensive test files for each manager
• Integration tests in test_integration.cpp
• Fixture-based setup with GoogleTest

Issues

Tests Create Full EditorCore

lumacs::EditorCore core;

• Every test initializes:
  • All 11+ managers
  • Lua VM
  • Theme system
  • Loads embedded defaults
• Slow and couples tests to entire system

No Mock Implementations

• Interfaces exist (ICommandTarget, IEditorNotifier) but no mocks
• Hard to test managers in isolation

LuaApi Test Depends on Disk (test_lua_api.cpp)

• Tests that load files can fail based on environment
• No embedded test scripts

Comments Acknowledge Limitations (test_editor_core.cpp:103-106)

// Executing the macro will depend on process_key being called
// which is hard to test without mocking keybinding_manager directly.
// For now, just test recording state.

Recommendations

1. Create Mock Implementations

   class MockEditorNotifier : public IEditorNotifier { ... };
   class MockCommandTarget : public ICommandTarget { ... };
   

2. Manager-Level Test Fixtures

   struct KillRingManagerTest : public ::testing::Test {
      MockEditorNotifier notifier;
      KillRingManager manager;  // No EditorCore needed!
   };
   

3. Embedded Lua Test Scripts

   static const char* TEST_LUA_SCRIPT = R"lua(...)lua";
   

---

9. Performance Considerations

Current Bottlenecks

Key Processing Trie Lookup (keybinding.hpp:185-210)

struct TrieNode {
    std::map<Key, std::unique_ptr<TrieNode>> children;
};

• std::map means O(log n) per key in sequence
• Fine for typical Emacs sequences, but std::unordered_map would be O(1)

Syntax Highlighting Per-Render (tui_editor.cpp:521-572)

const auto& styles = buffer.get_line_styles(buffer_line_idx);

• Styles retrieved and iterated per line, per frame
• For large files, this could be expensive

Event Callback Vector Copy (editor_core.cpp:757-760)

for (const auto& callback : event_callbacks_) {
    callback(event);
}

• Safe iteration, but callbacks could modify the vector

No Line Caching in Buffer

• buffer.line(idx) constructs string view each time
• No memoization for frequently accessed lines

Recommendations

1. Use std::unordered_map for Trie Nodes

   • With custom hash for Key

2. Cache Visible Lines' Styles

   • Invalidate on buffer modification in visible range

3. Consider Rope or Gap Buffer for Large Files

   • Current std::vector<std::string> is O(n) for mid-file insertions

---

10. Maintainability

Strengths

• Consistent naming conventions
• Doxygen-style comments on public interfaces
• Clear header/implementation separation
• Logical file organization

Issues

Large Files

• lua_api.cpp: 1031 lines
• editor_core.cpp: 763 lines
• tui_editor.cpp: 765 lines

Constructor Comments Reveal Design Debt (editor_core.cpp:27-103)

• 76 lines of comments explaining circular dependency workaround
• Indicates architectural complexity

Inconsistent Header Guards

• Mix of #pragma once (good) throughout

Debug Code in Production

// tui_editor.cpp:14-15
extern std::ofstream debug_log;
std::ofstream debug_log("lumacs_debug.log");

• Creates log file unconditionally

Recommendations

1. Split Large Files

   • lua_api.cpp → lua_types.cpp, lua_functions.cpp, lua_api.cpp
   • editor_core.cpp → core + operations

2. Remove Production Debug Logging

   • Use spdlog with runtime level control

3. Address Design Debt

   • Refactor circular dependency if comments exceed code

---

Summary of Key Recommendations

High Priority

1. Split EditorCore into Coordinator + Operations to address God Object anti-pattern
2. Extract ISearchManager from MinibufferManager
3. Add Plugin Sandboxing using Lua environments
4. Create Mock Implementations for testability

Medium Priority

1. Unified Event Data payloads for type-safe event handling
2. Common Mode Activation handler shared between TUI/GTK
3. Plugin Manifest system with dependency resolution
4. Extract Lua Callback Wrapper to eliminate duplication

Low Priority (Technical Debt)

1. Performance optimizations (Trie hash map, style caching)
2. Split large implementation files
3. Production debug log cleanup

---

Conclusion

Lumacs demonstrates a thoughtful architecture with proper interface segregation and dependency injection patterns. The main concerns are:

1. EditorCore complexity - trending toward a God Object
2. Lua integration security - shared state with no sandboxing
3. Test isolation - full system initialization for each test
4. MinibufferManager overload - too many responsibilities

The foundation is solid, and the suggested improvements would enhance scalability without requiring a complete rewrite. The interface-based design already in place makes incremental refactoring feasible.