Merge lp:~saviq/unity-2d/unmaximize-on-drag into lp:unity-2d/3.0

[Michał Sawicz]

[Michał Sawicz (saviq)]

Double clicking and dragging works as expected, with one exception that after dragging there seems to be a period of time during which, or a count of events, the panel doesn't receive Hover events.

I will yet try to hunt this down and will take any and all comments on why that might happen.

BTW: Hi! ;)

[Aurélien Gâteau (agateau)]

Hi Michał,

Thanks for your work. It works well here, but there are a few changes in the code I would like to suggest:

- The code you added to ClosedMenuBarHelper should be in MenuBarWidget: I would like ClosedMenuBarHelper to remain a small helper class to notice when a menu in the menubar is closed.

- Prefer using "const QPoint&" instead of "QPoint*" in WindowHelper::drag() and in MenuBarWidget::menuBarDragged(). Using QPoint* makes it look like the code is going to modify the point.

- Prefer using a forward declaration of QPoint class in windowhelper.h instead of #including it.

- I am worried with r564: it looks like a duplication of the code you added in MenuBarWidget. Maybe it is possible to do most of the work from MenuBarApplet?

review: Needs Fixing

[Ugo Riboni (uriboni)]

Hey Michal, good to see your contributions again :)
Sorry if it took a while to review this one, but UDS was in the way last week.

Functionally it works well, besides the issue you mention when dragging.
There's just one small glitch that I sometimes notice with gnome-terminal windows: they don't un-maximize to the same size they were before randomly. But i have not seen any other app do that, so it might be some other weird quirk of gnome-terminal.

May I suggest that while you try to figure out what is that is making the drag not work properly you split off the part of the patch that just takes care of the double click and submit that for review so that it can be taken in ? It's already a good improvement over not having neither doubleclick nor drag.

Some comment on code style will follow below.

[Olivier Tilloy (osomon)]

> Functionally it works well, besides the issue you mention when dragging.
> There's just one small glitch that I sometimes notice with gnome-terminal
> windows: they don't un-maximize to the same size they were before randomly.
> But i have not seen any other app do that, so it might be some other weird
> quirk of gnome-terminal.

