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

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! ;)

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?

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.

> 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ł!

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 ?

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.

> 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.

> 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).

> - 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?

> > - 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?

> 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?

> 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.

> > - 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?

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

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

Fix for that is coming.

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