fix(UI): DPI scale new UI additions#2457
Conversation
📝 WalkthroughWalkthroughThis PR centralizes search and baseline UI sizing in ThemeManager, adds search-specific scaling helpers, replaces hardcoded search/icon drawing, makes profiler graphs and legend scale-aware, and localizes profiling/menu strings while extracting profiling UI helpers. ChangesTheme-Driven UI Scaling and Profiling Localization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.15][ERROR]: Error: exception Unix_error: No such file or directory stat src/Menu/ProfilingRenderer.cpp 🔧 Infer (1.2.0)src/Menu/ProfilingRenderer.cppUsage Error: Failed to execute compilation command: Error message: *** Infer needs a working compilation command to run. src/Menu/SettingsTabRenderer.cppUsage Error: Failed to execute compilation command: Error message: *** Infer needs a working compilation command to run. 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. Comment |
|
No actionable suggestions for changed features. |
Dlizzio
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Utils/UI.cpp (1)
1352-1390:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the icon centered against the final input rect.
Line 1361 captures
ImGui::GetFrameHeight()before the scaledFramePaddingis pushed, so the icon is centered against the old height, not the actual rendered field. At higher DPI scales this leaves the magnifier visibly off-center.Proposed fix
- float frameHeight = ImGui::GetFrameHeight(); - // Custom style - always transparent background to avoid click blocking ImVec4 bgColor = ImVec4(0.0f, 0.0f, 0.0f, 0.0f); @@ - ImVec2 iconPos = ImVec2(cursorPos.x + ThemeManager::Constants::SEARCH_ICON_OFFSET_X * scale, cursorPos.y + (frameHeight - iconSize) * 0.5f); + const ImVec2 itemMin = ImGui::GetItemRectMin(); + const ImVec2 itemSize = ImGui::GetItemRectSize(); + ImVec2 iconPos = ImVec2( + itemMin.x + ThemeManager::Constants::SEARCH_ICON_OFFSET_X * scale, + itemMin.y + (itemSize.y - iconSize) * 0.5f); DrawSearchIcon(iconPos, iconSize, ThemeManager::Constants::SEARCH_ICON_ALPHA);🤖 Prompt for 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. In `@src/Utils/UI.cpp` around lines 1352 - 1390, The icon vertical position is computed using frameHeight obtained via ImGui::GetFrameHeight() before the scaled FramePadding style is pushed, causing misalignment at DPI scales; move or recompute the frame height after the ImGui::PushStyleVar(ImGuiStyleVar_FramePadding, ...) (i.e., after the style changes are applied) and then use that updated frameHeight when computing iconPos for DrawSearchIcon so the magnifier is centered against the actual rendered InputTextWithHint field (references: ImGui::GetFrameHeight, ImGui::PushStyleVar(ImGuiStyleVar_FramePadding,...), ImGui::InputTextWithHint, DrawSearchIcon, cursorPos, iconPos).
🧹 Nitpick comments (2)
src/Menu/FeatureListRenderer.cpp (2)
816-816: ⚡ Quick winUse a dedicated i18n key for the in-panel profiling section.
Reusing
menu.features.profilingfor both the left-nav entry and this separator couples two different UI contexts, which makes contextual translations harder for non-English locales. A section-specific key would avoid that bleed-over.🤖 Prompt for 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. In `@src/Menu/FeatureListRenderer.cpp` at line 816, Replace the reused i18n key at the separator call in FeatureListRenderer.cpp (the ImGui::SeparatorText(T(...)) invocation) with a new, section-specific key (e.g. "menu.features.profiling.section") so the in-panel heading has its own translation context; update your localization/resource files to add the new key and provide translations, and leave the original "menu.features.profiling" key intact for the left-nav entry to avoid cross-context coupling.
816-816: ⚡ Quick winRetitle the PR to match the shipped change, and add an issue link if there is one.
chore(UI): DPI scale new UI additionsdoesn't really describe this localization diff and also misses the lowercase Conventional Commit style. Something likefix(ui): localize profiling and hdr warning labelswould fit this slice better. If this work maps to a tracked issue, addAddresses #<id>orCloses #<id>in the PR description.As per coding guidelines,
When reviewing PRs, please provide suggestions for: Conventional Commit Titles ... Issue References.🤖 Prompt for 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. In `@src/Menu/FeatureListRenderer.cpp` at line 816, Update the PR title and description to accurately reflect the localization change and follow conventional commits: change the title from "chore(UI): DPI scale new UI additions" to a descriptive, lowercase-conventional title such as "fix(ui): localize profiling and hdr warning labels", and if this change corresponds to a tracked issue add "Addresses #<id>" or "Closes #<id>" to the PR body; this code change is the localization of the Profiling label (see ImGui::SeparatorText and the T("menu.features.profiling", "Profiling") call in FeatureListRenderer.cpp) so reference those symbols when updating the commit/PR message.
🤖 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 `@src/Menu/ProfilingRenderer.cpp`:
- Around line 130-145: When the global timingMode changes, always invalidate
cached stats instead of relying on the caller's refreshCachedStats flag: in
ProfilingRenderer::RenderTimingModeToggle detect newMode != timingMode and
unconditionally set timeSinceLastUpdate = kStatsRefreshSeconds (remove the
refreshCachedStats check) so RenderFeatureTimers()/RenderStatistics() won't show
stale GPU/CPU aggregates; apply the same change where RenderTimingModeToggle is
called from the other view (the other usage noted around
RenderFeatureTimers/RenderStatistics) to ensure consistency across both views.
- Around line 64-79: GetGraphLayout currently returns graphWidth =
std::max(minGraphWidth, availableWidth - legendWidth) which can exceed
availableWidth when availableWidth < minGraphWidth (causing overflow); change
the logic in GetGraphLayout so the computed graph width is clamped to
availableWidth (e.g. compute graphWidth = std::max(minGraphWidth, availableWidth
- legendWidth) and then graphWidth = std::min(graphWidth, availableWidth)), and
ensure legendWidth is reduced to 0 when there isn't room so the returned
GraphLayout (from GetGraphLayout) never has graphWidth greater than
availableWidth.
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 1211-1225: The search icon vertical position is computed using
ImGui::GetFrameHeight() before the scaled frame padding is applied, causing
vertical drift at >1x scale; after calling colorFilter.Draw(...) (which renders
the framed input with the pushed style), query the rendered item's rectangle
(e.g., ImGui::GetItemRectMin/Max or ImGui::GetItemRectSize) and compute
iconPos.y from that rect (center using rect_min.y + rect_height*0.5f) instead of
using the earlier frameHeight; keep the existing scale, iconSize, and
SEARCH_ICON_OFFSET_X usage and still compute iconPos.x from the original cursor
x plus offset so Util::DrawSearchIcon(...) gets the corrected position.
---
Outside diff comments:
In `@src/Utils/UI.cpp`:
- Around line 1352-1390: The icon vertical position is computed using
frameHeight obtained via ImGui::GetFrameHeight() before the scaled FramePadding
style is pushed, causing misalignment at DPI scales; move or recompute the frame
height after the ImGui::PushStyleVar(ImGuiStyleVar_FramePadding, ...) (i.e.,
after the style changes are applied) and then use that updated frameHeight when
computing iconPos for DrawSearchIcon so the magnifier is centered against the
actual rendered InputTextWithHint field (references: ImGui::GetFrameHeight,
ImGui::PushStyleVar(ImGuiStyleVar_FramePadding,...), ImGui::InputTextWithHint,
DrawSearchIcon, cursorPos, iconPos).
---
Nitpick comments:
In `@src/Menu/FeatureListRenderer.cpp`:
- Line 816: Replace the reused i18n key at the separator call in
FeatureListRenderer.cpp (the ImGui::SeparatorText(T(...)) invocation) with a
new, section-specific key (e.g. "menu.features.profiling.section") so the
in-panel heading has its own translation context; update your
localization/resource files to add the new key and provide translations, and
leave the original "menu.features.profiling" key intact for the left-nav entry
to avoid cross-context coupling.
- Line 816: Update the PR title and description to accurately reflect the
localization change and follow conventional commits: change the title from
"chore(UI): DPI scale new UI additions" to a descriptive, lowercase-conventional
title such as "fix(ui): localize profiling and hdr warning labels", and if this
change corresponds to a tracked issue add "Addresses #<id>" or "Closes #<id>" to
the PR body; this code change is the localization of the Profiling label (see
ImGui::SeparatorText and the T("menu.features.profiling", "Profiling") call in
FeatureListRenderer.cpp) so reference those symbols when updating the commit/PR
message.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab291b37-8050-4dbb-9679-b83ce1397fcb
📒 Files selected for processing (10)
package/SKSE/Plugins/CommunityShaders/Translations/en.jsonsrc/Features/HDRDisplay.cppsrc/Menu/FeatureListRenderer.cppsrc/Menu/ProfilingRenderer.cppsrc/Menu/ProfilingRenderer.hsrc/Menu/SettingsTabRenderer.cppsrc/Menu/ThemeManager.hsrc/Utils/LegitProfiler.hsrc/Utils/UI.cppsrc/Utils/UI.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Menu/ProfilingRenderer.cpp (1)
131-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake ImGui timing toggle IDs instance-scoped
ProfilingRenderer::RenderTimingModeToggle()always doesImGui::PushID("ProfilingTimingMode"), andProfilingRenderer::RenderFeatureTimers()calls it for every rendered feature. SinceFeatureListRenderercan render multiple feature entries (and thus multipleRenderFeatureTimers(...)calls) in the same ImGui window/frame, the GPU/CPU radio buttons can share the same ImGui IDs and alias.Pass a unique ID context into
RenderTimingModeToggle()(e.g., based onfeaturePrefix/feat->GetShortName()), or push a caller-specificPushID(...)around the call site so each instance’s radio buttons get distinct IDs.🤖 Prompt for 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. In `@src/Menu/ProfilingRenderer.cpp` around lines 131 - 139, The ImGui ID for the timing mode radio buttons is global because ProfilingRenderer::RenderTimingModeToggle() always calls ImGui::PushID("ProfilingTimingMode"), causing ID collisions when FeatureListRenderer renders multiple features; fix by making the ID instance-scoped — either change RenderTimingModeToggle(...) to accept a caller-provided unique id (e.g., featurePrefix or feat->GetShortName()) and use that in ImGui::PushID, or wrap the call site in ProfilingRenderer::RenderFeatureTimers(...) with a caller-specific ImGui::PushID/PopID (derived from featurePrefix/feat->GetShortName()) so each feature’s GPU/CPU radio buttons have distinct IDs.
🧹 Nitpick comments (1)
src/Menu/ProfilingRenderer.cpp (1)
1-1: Suggested PR metadata cleanup.The current title is close, but to match the repo guideline I’d rename it to
fix(ui): scale profiling UI with DPI. If this change closes a tracked bug, add an issue reference likeFixes #<id>.As per coding guidelines, "Conventional Commit Titles ... Format: type(scope): description" and "Issue References ... Suggest adding appropriate GitHub keywords".
🤖 Prompt for 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. In `@src/Menu/ProfilingRenderer.cpp` at line 1, Rename the pull request title to follow Conventional Commits—use "fix(ui): scale profiling UI with DPI"—and if this change closes a tracked bug add an issue reference like "Fixes #<id>" in the PR description; also update the PR body to state the conventional-commit style and mention the affected symbol (ProfilingRenderer.h / ProfilingRenderer implementation) so reviewers know which component is changed.
🤖 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.
Outside diff comments:
In `@src/Menu/ProfilingRenderer.cpp`:
- Around line 131-139: The ImGui ID for the timing mode radio buttons is global
because ProfilingRenderer::RenderTimingModeToggle() always calls
ImGui::PushID("ProfilingTimingMode"), causing ID collisions when
FeatureListRenderer renders multiple features; fix by making the ID
instance-scoped — either change RenderTimingModeToggle(...) to accept a
caller-provided unique id (e.g., featurePrefix or feat->GetShortName()) and use
that in ImGui::PushID, or wrap the call site in
ProfilingRenderer::RenderFeatureTimers(...) with a caller-specific
ImGui::PushID/PopID (derived from featurePrefix/feat->GetShortName()) so each
feature’s GPU/CPU radio buttons have distinct IDs.
---
Nitpick comments:
In `@src/Menu/ProfilingRenderer.cpp`:
- Line 1: Rename the pull request title to follow Conventional Commits—use
"fix(ui): scale profiling UI with DPI"—and if this change closes a tracked bug
add an issue reference like "Fixes #<id>" in the PR description; also update the
PR body to state the conventional-commit style and mention the affected symbol
(ProfilingRenderer.h / ProfilingRenderer implementation) so reviewers know which
component is changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d65dd1b-9981-4c80-95cc-e782c860a78e
📒 Files selected for processing (4)
src/Menu/ProfilingRenderer.cppsrc/Menu/ProfilingRenderer.hsrc/Menu/SettingsTabRenderer.cppsrc/Utils/LegitProfiler.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Utils/LegitProfiler.h
- src/Menu/ProfilingRenderer.h
- src/Menu/SettingsTabRenderer.cpp
|
✅ A pre-release build is available for this PR: |
3 participants
Adds DPI scaling to new UI additions that forgot.
Summary by CodeRabbit
New Features
Bug Fixes
Localization