# 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**
```cpp
// 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**
```cpp
// 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**
```cpp
// 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`)
```cpp
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`)
```cpp
} 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**
   ```cpp
   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**
   ```cpp
   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`)

```cpp
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`)
```cpp
std::unique_ptr<IEditorView> create_gtk_editor();
std::unique_ptr<IEditorView> create_tui_editor();
```

**Conditional Compilation** (`main.cpp:52-66`)
```cpp
#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:

```cpp
// 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**
```cpp
// 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**
   ```cpp
   class MinibufferModeHandler {
       void handle_mode_event(EditorEvent event, EditorCore& core, ...);
   };
   ```

2. **Use Non-Owning Smart Pointer**
   ```cpp
   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`)

```cpp
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**
```cpp
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**
```cpp
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**
   ```cpp
   struct EditorEventData {
       EditorEvent type;
       std::variant<BufferModifiedData, CursorMovedData, ...> payload;
   };
   ```

2. **Consider Observer Pattern with Typed Handlers**
   ```cpp
   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`)

```cpp
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**
```cpp
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**
```cpp
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**
   ```lua
   -- plugin.lua
   return {
       name = "my-plugin",
       version = "1.0.0",
       requires = {"core >= 0.1.0"},
       on_load = function() ... end
   }
   ```

2. **Capability-Based Sandboxing**
   ```cpp
   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**
```cpp
// 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`)
```cpp
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**
   ```cpp
   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`)

```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**
```cpp
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`)
```cpp
// 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**
   ```cpp
   class MockEditorNotifier : public IEditorNotifier { ... };
   class MockCommandTarget : public ICommandTarget { ... };
   ```

2. **Manager-Level Test Fixtures**
   ```cpp
   struct KillRingManagerTest : public ::testing::Test {
       MockEditorNotifier notifier;
       KillRingManager manager;  // No EditorCore needed!
   };
   ```

3. **Embedded Lua Test Scripts**
   ```cpp
   static const char* TEST_LUA_SCRIPT = R"lua(...)lua";
   ```

---

## 9. Performance Considerations

### Current Bottlenecks

**Key Processing Trie Lookup** (`keybinding.hpp:185-210`)
```cpp
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`)
```cpp
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`)
```cpp
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**
```cpp
// 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

5. **Unified Event Data** payloads for type-safe event handling
6. **Common Mode Activation** handler shared between TUI/GTK
7. **Plugin Manifest** system with dependency resolution
8. **Extract Lua Callback Wrapper** to eliminate duplication

### Low Priority (Technical Debt)

9. **Performance optimizations** (Trie hash map, style caching)
10. **Split large implementation files**
11. **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.