Conversation
…g and cursor management
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…s for GUI rendering
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive GUI performance improvements through a double-buffering architecture and granular window rendering optimizations. The changes introduce a new graphics API layer, enhance the editor and shell to support windowed graphics mode, and add full mouse interaction capabilities including window dragging, resizing, and focus management.
Key changes:
- Implemented double-buffering in the framebuffer layer to eliminate screen tearing
- Added a new
graphicsnamespace providing a high-level API for process-based window rendering - Enhanced mouse support with PS/2 initialization, cursor rendering, and event dispatching
- Introduced per-window dirty region tracking to minimize redraw operations
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/user/editor.cpp | Integrates graphics API for windowed editor rendering with efficient per-cell updates |
| src/kernel/vga.cpp | Adds framebuffer present calls at key cursor/render points |
| src/kernel/terminal_windows.cpp | Implements window management with z-ordering, resizing, dragging, and per-window dirty tracking |
| src/kernel/shell.cpp | Adds windowed input rendering with cursor management and graphics API integration |
| src/kernel/mouse.cpp | Implements PS/2 mouse driver with scroll wheel detection and event dispatching |
| src/kernel/kernel.cpp | Adds debug logging for framebuffer initialization status |
| src/kernel/gui.cpp | Implements mouse cursor rendering with background capture/restore |
| src/kernel/graphics.cpp | Provides high-level graphics API wrapping terminal window operations |
| src/kernel/framebuffer.cpp | Implements double-buffering with buffer swapping and optimized fill operations |
| src/kernel/debug.cpp | Adds present call after panic screen rendering |
| libc/include/sys/events.h | Extends event system to support mouse events |
| include/kernel/terminal_windows.h | Exposes window manipulation and mouse handling APIs |
| include/kernel/mouse.h | Defines mouse event structures and state |
| include/kernel/hooks.h | Adds missing pragma once guard |
| include/kernel/gui.h | Declares mouse cursor and window redraw functions |
| include/kernel/graphics.h | Declares new graphics API for process-based rendering |
| include/kernel/framebuffer.h | Extends framebuffer API with BufferTarget and present function |
| include/kernel/editor.h | Adds window-aware rendering methods to Editor class |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| constexpr uint8_t RESIZE_EDGE_NONE = 0; | ||
| constexpr uint8_t RESIZE_EDGE_LEFT = 1u << 0; | ||
| constexpr uint8_t RESIZE_EDGE_RIGHT = 1u << 1; | ||
| constexpr uint8_t RESIZE_EDGE_TOP = 1u << 2; | ||
| constexpr uint8_t RESIZE_EDGE_BOTTOM = 1u << 3; |
There was a problem hiding this comment.
These bitfield constants should use a consistent suffix. Lines 102-105 use the u suffix (1u << ...) while line 101 uses a plain 0. For consistency, line 101 should be constexpr uint8_t RESIZE_EDGE_NONE = 0u;.
| if (g_bytes_per_pixel == 4) | ||
| { | ||
| uint8_t *row_ptr = base + (y + row) * row_stride + x * g_bytes_per_pixel; | ||
| for (uint32_t col = 0; col < width; ++col) | ||
| for (uint32_t row = 0; row < height; ++row) | ||
| { | ||
| uint8_t *pixel = row_ptr + col * g_bytes_per_pixel; | ||
| switch (g_bytes_per_pixel) | ||
| uint8_t *row_base = base + (y + row) * row_stride + x * pixel_stride; | ||
| uint32_t *dst = reinterpret_cast<uint32_t *>(row_base); | ||
| for (uint32_t col = 0; col < width; ++col) | ||
| { | ||
| case 4: | ||
| { | ||
| uint32_t value = color; | ||
| memcpy(pixel, &value, sizeof(uint32_t)); | ||
| break; | ||
| } | ||
| case 3: | ||
| { | ||
| pixel[0] = static_cast<uint8_t>(color & 0xFF); | ||
| pixel[1] = static_cast<uint8_t>((color >> 8) & 0xFF); | ||
| pixel[2] = static_cast<uint8_t>((color >> 16) & 0xFF); | ||
| break; | ||
| } | ||
| case 2: | ||
| { | ||
| uint16_t value = static_cast<uint16_t>(color & 0xFFFF); | ||
| memcpy(pixel, &value, sizeof(uint16_t)); | ||
| break; | ||
| } | ||
| default: | ||
| break; | ||
| dst[col] = color; | ||
| } | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
The fast path assumes 4-byte alignment for the uint32_t pointer at line 678, but row_base may not be 4-byte aligned depending on the values of x and pixel_stride. This can cause unaligned memory access. Consider using memcpy or checking alignment before using the fast path, or use the generic store_color function for all cases.
No description provided.