Skip to content

Fix, ids in view manager are not stable#196

Merged
Segfaultd merged 2 commits into
MafiaHub:developfrom
CrosRoad95:fix/webview-not-stable-ids
May 26, 2026
Merged

Fix, ids in view manager are not stable#196
Segfaultd merged 2 commits into
MafiaHub:developfrom
CrosRoad95:fix/webview-not-stable-ids

Conversation

@CrosRoad95
Copy link
Copy Markdown
Contributor

@CrosRoad95 CrosRoad95 commented May 26, 2026

Fixes issue where you create multiple views and destroy them in different order

Summary by CodeRabbit

  • Refactor
    • Views now receive explicit, unique numeric identifiers to improve tracking.
    • Creation flow updated to assign and return stable IDs when new views are made.
    • Destruction now removes views by their ID rather than by list index.
    • Added a public accessor to retrieve a view's ID.
    • Internal view construction updated to require an ID at initialization.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dbf58995-cc2c-4022-9681-4039f82755c4

📥 Commits

Reviewing files that changed from the base of the PR and between e6a2d9a and 501d51f.

📒 Files selected for processing (2)
  • code/framework/src/gui/manager.cpp
  • code/framework/src/gui/manager.h

Walkthrough

View gained an explicit integer id stored in _id and exposed via GetId(). ViewD3D11 forwards the id to the base. Manager now assigns incremental ids on creation, returns that id, and looks up/destroys views by id instead of by vector index.

Changes

View ID Lifecycle Management

Layer / File(s) Summary
View base class contract and accessor
code/framework/src/gui/view.h, code/framework/src/gui/view.cpp
View constructor now requires an int id parameter and stores it as _id. A new GetId() const accessor returns the stored id.
ViewD3D11 subclass alignment
code/framework/src/gui/backend/view_d3d11.h, code/framework/src/gui/backend/view_d3d11.cpp
ViewD3D11 constructor updated to accept id parameter and forward it to the base View constructor.
Manager id state and lifecycle
code/framework/src/gui/manager.h, code/framework/src/gui/manager.cpp
Manager tracks an internal _id counter, increments it when creating views, returns the assigned id, implements GetView(int) out-of-line, and refactors DestroyView to find and erase views by GetId() matching rather than treating id as a vector index.
sequenceDiagram
  participant Manager
  participant ViewD3D11
  participant View
  Manager->>Manager: ++_id
  Manager->>ViewD3D11: CreateView(++_id, renderer, manager)
  ViewD3D11->>View: View(id, renderer, manager)
  View->>View: store _id
  Manager->>Manager: push view to _views
  Manager->>Manager: return _id
  Manager->>Manager: DestroyView(id)
  Manager->>Manager: find view by GetId() == id
  Manager->>Manager: erase matched view
Loading

🎯 3 (Moderate) | ⏱️ ~25 minutes

🐰 I hopped in code with nimble paws and cheer,
Gave every View an id to hold dear,
Manager counts up, creates, then finds,
No more index guessing—order aligns,
A rabbit's stamp on views, tidy and clear.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main issue: introducing stable IDs in the view manager by adding an _id counter and explicit id parameters to View/ViewD3D11 constructors, replacing index-based ID assignment.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@code/framework/src/gui/manager.cpp`:
- Around line 148-149: The change that returns a monotonic view id (++_id) in
the view creation path (where ViewD3D11 is constructed) no longer matches how
GetView(int id) looks up views by directly indexing _views[id], causing
out-of-bounds access; fix by making the lookup consistent: either return the
vector index (use _id before increment or don't increment) or update GetView(int
id) to locate views by their stable id (e.g., maintain a mapping from viewId ->
vector index or search _views for view->Id() and return that pointer). Adjust
the creation code that assigns ids (the ++_id / ViewD3D11 construction) and/or
add/maintain an _viewIndexMap (or change GetView to linear search) so
GetView(int id) uses the same id semantics as the creator.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 19908997-7ab1-4575-8d1b-f72d7050bb59

📥 Commits

Reviewing files that changed from the base of the PR and between 20e7edb and e6a2d9a.

📒 Files selected for processing (6)
  • code/framework/src/gui/backend/view_d3d11.cpp
  • code/framework/src/gui/backend/view_d3d11.h
  • code/framework/src/gui/manager.cpp
  • code/framework/src/gui/manager.h
  • code/framework/src/gui/view.cpp
  • code/framework/src/gui/view.h

Comment thread code/framework/src/gui/manager.cpp
@Segfaultd Segfaultd merged commit a9aa0d6 into MafiaHub:develop May 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants