Perf fixes + Scrollbar/KeyValueView classes by skies912 · Pull Request #43 · jdolan/ObjectivelyMVC

skies912 · 2026-07-09T20:39:09Z

Summary

O(depth) hover/clipping fixes for WindowController/View (was O(views * depth^2) on mouse motion, and O(depth^2) per clippingFrame computation) and self-only Control state invalidation, instead of re-styling whole subtrees on hover changes.
CSS parser now strips /* */ comments (a comment before a rule was previously getting absorbed into its selector, silently killing the rule).
Select viewport clamp fix.
TabViewItem gains a TabStateDisabled state (greys out and blocks clicks on a tab without removing it from the TabView).
New Scrollbar/ScrollThumb classes: a visible, draggable vertical scrollbar bound directly to a StackView's own opt-in scrolling (scroll: true, contentOffset/scrollToOffset) — no ScrollView wrapper needed.
New KeyValueView/KeyValueTableView classes: a two-column key→value row (any View in either column) and a vertical table of such rows with shared, CSS-driven column widths (-key-width/-value-width).

All of this was built and used in-tree for Quetoo's in-game material/entity editor, then generalized and promoted here since none of it is Quetoo-specific.

Test plan

Builds clean (Debug x64) against current main, including the SDL_gpu/ObjectivelyGPU renderer.
Exercised in Quetoo's cgame editor: scrolling stage list via Scrollbar bound to a StackView, KeyValueTableView rows in the entity/material/mesh tabs, disabled mesh tab via TabStateDisabled.

🤖 Generated with Claude Code

- Builds Scrolling (via scrollbar) into StackView as an opt-in path via css - Support for Image-buttons

…clamp WindowController: replace the per-motion whole-tree hover scan with a hit-test chain-diff (O(views*depth^2) -> O(depth)); mark only the entered/left views for re-theme, not their subtrees (no ancestor:hover cascade exists), killing the spike on crossing into a large container. View: rewrite clippingFrame from a per-ancestor recursion (O(depth^2), called per-view per-frame) to a single O(depth) top-down pass; result is value-identical by construction. Style: strip /* */ comments before tokenizing. The parser splits on { : ; } with no notion of comments, so a comment absorbed into a selector silently broke the rule it preceded. Select: keep an expanded menu on-screen by clamping it up from the window bottom edge, in layoutSubviews (the earliest point its true height is known). Control: revert stateDidChange to subtree invalidateStyle -- state pseudos (Checkbox:selected ImageView, etc.) do style descendants; the self-only variant broke checkmark toggling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

- ability to disable tabs

Promoted from quetoo's cgame editor, where they were originally built as project-local classes. Scrollbar now binds directly to a StackView's own opt-in scrolling (contentOffset/scrollToOffset) instead of requiring a ScrollView wrapper. All four are generic, renderer-agnostic View subclasses with no cgame dependency.

Copilot

Pull request overview

This PR improves ObjectivelyMVC UI performance (hover tracking, clipping computation, and page layout), fixes CSS parsing around block comments, and introduces new UI components for scrolling and key/value layouts.

Changes:

Reworked hover enter/leave dispatch in WindowController and optimized View::clippingFrame to avoid repeated ancestor recursion.
Added opt-in StackView scrolling (content offset + wheel handling) plus new Scrollbar/ScrollThumb widgets to drive it.
Added KeyValueView / KeyValueTableView and a disabled-tab state for TabViewItem; improved CSS parsing by stripping /* ... */ comments.

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
Sources/ObjectivelyMVC/WindowController.c	Replaces full-tree hover scanning with hit-test path diffing and marks views for re-theming.
Sources/ObjectivelyMVC/View.c	Optimizes `clippingFrame` from recursive to single-pass chain computation.
Sources/ObjectivelyMVC/TabViewItem.h	Adds `TabStateDisabled` enum flag.
Sources/ObjectivelyMVC/TabViewItem.c	Applies/removes `"disabled"` class based on tab state.
Sources/ObjectivelyMVC/TabView.c	Ignores clicks on disabled tabs.
Sources/ObjectivelyMVC/Style.c	Strips `/* ... */` comments before tokenizing CSS.
Sources/ObjectivelyMVC/StackView.h	Adds opt-in scrolling fields and `scrollToOffset` API.
Sources/ObjectivelyMVC/StackView.c	Implements scroll styling, wheel scrolling, and offset clamping.
Sources/ObjectivelyMVC/SelectorSequence.c	Fixes `isspace` UB by casting to `unsigned char`.
Sources/ObjectivelyMVC/Select.c	Clamps expanded menu viewport to window bounds.
Sources/ObjectivelyMVC/ScrollThumb.h	Introduces draggable thumb control + delegate protocol.
Sources/ObjectivelyMVC/ScrollThumb.c	Implements thumb drag event capture and delegate callbacks.
Sources/ObjectivelyMVC/Scrollbar.h	Introduces vertical scrollbar API bound to `StackView` scrolling.
Sources/ObjectivelyMVC/Scrollbar.c	Implements scrollbar layout, styling, stepping, and thumb syncing.
Sources/ObjectivelyMVC/PageView.c	Avoids laying out hidden pages to reduce tab-panel layout stalls.
Sources/ObjectivelyMVC/Makefile.am	Adds new source/header files to the build.
Sources/ObjectivelyMVC/KeyValueView.h	Adds key/value row view API (two-column row).
Sources/ObjectivelyMVC/KeyValueView.c	Implements key/value row creation, inlets, and column clamping.
Sources/ObjectivelyMVC/KeyValueTableView.h	Adds vertical table API for shared column widths across rows.
Sources/ObjectivelyMVC/KeyValueTableView.c	Implements table-wide column propagation and row management.
Sources/ObjectivelyMVC/Control.c	Documents why control state changes must invalidate subtree styles.
Sources/ObjectivelyMVC/Button.c	Adds JSON inlet binding for button image.
ObjectivelyMVC.vs15/ObjectivelyMVC.vcxproj.filters	Registers new files in the VS filters list.
ObjectivelyMVC.vs15/ObjectivelyMVC.vcxproj	Registers new files in the VS project.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+  for (size_t r = 0; r < len; ) {
+    if (css[r] == '/' && css[r + 1] == '*') {
+      r += 2;
+      while (r < len && !(css[r] == '*' && css[r + 1] == '/')) {
+        r++;
+      }
+      r = (r < len) ? r + 2 : len;
+      out[w++] = ' ';
+    } else {
+      out[w++] = css[r++];
+    }
+  }


+    // Opt-in scrolling: when enabled, shift the stack by the (negative) scroll
+    // offset and clip the overflow. Disabled StackViews lay out exactly as before.
+    if (this->scrolls) {
+      pos = (this->axis == StackViewAxisVertical) ? this->contentOffset.y : this->contentOffset.x;
+      self->clipsSubviews = true;
+    }


+ */
+static void applyStyle(View *self, const Style *style) {
+
+  super(View, self, applyStyle, style);


+ */
+static void applyStyle(View *self, const Style *style) {
+
+  super(View, self, applyStyle, style);


+    }
+  }
+
+  super(View, self, layoutSubviews);


+    if (!isInChain(v, to)) {
+      $(v, emitViewEvent, ViewEventMouseLeave, NULL);
+      v->needsApplyTheme = true;
+    }
+  }


+    MakeInlet("-adorner-size", InletTypeInteger, &this->adornerSize, NULL),
+    MakeInlet("-adorner-shade", InletTypeInteger, &this->adornerShade, NULL),
+    MakeInlet("-orientation", InletTypeEnum, &this->orientation, (ident) ScrollbarOrientationNames),
+    MakeInlet("-bgcolor", InletTypeColor, &this->bgColor, NULL),
+    MakeInlet("-fgcolor", InletTypeColor, &this->fgColor, NULL),


jdolan

ObjectivelyMVC already has ScrollView, that is specifically meant for wrapping anything you want to scroll, within a sized "viewport" (frame). It's what TableView uses internally, for example.

We should definitely not add ScrollView / ScrollBar functionality directly to StackView, whose primary responsibility is layout of its subviews. Maintain single-responsibility principle / separation of concerns.

Really, the ScrollBar stuff should all be added as an option (opt-in) on ScrollView.

Then, if you want to scroll over the contents of a StackView, you put that StackView inside a ScrollView. This is exactly what TableView does.

I would suggest we break this into several PRs:

TabView disablement
Select layout fixes
View / WindowController perf fixes
ScrollBar as an nullable member on ScrollView

The latter requires significant rework from the rest of this PR. Is this something that you want to run with, or do you want to hand it off? It's gonna be a little while if I take it..

jdolan · 2026-07-11T13:58:17Z

+ * is provided by an ancestor StackView with `scroll: true`.
+ */
+
+typedef struct KeyValueTableView KeyValueTableView;


Hm. This whole class feels like a workaround for either a CSS bug or a missing feature in our CSS engine? It should be possible to write a CSS rule that would apply to a given column for all rows -- which is basically all you're addressing here, right? The ability to say "key" is always 120px and "value" is always 240px or whatever, right?

