QMenu: Fix margins related display issues

Currently the contents margins and the menu paddings are not considered
for calculating the menu size, the positions and the size of tear-off
bar, scrollers and the positions of the menu items when scrolling the
menu, which results in the following problems when valid contents
margins and/or menu paddings are set:

- The tear off area is displayed in a wrong position. The mouse events
are not handled correctly in the tear off area. For example, when you
click in the tear off area, the menu should be torn off but nothing
happens
- For a multi-column menu, the menu width is not calculated correctly
- For a scrollable menu,
  - the menu width is not calculated correctly
  - the menu items are not displayed in correct positions
  - the scrollers are not displayed in correct positions
  - menu items are displayed on the area of borders and margins when
    scrolling the menu
  - the last menu item is not displayed above the bottom of the content
    area when scrolling the menu to the end.

The changes are to fix the problems above.

Change-Id: I7931e1088dff0029f2d4825e2aa34b4e32fdccd9
Reviewed-by: Gabriel de Dietrich <gabriel.dedietrich@qt.io>
This commit is contained in:
Dongmei Wang 2017-02-01 20:43:00 -08:00 committed by Gabriel de Dietrich
parent e1c8451ffe
commit eea585ad0b
3 changed files with 242 additions and 22 deletions

View File

@ -267,9 +267,6 @@ void QMenuPrivate::updateActionRects(const QRect &screen) const
int lastVisibleAction = getLastVisibleAction(); int lastVisibleAction = getLastVisibleAction();
int max_column_width = 0,
dh = screen.height(),
y = 0;
QStyle *style = q->style(); QStyle *style = q->style();
QStyleOption opt; QStyleOption opt;
opt.init(q); opt.init(q);
@ -279,6 +276,10 @@ void QMenuPrivate::updateActionRects(const QRect &screen) const
const int fw = style->pixelMetric(QStyle::PM_MenuPanelWidth, &opt, q); const int fw = style->pixelMetric(QStyle::PM_MenuPanelWidth, &opt, q);
const int deskFw = style->pixelMetric(QStyle::PM_MenuDesktopFrameWidth, &opt, q); const int deskFw = style->pixelMetric(QStyle::PM_MenuDesktopFrameWidth, &opt, q);
const int tearoffHeight = tearoff ? style->pixelMetric(QStyle::PM_MenuTearoffHeight, &opt, q) : 0; const int tearoffHeight = tearoff ? style->pixelMetric(QStyle::PM_MenuTearoffHeight, &opt, q) : 0;
const int base_y = vmargin + fw + topmargin + (scroll ? scroll->scrollOffset : 0) + tearoffHeight;
int max_column_width = 0;
int dh = screen.height();
int y = base_y;
//for compatibility now - will have to refactor this away //for compatibility now - will have to refactor this away
tabWidth = 0; tabWidth = 0;
@ -356,11 +357,12 @@ void QMenuPrivate::updateActionRects(const QRect &screen) const
max_column_width = qMax(max_column_width, sz.width()); max_column_width = qMax(max_column_width, sz.width());
//wrapping //wrapping
if (!scroll && if (!scroll &&
y+sz.height()+vmargin > dh - (deskFw * 2)) { y + sz.height() + vmargin + bottommargin + fw > dh - (deskFw * 2)) {
ncols++; ncols++;
y = vmargin; y = base_y;
} else {
y += sz.height();
} }
y += sz.height();
//update the item //update the item
actionRects[i] = QRect(0, 0, sz.width(), sz.height()); actionRects[i] = QRect(0, 0, sz.width(), sz.height());
} }
@ -372,9 +374,6 @@ void QMenuPrivate::updateActionRects(const QRect &screen) const
max_column_width = qMax(min_column_width, max_column_width); max_column_width = qMax(min_column_width, max_column_width);
//calculate position //calculate position
const int base_y = vmargin + fw + topmargin +
(scroll ? scroll->scrollOffset : 0) +
tearoffHeight;
int x = hmargin + fw + leftmargin; int x = hmargin + fw + leftmargin;
y = base_y; y = base_y;
@ -383,7 +382,7 @@ void QMenuPrivate::updateActionRects(const QRect &screen) const
if (rect.isNull()) if (rect.isNull())
continue; continue;
if (!scroll && if (!scroll &&
y+rect.height() > dh - deskFw * 2) { y + rect.height() + vmargin + bottommargin + fw > dh - deskFw * 2) {
x += max_column_width + hmargin; x += max_column_width + hmargin;
y = base_y; y = base_y;
} }
@ -761,7 +760,7 @@ QWidget *QMenuPrivate::topCausedWidget() const
QAction *QMenuPrivate::actionAt(QPoint p) const QAction *QMenuPrivate::actionAt(QPoint p) const
{ {
if (!q_func()->rect().contains(p)) //sanity check if (!rect().contains(p)) //sanity check
return 0; return 0;
for(int i = 0; i < actionRects.count(); i++) { for(int i = 0; i < actionRects.count(); i++) {
@ -855,6 +854,19 @@ void QMenuPrivate::drawTearOff(QPainter *painter, const QRect &rect)
q->style()->drawControl(QStyle::CE_MenuTearoff, &menuOpt, painter, q); q->style()->drawControl(QStyle::CE_MenuTearoff, &menuOpt, painter, q);
} }
QRect QMenuPrivate::rect() const
{
Q_Q(const QMenu);
QStyle *style = q->style();
QStyleOption opt(0);
opt.init(q);
const int hmargin = style->pixelMetric(QStyle::PM_MenuHMargin, &opt, q);
const int vmargin = style->pixelMetric(QStyle::PM_MenuVMargin, &opt, q);
const int fw = style->pixelMetric(QStyle::PM_MenuPanelWidth, &opt, q);
return (q->rect().adjusted(hmargin + fw + leftmargin, vmargin + fw + topmargin,
-(hmargin + fw + rightmargin), -(vmargin + fw + bottommargin)));
}
QMenuPrivate::ScrollerTearOffItem::ScrollerTearOffItem(QMenuPrivate::ScrollerTearOffItem::Type type, QMenuPrivate *mPrivate, QWidget *parent, Qt::WindowFlags f) QMenuPrivate::ScrollerTearOffItem::ScrollerTearOffItem(QMenuPrivate::ScrollerTearOffItem::Type type, QMenuPrivate *mPrivate, QWidget *parent, Qt::WindowFlags f)
: QWidget(parent, f), menuPrivate(mPrivate), scrollType(type) : QWidget(parent, f), menuPrivate(mPrivate), scrollType(type)
{ {
@ -989,7 +1001,9 @@ void QMenuPrivate::scrollMenu(QAction *action, QMenuScroller::ScrollLocation loc
} }
if (!(newScrollFlags & QMenuScroller::ScrollDown) && (scroll->scrollFlags & QMenuScroller::ScrollDown)) { if (!(newScrollFlags & QMenuScroller::ScrollDown) && (scroll->scrollFlags & QMenuScroller::ScrollDown)) {
newOffset = q->height() - (saccum - newOffset) - fw*2 - vmargin; //last item at bottom newOffset = q->height() - (saccum - newOffset) - fw*2 - vmargin - topmargin - bottommargin; //last item at bottom
if (tearoff)
newOffset -= q->style()->pixelMetric(QStyle::PM_MenuTearoffHeight, 0, q);
} }
if (!(newScrollFlags & QMenuScroller::ScrollUp) && (scroll->scrollFlags & QMenuScroller::ScrollUp)) { if (!(newScrollFlags & QMenuScroller::ScrollUp) && (scroll->scrollFlags & QMenuScroller::ScrollUp)) {
@ -1141,15 +1155,23 @@ bool QMenuPrivate::mouseEventTaken(QMouseEvent *e)
{ {
Q_Q(QMenu); Q_Q(QMenu);
QPoint pos = q->mapFromGlobal(e->globalPos()); QPoint pos = q->mapFromGlobal(e->globalPos());
QStyle *style = q->style();
QStyleOption opt(0);
opt.init(q);
const int hmargin = style->pixelMetric(QStyle::PM_MenuHMargin, &opt, q);
const int vmargin = style->pixelMetric(QStyle::PM_MenuVMargin, &opt, q);
const int fw = style->pixelMetric(QStyle::PM_MenuPanelWidth, &opt, q);
if (scroll && !activeMenu) { //let the scroller "steal" the event if (scroll && !activeMenu) { //let the scroller "steal" the event
bool isScroll = false; bool isScroll = false;
if (pos.x() >= 0 && pos.x() < q->width()) { if (pos.x() >= 0 && pos.x() < q->width()) {
for(int dir = QMenuScroller::ScrollUp; dir <= QMenuScroller::ScrollDown; dir = dir << 1) { for (int dir = QMenuScroller::ScrollUp; dir <= QMenuScroller::ScrollDown; dir = dir << 1) {
if (scroll->scrollFlags & dir) { if (scroll->scrollFlags & dir) {
if (dir == QMenuScroller::ScrollUp) if (dir == QMenuScroller::ScrollUp)
isScroll = (pos.y() <= scrollerHeight()); isScroll = (pos.y() <= scrollerHeight() + fw + vmargin + topmargin);
else if (dir == QMenuScroller::ScrollDown) else if (dir == QMenuScroller::ScrollDown)
isScroll = (pos.y() >= q->height() - scrollerHeight()); isScroll = (pos.y() >= q->height() - scrollerHeight() - fw - vmargin - bottommargin);
if (isScroll) { if (isScroll) {
scroll->scrollDirection = dir; scroll->scrollDirection = dir;
break; break;
@ -1166,7 +1188,8 @@ bool QMenuPrivate::mouseEventTaken(QMouseEvent *e)
} }
if (tearoff) { //let the tear off thingie "steal" the event.. if (tearoff) { //let the tear off thingie "steal" the event..
QRect tearRect(0, 0, q->width(), q->style()->pixelMetric(QStyle::PM_MenuTearoffHeight, 0, q)); QRect tearRect(leftmargin + hmargin + fw, topmargin + vmargin + fw, q->width() - fw * 2 - hmargin * 2 -leftmargin - rightmargin,
q->style()->pixelMetric(QStyle::PM_MenuTearoffHeight, &opt, q));
if (scroll && scroll->scrollFlags & QMenuPrivate::QMenuScroller::ScrollUp) if (scroll && scroll->scrollFlags & QMenuPrivate::QMenuScroller::ScrollUp)
tearRect.translate(0, scrollerHeight()); tearRect.translate(0, scrollerHeight());
q->update(tearRect); q->update(tearRect);
@ -2615,21 +2638,28 @@ void QMenu::paintEvent(QPaintEvent *e)
//calculate the scroll up / down rect //calculate the scroll up / down rect
const int fw = style()->pixelMetric(QStyle::PM_MenuPanelWidth, 0, this); const int fw = style()->pixelMetric(QStyle::PM_MenuPanelWidth, 0, this);
const int hmargin = style()->pixelMetric(QStyle::PM_MenuHMargin,0, this);
const int vmargin = style()->pixelMetric(QStyle::PM_MenuVMargin, 0, this);
QRect scrollUpRect, scrollDownRect; QRect scrollUpRect, scrollDownRect;
const int leftmargin = fw + hmargin + d->leftmargin;
const int topmargin = fw + vmargin + d->topmargin;
const int bottommargin = fw + vmargin + d->bottommargin;
const int contentWidth = width() - (fw + hmargin) * 2 - d->leftmargin - d->rightmargin;
if (d->scroll) { if (d->scroll) {
if (d->scroll->scrollFlags & QMenuPrivate::QMenuScroller::ScrollUp) if (d->scroll->scrollFlags & QMenuPrivate::QMenuScroller::ScrollUp)
scrollUpRect.setRect(fw, fw, width() - (fw * 2), d->scrollerHeight()); scrollUpRect.setRect(leftmargin, topmargin, contentWidth, d->scrollerHeight());
if (d->scroll->scrollFlags & QMenuPrivate::QMenuScroller::ScrollDown) if (d->scroll->scrollFlags & QMenuPrivate::QMenuScroller::ScrollDown)
scrollDownRect.setRect(fw, height() - d->scrollerHeight() - fw, width() - (fw * 2), scrollDownRect.setRect(leftmargin, height() - d->scrollerHeight() - bottommargin,
d->scrollerHeight()); contentWidth, d->scrollerHeight());
} }
//calculate the tear off rect //calculate the tear off rect
QRect tearOffRect; QRect tearOffRect;
if (d->tearoff) { if (d->tearoff) {
tearOffRect.setRect(fw, fw, width() - (fw * 2), tearOffRect.setRect(leftmargin, topmargin, contentWidth,
style()->pixelMetric(QStyle::PM_MenuTearoffHeight, 0, this)); style()->pixelMetric(QStyle::PM_MenuTearoffHeight, 0, this));
if (d->scroll && d->scroll->scrollFlags & QMenuPrivate::QMenuScroller::ScrollUp) if (d->scroll && d->scroll->scrollFlags & QMenuPrivate::QMenuScroller::ScrollUp)
tearOffRect.translate(0, d->scrollerHeight()); tearOffRect.translate(0, d->scrollerHeight());
} }
@ -2646,6 +2676,12 @@ void QMenu::paintEvent(QPaintEvent *e)
emptyArea -= QRegion(actionRect); emptyArea -= QRegion(actionRect);
QRect adjustedActionRect = actionRect; QRect adjustedActionRect = actionRect;
if (!scrollUpTearOffRect.isEmpty() && adjustedActionRect.bottom() <= scrollUpTearOffRect.top())
continue;
if (!scrollDownRect.isEmpty() && adjustedActionRect.top() >= scrollDownRect.bottom())
continue;
if (adjustedActionRect.intersects(scrollUpTearOffRect)) { if (adjustedActionRect.intersects(scrollUpTearOffRect)) {
if (adjustedActionRect.bottom() <= scrollUpTearOffRect.bottom()) if (adjustedActionRect.bottom() <= scrollUpTearOffRect.bottom())
continue; continue;

View File

@ -463,6 +463,7 @@ public:
void drawScroller(QPainter *painter, ScrollerTearOffItem::Type type, const QRect &rect); void drawScroller(QPainter *painter, ScrollerTearOffItem::Type type, const QRect &rect);
void drawTearOff(QPainter *painter, const QRect &rect); void drawTearOff(QPainter *painter, const QRect &rect);
QRect rect() const;
}; };
#endif // QT_NO_MENU #endif // QT_NO_MENU

View File

@ -55,6 +55,20 @@ static inline void centerOnScreen(QWidget *w, const QSize &size)
w->move(QGuiApplication::primaryScreen()->availableGeometry().center() - offset); w->move(QGuiApplication::primaryScreen()->availableGeometry().center() - offset);
} }
struct MenuMetrics {
int fw;
int hmargin;
int vmargin;
int tearOffHeight;
MenuMetrics(const QMenu *menu) {
fw = menu->style()->pixelMetric(QStyle::PM_MenuPanelWidth, nullptr, menu);
hmargin = menu->style()->pixelMetric(QStyle::PM_MenuHMargin, nullptr, menu);
vmargin = menu->style()->pixelMetric(QStyle::PM_MenuVMargin, nullptr, menu);
tearOffHeight = menu->style()->pixelMetric(QStyle::PM_MenuTearoffHeight, nullptr, menu);
}
};
static inline void centerOnScreen(QWidget *w) static inline void centerOnScreen(QWidget *w)
{ {
centerOnScreen(w, w->geometry().size()); centerOnScreen(w, w->geometry().size());
@ -114,6 +128,8 @@ private slots:
void QTBUG_56917_wideMenuSize(); void QTBUG_56917_wideMenuSize();
void QTBUG_56917_wideMenuScreenNumber(); void QTBUG_56917_wideMenuScreenNumber();
void QTBUG_56917_wideSubmenuScreenNumber(); void QTBUG_56917_wideSubmenuScreenNumber();
void menuSize_Scrolling_data();
void menuSize_Scrolling();
protected slots: protected slots:
void onActivated(QAction*); void onActivated(QAction*);
void onHighlighted(QAction*); void onHighlighted(QAction*);
@ -617,7 +633,10 @@ void tst_QMenu::tearOff()
QVERIFY(QTest::qWaitForWindowActive(menu.data())); QVERIFY(QTest::qWaitForWindowActive(menu.data()));
QVERIFY(!menu->isTearOffMenuVisible()); QVERIFY(!menu->isTearOffMenuVisible());
QTest::mouseClick(menu.data(), Qt::LeftButton, 0, QPoint(3, 3), 10); MenuMetrics mm(menu.data());
const int tearOffOffset = mm.fw + mm.vmargin + mm.tearOffHeight / 2;
QTest::mouseClick(menu.data(), Qt::LeftButton, 0, QPoint(10, tearOffOffset), 10);
QTRY_VERIFY(menu->isTearOffMenuVisible()); QTRY_VERIFY(menu->isTearOffMenuVisible());
QPointer<QMenu> torn = getTornOffMenu(); QPointer<QMenu> torn = getTornOffMenu();
QVERIFY(torn); QVERIFY(torn);
@ -1373,5 +1392,169 @@ void tst_QMenu::QTBUG_56917_wideSubmenuScreenNumber()
} }
} }
void tst_QMenu::menuSize_Scrolling_data()
{
QTest::addColumn<int>("numItems");
QTest::addColumn<int>("topMargin");
QTest::addColumn<int>("bottomMargin");
QTest::addColumn<int>("leftMargin");
QTest::addColumn<int>("rightMargin");
QTest::addColumn<int>("topPadding");
QTest::addColumn<int>("bottomPadding");
QTest::addColumn<int>("leftPadding");
QTest::addColumn<int>("rightPadding");
QTest::addColumn<int>("border");
QTest::addColumn<bool>("scrollable");
QTest::addColumn<bool>("tearOff");
// test data
// a single column and non-scrollable menu with contents margins + border
QTest::newRow("data0") << 5 << 2 << 2 << 2 << 2 << 0 << 0 << 0 << 0 << 2 << false << false;
// a single column and non-scrollable menu with paddings + border
QTest::newRow("data1") << 5 << 0 << 0 << 0 << 0 << 2 << 2 << 2 << 2 << 2 << false << false;
// a single column and non-scrollable menu with contents margins + paddings + border
QTest::newRow("data2") << 5 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << false << false;
// a single column and non-scrollable menu with contents margins + paddings + border + tear-off
QTest::newRow("data3") << 5 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << false << true;
// a multi-column menu with contents margins + border
QTest::newRow("data4") << 80 << 2 << 2 << 2 << 2 << 0 << 0 << 0 << 0 << 2 << false << false;
// a multi-column menu with paddings + border
QTest::newRow("data5") << 80 << 0 << 0 << 0 << 0 << 2 << 2 << 2 << 2 << 2 << false << false;
// a multi-column menu with contents margins + paddings + border
QTest::newRow("data6") << 80 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << false << false;
// a multi-column menu with contents margins + paddings + border + tear-off
QTest::newRow("data7") << 80 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << false << true;
// a scrollable menu with contents margins + border
QTest::newRow("data8") << 80 << 2 << 2 << 2 << 2 << 0 << 0 << 0 << 0 << 2 << true << false;
// a scrollable menu with paddings + border
QTest::newRow("data9") << 80 << 0 << 0 << 0 << 0 << 2 << 2 << 2 << 2 << 2 << true << false;
// a scrollable menu with contents margins + paddings + border
QTest::newRow("data10") << 80 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << true << false;
// a scrollable menu with contents margins + paddings + border + tear-off
QTest::newRow("data11") << 80 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << 2 << true << true;
}
void tst_QMenu::menuSize_Scrolling()
{
class TestMenu : public QMenu
{
public:
struct ContentsMargins
{
ContentsMargins(int l, int t, int r, int b)
: left(l), top(t), right(r), bottom(b) {}
int left;
int top;
int right;
int bottom;
};
struct MenuPaddings
{
MenuPaddings(int l, int t, int r, int b)
: left(l), top(t), right(r), bottom(b) {}
int left;
int top;
int right;
int bottom;
};
TestMenu(int numItems, const ContentsMargins &margins, const MenuPaddings &paddings,
int border, bool scrollable, bool tearOff)
: QMenu("Test Menu"),
m_numItems(numItems),
m_scrollable(scrollable),
m_tearOff(tearOff)
{
init(margins, paddings, border);
}
~TestMenu() {}
private:
void showEvent(QShowEvent *e) Q_DECL_OVERRIDE
{
QVERIFY(actions().length() == m_numItems);
int hmargin = style()->pixelMetric(QStyle::PM_MenuHMargin, nullptr, this);
int fw = style()->pixelMetric(QStyle::PM_MenuPanelWidth, nullptr, this);
int leftMargin, topMargin, rightMargin, bottomMargin;
getContentsMargins(&leftMargin, &topMargin, &rightMargin, &bottomMargin);
QRect lastItem = actionGeometry(actions().at(actions().length() - 1));
QSize s = size();
QCOMPARE( s.width(), lastItem.right() + fw + hmargin + rightMargin + 1);
QMenu::showEvent(e);
}
void init(const ContentsMargins &margins, const MenuPaddings &paddings, int border)
{
setLayoutDirection(Qt::LeftToRight);
setTearOffEnabled(m_tearOff);
setContentsMargins(margins.left, margins.top, margins.right, margins.bottom);
QString cssStyle("QMenu {menu-scrollable: ");
cssStyle += (m_scrollable ? QString::number(1) : QString::number(0));
cssStyle += "; border: ";
cssStyle += QString::number(border);
cssStyle += "px solid black; padding: ";
cssStyle += QString::number(paddings.top);
cssStyle += "px ";
cssStyle += QString::number(paddings.right);
cssStyle += "px ";
cssStyle += QString::number(paddings.bottom);
cssStyle += "px ";
cssStyle += QString::number(paddings.left);
cssStyle += "px;}";
setStyleSheet(cssStyle);
for (int i = 1; i <= m_numItems; i++)
addAction("MenuItem " + QString::number(i));
}
private:
int m_numItems;
bool m_scrollable;
bool m_tearOff;
};
QFETCH(int, numItems);
QFETCH(int, topMargin);
QFETCH(int, bottomMargin);
QFETCH(int, leftMargin);
QFETCH(int, rightMargin);
QFETCH(int, topPadding);
QFETCH(int, bottomPadding);
QFETCH(int, leftPadding);
QFETCH(int, rightPadding);
QFETCH(int, border);
QFETCH(bool, scrollable);
QFETCH(bool, tearOff);
qApp->setAttribute(Qt::AA_DontUseNativeMenuBar);
TestMenu::ContentsMargins margins(leftMargin, topMargin, rightMargin, bottomMargin);
TestMenu::MenuPaddings paddings(leftPadding, topPadding, rightPadding, bottomPadding);
TestMenu menu(numItems, margins, paddings, border, scrollable, tearOff);
menu.popup(QPoint(0,0));
centerOnScreen(&menu);
QVERIFY(QTest::qWaitForWindowExposed(&menu));
QList<QAction *> actions = menu.actions();
QCOMPARE(actions.length(), numItems);
MenuMetrics mm(&menu);
QTest::keyClick(&menu, Qt::Key_Home);
QTRY_COMPARE(menu.actionGeometry(actions.first()).y(), mm.fw + mm.vmargin + topMargin + (tearOff ? mm.tearOffHeight : 0));
QCOMPARE(menu.actionGeometry(actions.first()).x(), mm.fw + mm.hmargin + leftMargin);
if (!scrollable)
return;
QTest::keyClick(&menu, Qt::Key_End);
QTRY_COMPARE(menu.actionGeometry(actions.last()).right(),
menu.width() - mm.fw - mm.hmargin - leftMargin - 1);
QCOMPARE(menu.actionGeometry(actions.last()).bottom(),
menu.height() - mm.fw - mm.vmargin - bottomMargin - 1);
}
QTEST_MAIN(tst_QMenu) QTEST_MAIN(tst_QMenu)
#include "tst_qmenu.moc" #include "tst_qmenu.moc"