This is a known issue (bug #684958), I don’t think it is related to Michał’s work.

BTW, hey Michał!

[Ugo Riboni (uriboni)]

124 + case QEvent::MouseButtonDblClick:
125 + QMetaObject::invokeMethod(m_widget, "menuBarDblClicked");

and

145 + QMetaObject::invokeMethod(m_widget, "menuBarDragged",
146 + Q_ARG(QPoint*, &m_dragStartPosition));

Is there any particular reason why you used invokeMethod instead of using qobject_cast<MenuBarWidget*>(m_widget) and they if the cast succeeded just call the method directly ?

review: Needs Information

[Ugo Riboni (uriboni)]

173 + void menuBarDblClicked();
174 + void menuBarDragged(QPoint* pos);

Please use DoubleClicked instead of DblClicked. We generally try to use non-abbreviated names in unity-2d identifiers as a rule.

review: Needs Fixing

[Michał Sawicz (saviq)]

> Is there any particular reason why you used invokeMethod instead of using
> qobject_cast<MenuBarWidget*>(m_widget) and they if the cast succeeded just
> call the method directly ?

I was just following what the rest of the code did. Seemed like a good way to encapsulate things, but since it's supposed to go into MenuBarWidget I might as well call it directly.

[Olivier Tilloy (osomon)]

> 173 + void menuBarDblClicked();
> 174 + void menuBarDragged(QPoint* pos);
>
> Please use DoubleClicked instead of DblClicked. We generally try to use non-
> abbreviated names in unity-2d identifiers as a rule.

This is consistent with the name of the event as exposed by Qt (QEvent::MouseButtonDblClick, see http://doc.qt.nokia.com/qevent.html#Type-enum).

[Michał Sawicz (saviq)]

> - The code you added to ClosedMenuBarHelper should be in MenuBarWidget: I
> would like ClosedMenuBarHelper to remain a small helper class to notice when a
> menu in the menubar is closed.

For some reason I didn't get the events in MenuBarWidget, but now you mentioned it they obviously should happen there. Maybe that's even the cause of the Hover events getting lost.

> - Prefer using "const QPoint&" instead of "QPoint*" in WindowHelper::drag()
> and in MenuBarWidget::menuBarDragged(). Using QPoint* makes it look like the
> code is going to modify the point.
>
> - Prefer using a forward declaration of QPoint class in windowhelper.h instead
> of #including it.

Noted.

> - I am worried with r564: it looks like a duplication of the code you added in
> MenuBarWidget. Maybe it is possible to do most of the work from MenuBarApplet?

Yes it definitely is duplication that I disliked, too. IMO there should simply be a 'QDragEvent' available that does the heavy lifting. Since the widgets alternate in visibility, what approach do you propose to get rid of that?

[Michał Sawicz (saviq)]

> > - The code you added to ClosedMenuBarHelper should be in MenuBarWidget: I
> > would like ClosedMenuBarHelper to remain a small helper class to notice when a
> > menu in the menubar is closed.
>
> For some reason I didn't get the events in MenuBarWidget, but now you
> mentioned it they obviously should happen there. Maybe that's even the cause
> of the Hover events getting lost.

I'm probably missing something obvious, but I'm not getting any mouse events on MenuBarWidget. MenuBarClosedHelper is installed as a filter event for MenuBarWidget, but that shouldn't prevent the latter to receive events as long as the filter doesn't return true, should it?

> > - I am worried with r564: it looks like a duplication of the code you added in
> > MenuBarWidget. Maybe it is possible to do most of the work from MenuBarApplet?
>
> Yes it definitely is duplication that I disliked, too. IMO there should simply
> be a 'QDragEvent' available that does the heavy lifting. Since the widgets
> alternate in visibility, what approach do you propose to get rid of that?

We could probably install MenuBarWidget as an event filter on AppNameApplet?

[Michał Sawicz (saviq)]

> May I suggest that while you try to figure out what is that is making the drag
> not work properly you split off the part of the patch that just takes care of
> the double click and submit that for review so that it can be taken in ? It's
> already a good improvement over not having neither doubleclick nor drag.

Since my approach here needs to be revised (see Aurélien's first comment), I'm gonna
keep that on hold until then, ok?

[Ugo Riboni (uriboni)]

> Since my approach here needs to be revised (see Aurélien's first comment), I'm
> gonna keep that on hold until then, ok?

Sounds good to me.

[Aurélien Gâteau (agateau)]

> > - The code you added to ClosedMenuBarHelper should be in MenuBarWidget: I
> > would like ClosedMenuBarHelper to remain a small helper class to notice when
> a
> > menu in the menubar is closed.
>
> For some reason I didn't get the events in MenuBarWidget, but now you
> mentioned it they obviously should happen there. Maybe that's even the cause
> of the Hover events getting lost.

I think it is because the MenuBarWidget eventFilter is installed as a filter on the menus rather than on the menubar itself (see MenuBarWidget::updateMenuBar()).

> > - Prefer using "const QPoint&" instead of "QPoint*" in WindowHelper::drag()
> > and in MenuBarWidget::menuBarDragged(). Using QPoint* makes it look like the
> > code is going to modify the point.
> >
> > - Prefer using a forward declaration of QPoint class in windowhelper.h
> instead
> > of #including it.
>
> Noted.
>
> > - I am worried with r564: it looks like a duplication of the code you added
> in
> > MenuBarWidget. Maybe it is possible to do most of the work from
> MenuBarApplet?
>
> Yes it definitely is duplication that I disliked, too. IMO there should simply
> be a 'QDragEvent' available that does the heavy lifting. Since the widgets
> alternate in visibility, what approach do you propose to get rid of that?

Maybe you could create a separate class which would implement the double-click and drag behavior and would be installed as an event filter on both the menu bar (the QMenuBar, not MenuBarWidget) and the panel?

[Dmitry Shachnev (mitya57)]

@Michał: It's not fixed yet. Can you re-base and fix your code for the current Oneiric version of Unity 2D?

[Michał Sawicz (saviq)]

@Dmitry I have that on my backlog, but unfortunately it's not just rebasing the code, we need to refactor it quite a bit.

[Michał Sawicz (saviq)]

Fix for that is coming.

review: Disapprove

[Michał Sawicz (saviq)]

New mr: https://code.launchpad.net/~saviq/unity-2d/titlebar-dblclick/+merge/76759

Unmerged revisions

564. By Michał Sawicz on 2011-05-15

Also drag windows that don't have a menuBar on the panel.

563. By Michał Sawicz on 2011-05-15

Signal the window manager that the window is to be moved when titlebar
is dragged.

562. By Michał Sawicz on 2011-05-14

Unmaximize windows when their menubar is dragged down from the panel

561. By Michał Sawicz on 2011-05-14

Unmaximize windows when their menu bar is double clicked

=== modified file 'panel/applets/appname/appnameapplet.cpp'
--- panel/applets/appname/appnameapplet.cpp	2011-04-26 15:20:08 +0000
+++ panel/applets/appname/appnameapplet.cpp	2011-05-15 16:32:32 +0000
@@ -43,6 +43,7 @@
 #include <QPainter>
 #include <QApplication>
 #include <QDesktopWidget>
+#include <QMouseEvent>
 
 static const char* METACITY_DIR = "/usr/share/themes/Ambiance/metacity-1";
 
@@ -130,6 +131,8 @@
     QLabel* m_label;
     WindowHelper* m_windowHelper;
     MenuBarWidget* m_menuBarWidget;
+    QPoint m_dragStartPosition;
+    bool m_dragInProgress;
 
     void setupLabel()
     {
@@ -177,6 +180,10 @@
             q, SLOT(updateWidgets()));
         QObject::connect(m_menuBarWidget, SIGNAL(isEmptyChanged()),
             q, SLOT(updateWidgets()));
+        QObject::connect(m_menuBarWidget, SIGNAL(menuBarDblClicked()),
+            m_windowHelper, SLOT(unmaximize()));
+        QObject::connect(m_menuBarWidget, SIGNAL(menuBarDragged(QPoint*)),
+            m_windowHelper, SLOT(drag(QPoint*)));
     }
 
     void setupKeyboardModifiersMonitor()
@@ -274,12 +281,48 @@
     case QEvent::HoverLeave:
         updateWidgets();
         break;
+    case QEvent::MouseButtonDblClick:
+    case QEvent::MouseButtonPress:
+    case QEvent::MouseMove:
+        appNameAppletMouseEvent(static_cast<QMouseEvent*>(event));
+        break;
     default:
         break;
     }
     return false;
 }
 
+void AppNameApplet::appNameAppletMouseEvent(QMouseEvent* event)
+{
+    switch(event->type()) {
+    case QEvent::MouseButtonDblClick:
+        d->m_windowHelper->unmaximize();
+        break;
+    case QEvent::MouseButtonPress:
+        if (event->button() == Qt::LeftButton) {
+            d->m_dragStartPosition = event->pos();
+            d->m_dragInProgress = true;
+        }
+        break;
+    case QEvent::MouseMove:
+        if (!d->m_dragInProgress) {
+            break;
+        }
+        if (!(event->buttons() & Qt::LeftButton)) {
+            break;
+        }
+        if ((event->pos() - d->m_dragStartPosition).manhattanLength()
+                < QApplication::startDragDistance()) {
+            break;
+        }
+        d->m_dragInProgress = false;
+        d->m_windowHelper->drag(&d->m_dragStartPosition);
+        break;
+    default:
+        break;
+    }
+}
+
 } // namespace
 
 #include "appnameapplet.moc"

=== modified file 'panel/applets/appname/appnameapplet.h'
--- panel/applets/appname/appnameapplet.h	2011-01-15 01:41:03 +0000
+++ panel/applets/appname/appnameapplet.h	2011-05-15 16:32:32 +0000
@@ -43,6 +43,7 @@
 protected:
     bool event(QEvent*); // reimp
     bool eventFilter(QObject*, QEvent*); // reimp
+    void appNameAppletMouseEvent(QMouseEvent*);
 
 private Q_SLOTS:
     void updateWidgets();

=== modified file 'panel/applets/appname/menubarwidget.cpp'
--- panel/applets/appname/menubarwidget.cpp	2011-04-13 21:17:38 +0000
+++ panel/applets/appname/menubarwidget.cpp	2011-05-15 16:32:32 +0000
@@ -37,6 +37,7 @@
 
 // Qt
 #include <QActionEvent>
+#include <QMouseEvent>
 #include <QHBoxLayout>
 #include <QLabel>
 #include <QMenuBar>
@@ -271,6 +272,11 @@
             case QEvent::ActionChanged:
                 menuBarActionEvent(static_cast<QActionEvent*>(event));
                 break;
+            case QEvent::MouseButtonDblClick:
+            case QEvent::MouseButtonPress:
+            case QEvent::MouseMove:
+                menuBarMouseEvent(static_cast<QMouseEvent*>(event));
+                break;
             default:
                 break;
         }
@@ -322,4 +328,36 @@
     }
 }
 
+void MenuBarClosedHelper::menuBarMouseEvent(QMouseEvent* event)
+{
+    switch (event->type()) {
+    case QEvent::MouseButtonDblClick:
+        QMetaObject::invokeMethod(m_widget, "menuBarDblClicked");
+        break;
+    case QEvent::MouseButtonPress:
+        if (event->button() == Qt::LeftButton) {
+            m_dragStartPosition = event->pos();
+            m_dragInProgress = true;
+        }
+        break;
+    case QEvent::MouseMove:
+        if (!m_dragInProgress) {
+            break;
+        }
+        if (!(event->buttons() & Qt::LeftButton)) {
+            break;
+        }
+        if ((event->pos() - m_dragStartPosition).manhattanLength()
+                < QApplication::startDragDistance()) {
+            break;
+        }
+        m_dragInProgress = false;
+        QMetaObject::invokeMethod(m_widget, "menuBarDragged",
+                                  Q_ARG(QPoint*, &m_dragStartPosition));
+        break;
+    default:
+        break;
+    }
+}
+
 #include "menubarwidget.moc"

=== modified file 'panel/applets/appname/menubarwidget.h'
--- panel/applets/appname/menubarwidget.h	2011-04-13 21:17:38 +0000
+++ panel/applets/appname/menubarwidget.h	2011-05-15 16:32:32 +0000
@@ -59,7 +59,10 @@
 
 private:
     MenuBarWidget* m_widget;
+    QPoint m_dragStartPosition;
+    bool m_dragInProgress;
     void menuBarActionEvent(QActionEvent*);
+    void menuBarMouseEvent(QMouseEvent*);
 };
 
 class MenuBarWidget : public QWidget
@@ -74,6 +77,8 @@
 Q_SIGNALS:
     void menuBarClosed();
     void isEmptyChanged();
+    void menuBarDblClicked();
+    void menuBarDragged(QPoint* pos);
 
 protected:
     bool eventFilter(QObject*, QEvent*); // reimp

=== modified file 'panel/applets/appname/windowhelper.cpp'
--- panel/applets/appname/windowhelper.cpp	2011-04-13 14:04:45 +0000
+++ panel/applets/appname/windowhelper.cpp	2011-05-15 16:32:32 +0000
@@ -43,6 +43,11 @@
 #include <QApplication>
 #include <QDesktopWidget>
 
+// X11
+#include <X11/Xlib.h>
+#include <X11/Xatom.h>
+#include <QX11Info>
+
 struct WindowHelperPrivate
 {
     WnckWindow* m_window;
@@ -157,4 +162,27 @@
     wnck_window_unmaximize(d->m_window);
 }
 
+void WindowHelper::drag(QPoint* pos)
+{
+    // this code IMO should ultimately belong to wnck
+    if (wnck_window_is_maximized(d->m_window)) {
+        XEvent xev;
+        QX11Info info;
+        Atom netMoveResize = XInternAtom(QX11Info::display(), "_NET_WM_MOVERESIZE", false);
+        xev.xclient.type = ClientMessage;
+        xev.xclient.message_type = netMoveResize;
+        xev.xclient.display = QX11Info::display();
+        xev.xclient.window = wnck_window_get_xid(d->m_window);
+        xev.xclient.format = 32;
+        xev.xclient.data.l[0] = pos->x();
+        xev.xclient.data.l[1] = pos->y();
+        xev.xclient.data.l[2] = 8; // _NET_WM_MOVERESIZE_MOVE
+        xev.xclient.data.l[3] = Button1;
+        xev.xclient.data.l[4] = 0;
+        XUngrabPointer(QX11Info::display(), QX11Info::appTime());
+        XSendEvent(QX11Info::display(), QX11Info::appRootWindow(info.screen()), false,
+                   SubstructureRedirectMask | SubstructureNotifyMask, &xev);
+    }
+}
+
 #include "windowhelper.moc"

=== modified file 'panel/applets/appname/windowhelper.h'
--- panel/applets/appname/windowhelper.h	2011-03-30 16:21:18 +0000
+++ panel/applets/appname/windowhelper.h	2011-05-15 16:32:32 +0000
@@ -26,6 +26,7 @@
 
 // Qt
 #include <QObject>
+#include <QPoint>
 
 struct WindowHelperPrivate;
 class WindowHelper : public QObject
@@ -44,6 +45,7 @@
     void close();
     void minimize();
     void unmaximize();
+    void drag(QPoint* pos);
 
 private Q_SLOTS:
     void update();