One thing that would help is if TableView added CSS classes on each TableCellView based on its column headers. So in every row, the "key" cell would have classNames containing "key" for example.

Would that address this and make this class superfluous?

@copilot Weigh in on this.

jdolan · 2026-07-11T13:59:20Z

+ * tabbed-panel layout stall. A page is laid out lazily when it becomes current:
+ * setCurrentPage unhides it and flags this view for layout.
+ */
+static void layoutIfNeeded(View *self) {


jdolan · 2026-07-11T13:59:49Z

+    View *subview = subviews->elements[i];
+
+    if (!subview->hidden) {
+      $(subview, layoutIfNeeded);


I mean, should this actually just be the behavior in View::layoutSubviews? Seems like it...

@copilot Weigh in on this as well.

jdolan · 2026-07-11T14:00:58Z

+
+const EnumName ScrollbarOrientationNames[] = MakeEnumNames(
+  MakeEnumAlias(ScrollbarOrientationRight, right),
+  MakeEnumAlias(ScrollbarOrientationLeft, left)


Shouldn't we also support top and bottom, for horizontal scrolling?

jdolan · 2026-07-11T14:02:00Z

+/**
+ * @brief ScrollThumbDelegate; maps thumb travel onto the StackView's content offset.
+ */
+static void thumbDidDrag(ScrollThumb *thumb, int delta) {


You're gonna want a float for delta here. SDL3 uses floating point for mouse movement and scroll increments, because on high-DPI displays, sub-pixel (logical pixel) movement is definitely a thing.

jdolan · 2026-07-11T14:27:29Z

+
+#include "ScrollThumb.h"
+
+#define _Class _ScrollThumb


Also, Panel / Panel.contentView should probably get one of these huh?

jdolan · 2026-07-11T14:28:05Z

+  // Cast to unsigned char: isspace() is UB for negative values (non-ASCII bytes
+  // sign-extend to negative char), which the MSVC debug CRT asserts on. This
+  // also lets the parser tolerate arbitrary input bytes without crashing.
+  while (isspace((unsigned char) *c)) {


jdolan · 2026-07-11T14:34:56Z

    MakeInlet("distribution", InletTypeEnum, &this->distribution, (ident) StackViewDistributionNames),
-    MakeInlet("spacing", InletTypeInteger, &this->spacing, NULL)
+    MakeInlet("spacing", InletTypeInteger, &this->spacing, NULL),
+    MakeInlet("scroll", InletTypeBool, &this->scrolls, NULL)


Oof.

ObjectivelyMVC already has ScrollView, that is specifically meant for wrapping anything you want to scroll, within a sized "viewport" (frame). It's what TableView uses internally, for example.

We should definitely not add this functionality directly to StackView, whose primary responsibility is layout of its subviews. Maintain single-responsibility principle / separation of concerns.

Really, the ScrollBar stuff should all be added as an option (opt-in) on ScrollView.

Then, if you want to scroll over the contents of a StackView, you put that StackView inside a ScrollView. This is exactly what TableView does.

jdolan · 2026-07-11T14:35:54Z

+ * Each comment is replaced by a single space so it still separates adjacent tokens.
+ * The caller must free the returned buffer.
+ */
+static char *stripComments(const char *css) {


jdolan · 2026-07-11T14:40:59Z

  TabStateDefault = 0x0,
-  TabStateSelected = 0x1
+  TabStateSelected = 0x1,
+  TabStateDisabled = 0x2


Copilot · 2026-07-11T14:45:42Z

@jdolan I've opened a new pull request, #44, to work on those changes. Once the pull request is ready, I'll request review from you.

skies912 and others added 4 commits July 9, 2026 16:19

initial push

cddce12

- Builds Scrolling (via scrollbar) into StackView as an opt-in path via css - Support for Image-buttons

TabView disabler

7f3e7cd

- ability to disable tabs

skies912 mentioned this pull request Jul 10, 2026

In-game material stage editor + editor UI overhaul jdolan/quetoo#895

Open

jdolan requested a review from Copilot July 11, 2026 13:52

Copilot started reviewing on behalf of jdolan July 11, 2026 13:52 View session

Copilot AI reviewed Jul 11, 2026

View reviewed changes

jdolan requested changes Jul 11, 2026

View reviewed changes

Copilot AI mentioned this pull request Jul 11, 2026

[WIP] [WIP] Addressing review feedback on perf fixes and scrollbar classes #44

Draft

Uh oh!

Conversation

skies912 commented Jul 9, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

jdolan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants