[Marble-devel] Review Request: Measure Tool / MarbleWidgetPopupMenu Refactoring

[Jens-Michael Hoffmann]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/3806/#review5268
-----------------------------------------------------------

Ship it!


I tested the patch and it works fine for me.

However I'm not sure about the appoach.
The MarbleWidgetInputHandler handles mouse and keyboard input according to =
the class documentation.
So I'm not sure if MarbleWidget(Default)InputHandler is the right place for=
 addMeasurePoint
and setNumberOfMeasurePoints.

I agree that there is need for a way to extend the widges context menu. Bes=
ides this whole area
needs some refactoring imho (and someone starting to work on it), so I thin=
k this could go in,
even if it needs to reworked later.

- jmho


On 2010-04-25 08:49:09, Dennis Nienh=C3=BCser wrote:
> =

> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3806/
> -----------------------------------------------------------
> =

> (Updated 2010-04-25 08:49:09)
> =

> =

> Review request for marble.
> =

> =

> Summary
> -------
> =

> I'd like to re-use MarbleWidgetPopupMenu for routing to have a consistent=
 base context menu. Currently the measure tool is coupled tightly to the st=
andard context menu (MarbleWidgetPopupMenu). This is inconvenient as the me=
asure actions are disturbing for a routing context menu.
> =

> The patch moves the measure tool related stuff to MarbleWidgetDefaultInpu=
tHandler and provides a little hook in MarbleWidgetPopupMenu to add custom =
actions. We probably need to extend that in the future, but adding it now s=
houldn't hurt as MarbleWidgetPopupMenu is not part of the public library.
> =

> In the long term we should make the measure tool a plugin and give plugin=
s the ability to extend the menus. This requires changes to the public API =
which I think is a bit late for 4.5. With this patch, however, a routing co=
ntext menu can already be integrated neatly.
> =

> =

> Diffs
> -----
> =

>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetInputHandler.h 1118077 =

>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetInputHandler.cpp 1118077 =

>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetPopupMenu.h 1118077 =

>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetPopupMenu.cpp 1118077 =

> =

> Diff: http://reviewboard.kde.org/r/3806/diff
> =

> =

> Testing
> -------
> =

> Measure tool behaves the same, routing context menu using MarbleWidgetPop=
upMenu works without any measure actions in it (latter is not part of this =
patch).
> =

> =

> Thanks,
> =

> Dennis
> =

>

-------------------------------------------------------------