-
Notifications
You must be signed in to change notification settings - Fork 55
refactor: update display mode icons and logic #1123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideRefactors display mode components by renaming icon identifiers and assets for consistency, and updates selection logic to support more than two screens with proper sorting and dynamic icon selection. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @18202781743 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
deepin pr auto review代码审查意见:
综合以上意见,建议对代码进行如下修改: void DisPlayModeApplet::fetchPlanItems()
{
m_planItems.clear();
m_currentPlanItem = nullptr;
m_planItems << new DPItem(tr("Duplicate"), "osd_multi_screen_copy", DPItem::Merge, this);
m_planItems << new DPItem(tr("Extend"), "osd_multi_screen_extension", DPItem::Extend, this);
std::sort(outputNames.begin(), outputNames.end(), std::greater<QString>());
if (!outputNames.isEmpty()) {
for (int i = 0; i < outputNames.size(); i++) {
const auto item = outputNames[i];
const QString iconName = i < 2 ? QString("osd_multi_screen_only%1").arg(i + 1) : "osd_multi_screen_only_more";
m_planItems << new DPItem(tr("Only on %1").arg(item), item, iconName, DPItem::Single, this);
}
}
}同时,建议使用智能指针来管理动态分配的对象,例如: #include <memory>
void DisPlayModeApplet::fetchPlanItems()
{
m_planItems.clear();
m_currentPlanItem = nullptr;
m_planItems << std::make_unique<DPItem>(tr("Duplicate"), "osd_multi_screen_copy", DPItem::Merge, this);
m_planItems << std::make_unique<DPItem>(tr("Extend"), "osd_multi_screen_extension", DPItem::Extend, this);
std::sort(outputNames.begin(), outputNames.end(), std::greater<QString>());
if (!outputNames.isEmpty()) {
for (int i = 0; i < outputNames.size(); i++) {
const auto item = outputNames[i];
const QString iconName = i < 2 ? QString("osd_multi_screen_only%1").arg(i + 1) : "osd_multi_screen_only_more";
m_planItems << std::make_unique<DPItem>(tr("Only on %1").arg(item), item, iconName, DPItem::Single, this);
}
}
}这样可以避免内存泄漏,并提高代码的可读性和可维护性。 |
1. Changed icon names from 'osd_display_*' to 'osd_multi_screen_*' for better consistency 2. Modified display mode logic to support more than 2 screens by: - Always sorting output names (not just for 2 screens) - Using 'osd_multi_screen_only_more' icon when more than 2 screens exist 3. Removed old icon files and added new ones with updated naming scheme refactor: 更新显示模式图标和逻辑 1. 将图标名称从'osd_display_*'改为'osd_multi_screen_*'以提高一致性 2. 修改显示模式逻辑以支持超过2个屏幕: - 始终排序输出名称(不仅限于2个屏幕时) - 当存在超过2个屏幕时使用'osd_multi_screen_only_more'图标 3. 删除旧图标文件并添加具有更新命名方案的新图标 pms: BUG-315934
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
|
This pr force merged! (status: blocked) |
better consistency
exist
refactor: 更新显示模式图标和逻辑
pms: BUG-315934
Summary by Sourcery
Refactor display mode icons and logic to standardize naming and support more than two screens.
Enhancements:
Chores: