qt: various corrections in sort menu
See individual commits.
Request review @chub.
Merge request reports
Activity
added MRStatus::Reviewable label
added Component::Interface: Qt label
added MRStatus::InReview label and removed MRStatus::Reviewable label
1015 if (key == currentKey && currentOrder == order) 1016 action->setChecked(true); 1017 }; 1018 1019 for (const QVariant& it: m_controler->getSortKeyTitleList()) 1020 { 1021 const QVariantMap varmap = it.toMap(); 1022 1023 auto key = static_cast<PlaylistController::SortKey>(varmap.value("key").toInt()); 1024 QString label = varmap.value("text").toString(); 1025 1026 addSortAction(qtr("%1 Ascending").arg(label), key, PlaylistController::SORT_ORDER_ASC); 1027 addSortAction(qtr("%1 Descending").arg(label), key, PlaylistController::SORT_ORDER_DESC); 1028 } 1001 action = m_menu->addAction( qtr("Sort menu...") ); 1002 connect(action, &QAction::triggered, this, &PlaylistContextMenu::requestOpenSortMenu); - It is a really bad idea to re-implement sort menu multiple times for maintenance reasons. In fact, the current broken status proves that point. It seems you agree with me in this regard.
- It seems that most applications do not provide sort sub menu in the right click context menu, so in my opinion it is not relevant if it is a sub-menu, or it is an independent menu. The point is providing a way for the keyboard to trigger sorting, and as the toolbar can not be focused, this is a way to do that.
- Sort control manages the sort menu itself. I tried to embed the menu before, but decided to not do that because I did not want to map the sorting properties multiple times (as
SortMenuis generic).SortControldoes that already:
const model = root.model.map(function(modelData) { const checked = modelData.criteria === sortKey const order = checked ? root.sortOrder : undefined return { "text": modelData.text, "checked": checked, "order": order } })-
the issue is that the SortMenu (the C++ class) is only designed to be used from QML. This make reusing the menu in other scenario difficult. perhaps we should split the menu creation (having static QMenu* SortMenu::buildMenu(...)` and the popup function which instantiate and shows the menu for QML purpose. this would make the menu reusable and embed-able in other menus.
Menu creation still needs mapped model. That is passed from
SortControl, so we still can not add submenu in the context menu as you would need to access the sort control.Note that this is an already existing bug and I would prefer it to be fixed as soon as possible. It can be improved in the future.
I made the sort control focusable in !6280 (merged). If you want, I can remove the sort entry in the context menu. Sorting can be done through the sort button directly with keyboard, there is not much reason anymore to keep the entry in the context menu.
requested review from @chub
mentioned in merge request !6590 (merged)
changed milestone to %4.0
added MRStatus::Stale label and removed MRStatus::InReview label
