Populate the skin gallery on demand to reduce startup time#2902
Conversation
📝 WalkthroughWalkthroughThe skin gallery mapper now populates gallery items lazily on first use—triggered by dropdown initialization or ribbon page selection—instead of eagerly during MapFrom. A pageContains helper checks selected pages, and the redundant event subscription in createSkinGallery was removed. ChangesLazy skin gallery population
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Ribbon
participant SkinManagerToSkinGalleryMapper
participant SkinManager
User->>Ribbon: Open skins dropdown / select ribbon page
Ribbon->>SkinManagerToSkinGalleryMapper: InitDropDownGallery / SelectedPageChanged
SkinManagerToSkinGalleryMapper->>SkinManagerToSkinGalleryMapper: pageContains(skins gallery item)
SkinManagerToSkinGalleryMapper->>SkinManagerToSkinGalleryMapper: populateGallery()
SkinManagerToSkinGalleryMapper->>SkinManager: All()
SkinManagerToSkinGalleryMapper->>SkinManagerToSkinGalleryMapper: resolve ActivateSkinCommand per skin, add gallery items
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/OSPSuite.UI/Mappers/SkinManagerToSkinGalleryMapper.cs (1)
34-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard condition works but relies on an implicit invariant.
Using
rgbiSkins.Gallery.Groups[0].Items.Count > 0as the "already populated" check is a bit indirect — a future change toaddSkinToGallery/createSkinGallerythat alters group indexing or clears items could silently break the guard and cause a re-population/duplication bug. Consider an explicit boolean flag captured in the closure for clarity and robustness.♻️ Suggested clarity improvement
+ var isPopulated = false; + void populateGallery() { - if (rgbiSkins.Gallery.Groups[0].Items.Count > 0) - return; + if (isPopulated) + return; + + isPopulated = true; foreach (var skin in skinManager.All())The rest of the wiring (dual triggers via
InitDropDownGalleryandSelectedPageChanged,pageContainsscoping) looks correct and matches the PR's lazy-population goal.🤖 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/OSPSuite.UI/Mappers/SkinManagerToSkinGalleryMapper.cs` around lines 34 - 67, The lazy-population guard in the SkinManagerToSkinGalleryMapper logic relies on rgbiSkins.Gallery.Groups[0].Items.Count, which is an implicit invariant and can break if addSkinToGallery or createSkinGallery changes group/item structure. Replace that check inside populateGallery() with an explicit closure-captured boolean flag that tracks whether the gallery has already been populated, and set it after the first successful population so both InitDropDownGallery and SelectedPageChanged keep working without duplicate entries.
🤖 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.
Nitpick comments:
In `@src/OSPSuite.UI/Mappers/SkinManagerToSkinGalleryMapper.cs`:
- Around line 34-67: The lazy-population guard in the
SkinManagerToSkinGalleryMapper logic relies on
rgbiSkins.Gallery.Groups[0].Items.Count, which is an implicit invariant and can
break if addSkinToGallery or createSkinGallery changes group/item structure.
Replace that check inside populateGallery() with an explicit closure-captured
boolean flag that tracks whether the gallery has already been populated, and set
it after the first successful population so both InitDropDownGallery and
SelectedPageChanged keep working without duplicate entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 116c6b69-2a19-459b-be89-f6842bcb251b
📒 Files selected for processing (1)
src/OSPSuite.UI/Mappers/SkinManagerToSkinGalleryMapper.cs
|
nice! |
|
cool
what about remving journla editor :) |
Fixes #2901
Part of a broader effort to cut PK-Sim / MoBi startup time (Open-Systems-Pharmacology/PK-Sim#1472) — ~5 s combined across the fixes below. The ribbon skin gallery was built entirely at startup (one command + two skin-styled bitmaps per skin, ~50 skins). It is now populated on first use — when the drop-down is opened or its ribbon page is selected. Startup skin restore is unaffected. MoBi gets the same win via the shared OSPSuite.UI code.
Approximate startup savings across the investigation:
ValueOriginRepositoryre-mapping bug (guardStart())So, when you click the Utilities tab, there is a small delay because that's when the gallery is built. We could move this gallery if it feels weird, the other tabs are instant.
🤖 Generated with Claude Code