fix: displaying recommended kernel #1
Conversation
| QList<QLabel*> m_sectionHeaders; | ||
| QList<QFrame*> m_sectionSeparators; | ||
|
|
||
| QHBoxLayout* m_cardsLayout; |
There was a problem hiding this comment.
Initialize with nullptr
There was a problem hiding this comment.
Pull request overview
This PR fixes the display logic for the recommended kernel to properly show it as a badge on the currently in-use kernel card when they are the same, rather than displaying it as a separate card.
Key changes:
- Modified the visibility condition in both QML and C++ classic UI to hide the recommended kernel card when it matches the in-use kernel
- Added a "Recommended" badge to kernel cards that displays inline when a kernel is marked as recommended
- Updated the kernel selection logic in KernelProvider to allow marking the in-use kernel as recommended (removed the "not in use" filter)
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mcp-qt/kernel/ui/SelectedKernels.qml | Changed from Row to RowLayout and added logic to hide recommended kernel card when it's the same as in-use kernel |
| mcp-qt/kernel/ui/KernelDelegate.qml | Added visual "Recommended" badge with purple styling that displays inline on kernel cards |
| mcp-qt/kernel/classic/KernelPage.h | Added m_cardsLayout member variable for dynamic layout management |
| mcp-qt/kernel/classic/KernelPage.cpp | Implemented hiding logic for recommended card and dynamic layout stretch to handle single vs dual card display |
| mcp-qt/kernel/classic/KernelItemWidget.h | Added m_isRecommended field to track recommended kernel state |
| mcp-qt/kernel/classic/KernelItemWidget.cpp | Updated setKernelData methods to handle isRecommended field and display recommended badge |
| mcp-qt/kernel/classic/BadgeWidget.h | Added Recommended badge type enum value |
| mcp-qt/kernel/classic/BadgeWidget.cpp | Added purple color scheme for the recommended badge |
| libmcp/kernel/KernelProvider.cpp | Removed "not in use" condition from LTS tracking logic to allow marking the in-use kernel as recommended |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Rectangle { | ||
| width: recommendedLabel.width + 16 | ||
| height: recommendedLabel.height + 4 | ||
|
|
||
| visible: root.isRecommended | ||
| color: "transparent" | ||
| border.color: Qt.rgba(0.557, 0.267, 0.678, 0.3) | ||
| border.width: 1 | ||
| radius: 4 | ||
|
|
||
| Rectangle { | ||
| anchors.fill: parent | ||
| color: Qt.rgba(0.557, 0.267, 0.678, 0.15) | ||
| radius: parent.radius | ||
| } | ||
|
|
||
| QQC2.Label { | ||
| id: recommendedLabel | ||
| anchors.centerIn: parent | ||
| text: qsTr("Recommended") | ||
| font.pointSize: 8 | ||
| font.weight: Font.Bold | ||
| font.capitalization: Font.AllUppercase | ||
| color: "#8e44ad" |
There was a problem hiding this comment.
The hardcoded RGBA values for the recommended badge color (0.557, 0.267, 0.678) don't match the RGB values in the C++ BadgeWidget. The QML rgba values (0.557, 0.267, 0.678) correspond to rgb(142, 68, 173) which matches #8e44ad in the C++ code. However, it would be better to ensure consistency by either using a shared color constant or documenting that these values must be kept in sync. Consider extracting these to a shared theme/color configuration.
| visible: root.recommendedKernel.valid | ||
| Layout.fillWidth: true | ||
| Layout.fillHeight: true | ||
| visible: root.recommendedKernel.valid && !(root.inUseKernel.isInUse && root.inUseKernel.isRecommended) |
There was a problem hiding this comment.
The visibility condition checks root.inUseKernel.isInUse which should always be true for the inUseKernel object. This check is redundant since the inUseKernel is by definition the kernel that is in use. Consider simplifying the condition to just root.recommendedKernel.valid && !root.inUseKernel.isRecommended to avoid confusion.
| visible: root.recommendedKernel.valid && !(root.inUseKernel.isInUse && root.inUseKernel.isRecommended) | |
| visible: root.recommendedKernel.valid && !root.inUseKernel.isRecommended |
|
|
||
| KernelData recommendedData = m_viewModel->recommendedKernelData(); | ||
| if (recommendedData.isValid()) { | ||
| if (recommendedData.isValid() && !(inUseData.isInUse && inUseData.isRecommended)) { |
There was a problem hiding this comment.
The visibility condition checks inUseData.isInUse which should always be true for the in-use kernel data. This check is redundant since the inUseData is by definition the kernel that is in use. Consider simplifying the condition to just recommendedData.isValid() && !inUseData.isRecommended to avoid confusion and maintain consistency with the purpose of the check.
| if (recommendedData.isValid() && !(inUseData.isInUse && inUseData.isRecommended)) { | |
| if (recommendedData.isValid() && !inUseData.isRecommended) { |
No description provided.