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

[jensmh at gmx.de]

-----------------------------------------------------------
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. Besides this whole area
needs some refactoring imho (and someone starting to work on it), so I think this could go in,
even if it needs to reworked later.

- jmho


On 2010-04-25 08:49:09, Dennis Nienhüser 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 standard context menu (MarbleWidgetPopupMenu). This is inconvenient as the measure actions are disturbing for a routing context menu.
> 
> The patch moves the measure tool related stuff to MarbleWidgetDefaultInputHandler and provides a little hook in MarbleWidgetPopupMenu to add custom actions. We probably need to extend that in the future, but adding it now shouldn'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 plugins 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 context 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 MarbleWidgetPopupMenu works without any measure actions in it (latter is not part of this patch).
> 
> 
> Thanks,
> 
> Dennis
> 
>