OpenMaps improvements: please review

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

OpenMaps improvements: please review

Felix Natter
Administrator
hello,

I made two changes for master (diff attached), please review:

(1) you can select a node more than once in the openmaps plugin mapview
   (Blair had a look at it)
  - instead of remembering locationCount (we are only interested in 1
    location), remember the last MapMarker set
  - setMarkerAtLocation() removes old marker if set
  - change
   org.freeplane.plugin.openmaps.OpenMapsNodeHook.locationChoosenAction(Coordinate,
   int): do not unregister the listener at the first location selection
    --> Potential memory leak?

(2) You click on the globe icon to open the openmaps plugin dialog
  - in  org.freeplane.view.swing.ui.DefaultNodeMouseMotionListener.mouseClicked(MouseEvent),
    I call:
    mc.getAction("OpenMapsViewLocation").actionPerformed(new ActionEvent(node, 0, ""));
  - The ActionView hack is necessary because normally, the
    OpenMapsViewLocation action looks at the currently selected node,
    which may not be the node belonging to the globe icon that the
    user clicked on
  - MainView.isInFollowLinkRegion() was used to check that a node
    contained a link *and* that the click was within the Icon region.
    --> I changed this to isClickableLink() (same behavior) and pulled
    out isInIconRegion()
  - guard the above action call by isInLinkRegion():

                                if (component.isInIconRegion(e.getX())) {
                                        // this action is a no-op if there is no OpenMapsExtension on the clicked node
                                        mc.getAction("OpenMapsViewLocation").actionPerformed(new ActionEvent(node, 0, ""));
                                }

Thanks and Best Regards,
--
Felix Natter

diff --git a/freeplane/src/org/freeplane/view/swing/map/MainView.java b/freeplane/src/org/freeplane/view/swing/map/MainView.java
index 35f6ef4..ee6b1ed 100644
--- a/freeplane/src/org/freeplane/view/swing/map/MainView.java
+++ b/freeplane/src/org/freeplane/view/swing/map/MainView.java
@@ -178,11 +178,16 @@ public abstract class MainView extends ZoomableLabel {
  return getNodeView().getZoomedFoldingSymbolHalfWidth();
  }
 
- public boolean isInFollowLinkRegion(final double xCoord) {
+ public boolean isClickableLink(final double xCoord) {
  final NodeView nodeView = getNodeView();
  final NodeModel model = nodeView.getModel();
  if (NodeLinks.getValidLink(model) == null)
  return false;
+ return isInIconRegion(xCoord);
+ }
+
+ public boolean isInIconRegion(final double xCoord)
+ {
  Rectangle iconR = ((ZoomableLabelUI)getUI()).getIconR(this);
  return xCoord >= iconR.x && xCoord < iconR.x + iconR.width;
  }
@@ -663,7 +668,7 @@ public abstract class MainView extends ZoomableLabel {
  return MouseArea.MOTION;
  if(isInFoldingRegion(point))
  return MouseArea.FOLDING;
- if(isInFollowLinkRegion(x))
+ if(isClickableLink(x))
  return MouseArea.LINK;
  return MouseArea.DEFAULT;
  }
diff --git a/freeplane/src/org/freeplane/view/swing/ui/DefaultNodeMouseMotionListener.java b/freeplane/src/org/freeplane/view/swing/ui/DefaultNodeMouseMotionListener.java
index 33ab49d..5ac03e4 100644
--- a/freeplane/src/org/freeplane/view/swing/ui/DefaultNodeMouseMotionListener.java
+++ b/freeplane/src/org/freeplane/view/swing/ui/DefaultNodeMouseMotionListener.java
@@ -1,6 +1,7 @@
 package org.freeplane.view.swing.ui;
 
 import java.awt.Cursor;
+import java.awt.event.ActionEvent;
 import java.awt.event.MouseEvent;
 import java.net.URI;
 
@@ -10,6 +11,7 @@ import org.freeplane.core.resources.ResourceController;
 import org.freeplane.core.ui.DoubleClickTimer;
 import org.freeplane.core.ui.IMouseListener;
 import org.freeplane.core.ui.components.AutoHide;
+import org.freeplane.core.undo.IActor;
 import org.freeplane.core.util.Compat;
 import org.freeplane.core.util.LogUtils;
 import org.freeplane.features.link.LinkController;
@@ -71,12 +73,17 @@ public class DefaultNodeMouseMotionListener implements IMouseListener {
  final MapController mapController = mc.getMapController();
  if(e.getButton() == 1){
  if(plainEvent){
- if (component.isInFollowLinkRegion(e.getX())) {
+ if (component.isClickableLink(e.getX())) {
  LinkController.getController(mc).loadURL(node, e);
  e.consume();
  return;
  }
-
+
+ if (component.isInIconRegion(e.getX())) {
+ // this action is a no-op if there is no OpenMapsExtension on the clicked node
+ mc.getAction("OpenMapsViewLocation").actionPerformed(new ActionEvent(node, 0, ""));
+ }
+
  final String link = component.getLink(e.getPoint());
  if (link != null) {
  doubleClickTimer.start(new Runnable() {
@@ -163,7 +170,7 @@ public class DefaultNodeMouseMotionListener implements IMouseListener {
  boolean followLink = link != null;
  Controller currentController = Controller.getCurrentController();
         if(! followLink){
-         followLink = node.isInFollowLinkRegion(e.getX());
+         followLink = node.isClickableLink(e.getX());
          if(followLink){
  link = LinkController.getController(currentController.getModeController()).getLinkShortText(node.getNodeView().getModel());
          }
diff --git a/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/OpenMapsNodeHook.java b/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/OpenMapsNodeHook.java
index 02b1c8e..85c5cc7 100644
--- a/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/OpenMapsNodeHook.java
+++ b/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/OpenMapsNodeHook.java
@@ -48,19 +48,23 @@ public class OpenMapsNodeHook extends PersistentNodeHook implements LocationChoo
  }
 
 
- //Called when a location is chosen in the OpenMapsDialog - Only one location may be chosen at a time
+ //Called when a location is chosen in the OpenMapsDialog
  public void locationChoosenAction(Coordinate locationChoosen, int zoom) {
  addChoosenLocationToSelectedNode(locationChoosen, zoom);
- map.getController().removeLocationChoosenListener(this);
+// map.getController().removeLocationChoosenListener(this);
  }
 
- public void viewCurrentlySelectedLocation() {
- final NodeModel node = getCurrentlySelectedNode();
- OpenMapsExtension openMapsExtension = (OpenMapsExtension) node.getExtension(OpenMapsExtension.class);
+ public void viewCurrentlySelectedLocation(final NodeModel targetNode) {
+ OpenMapsExtension openMapsExtension;
+ if (targetNode == null)
+ openMapsExtension = (OpenMapsExtension) getCurrentlySelectedNode().getExtension(OpenMapsExtension.class);
+ else
+ openMapsExtension = (OpenMapsExtension) targetNode.getExtension(OpenMapsExtension.class);
 
  if (openMapsExtension != null) {
  map = new OpenMapsDialog();
  map.showZoomToLocation(openMapsExtension.getLocation(), openMapsExtension.getZoom());
+ map.getController().addLocationChoosenListener(this);
  }
 
  }
diff --git a/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/actions/ViewOpenMapsAction.java b/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/actions/ViewOpenMapsAction.java
index b49c171..7d952b8 100644
--- a/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/actions/ViewOpenMapsAction.java
+++ b/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/actions/ViewOpenMapsAction.java
@@ -3,6 +3,7 @@ package org.freeplane.plugin.openmaps.actions;
 import java.awt.event.ActionEvent;
 
 import org.freeplane.core.ui.AFreeplaneAction;
+import org.freeplane.features.map.NodeModel;
 import org.freeplane.plugin.openmaps.OpenMapsNodeHook;
 
 /**
@@ -18,8 +19,11 @@ public class ViewOpenMapsAction extends AFreeplaneAction {
  super(actionIdentifier);
  nodeHookReference = nodeHook;
  }
-
+
  public void actionPerformed(ActionEvent event) {
- nodeHookReference.viewCurrentlySelectedLocation();
+ if (event.getSource() == null || !(event.getSource() instanceof NodeModel))
+ nodeHookReference.viewCurrentlySelectedLocation(null); // use currently selected node!
+ else
+ nodeHookReference.viewCurrentlySelectedLocation((NodeModel)(event.getSource())); // use node that was clicked!
  }
 }
diff --git a/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/mapelements/OpenMapsController.java b/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/mapelements/OpenMapsController.java
index a3e9cfb..515d647 100644
--- a/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/mapelements/OpenMapsController.java
+++ b/freeplane_plugin_openmaps/src/org/freeplane/plugin/openmaps/mapelements/OpenMapsController.java
@@ -12,6 +12,7 @@ import org.openstreetmap.gui.jmapviewer.DefaultMapController;
 import org.openstreetmap.gui.jmapviewer.JMapViewer;
 import org.openstreetmap.gui.jmapviewer.MapMarkerDot;
 import org.openstreetmap.gui.jmapviewer.OsmMercator;
+import org.openstreetmap.gui.jmapviewer.interfaces.MapMarker;
 
 /**
  * @author Blair Archibald
@@ -19,14 +20,14 @@ import org.openstreetmap.gui.jmapviewer.OsmMercator;
 public class OpenMapsController extends DefaultMapController implements MouseListener {
 
  private Set<LocationChoosenListener> Listeners;
- private int locationCount;
+ private MapMarker currentLocation;
 
  public OpenMapsController(JMapViewer map) {
  super(map);
  configureButtons();
 
  Listeners = new HashSet<LocationChoosenListener>();
- locationCount = 0;
+ currentLocation = null;
  }
 
  private void configureButtons() {
@@ -38,10 +39,7 @@ public class OpenMapsController extends DefaultMapController implements MouseLis
  public void mouseClicked(MouseEvent e) {
  if (e.getClickCount() == 2 && e.getButton() == MouseEvent.BUTTON1) {
  final Coordinate locationChoosen = getSelectedLocation(e.getPoint());
- if (locationCount < 1) {
- addMarkerToLocation(locationChoosen);
- locationCount++;
- }
+ setMarkerAtLocation(locationChoosen);
  sendLocation(locationChoosen, getCurrentZoomLevel());
  }
  }
@@ -68,15 +66,14 @@ public class OpenMapsController extends DefaultMapController implements MouseLis
  return map.getZoom();
  }
 
- private void addMarkerToLocation(final Coordinate locationChoosen) {
- map.addMapMarker(new MapMarkerDot(locationChoosen.getLat(), locationChoosen.getLon()));
+ private void setMarkerAtLocation(final Coordinate locationChoosen) {
+ if (currentLocation != null)
+ map.removeMapMarker(currentLocation);
+ map.addMapMarker(currentLocation = new MapMarkerDot(locationChoosen.getLat(), locationChoosen.getLon()));
  }
 
  public void zoomToLocation(Coordinate location, int zoom) {
- if(locationCount == 0) {
- addMarkerToLocation(location);
- locationCount++;
- }
+ setMarkerAtLocation(location);
 
  // this method is not available in JMapViewer >= 1.03!
 // map.setDisplayPositionByLatLon(new Point(map.getWidth() / 2, map.getHeight() / 2), location.getLat(), location.getLon(), zoom);
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: OpenMaps improvements: please review

Felix Natter
Administrator
hi,

any volunteer for reviewing? :-)

Thanks,
Felix
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: OpenMaps improvements: please review

BArchibald
Hi Felix,

I'll have a look at this tonight and get back to you asap! Is it ready for testing? (if so I'll try it as a patch else I'll just read through it)

Many thanks,
Blair


On 15 July 2014 19:39, Felix Natter [via Freeplane Developer] <[hidden email]> wrote:
hi,

any volunteer for reviewing? :-)

Thanks,
Felix


If you reply to this email, your message will be added to the discussion below:
http://freeplane-developer.996965.n3.nabble.com/OpenMaps-improvements-please-review-tp470p471.html
To start a new topic under Freeplane Developer, email [hidden email]
To unsubscribe from Freeplane Developer, click here.
NAML

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: OpenMaps improvements: please review

Felix Natter
Administrator
"BArchibald [via Freeplane Developer]"
<[hidden email]> writes:

> Hi Felix,

hi Blair,

> I'll have a look at this tonight and get back to you asap! Is it ready for testing? (if so I'll try it as
> a patch else I'll just read through it)

Yes, it works for me :-)

Thanks and Best Regards,
--
Felix Natter
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: OpenMaps improvements: please review

BArchibald
Hi Felix,

I've imported the code and it all seems to be working very nicely, fast and does what it says on the tin! (I use Xmonad as my window manager so the dialogue always opens in the top left of the screen, I guess this is a tiling window manager bug not in freeplane?)

Code Review:

In "isInIconRegion" I would defiantly change this line: "((ZoomableLabelUI)getUI()).getIconR(this);", it's confusing to look at. I realise this isn't part of the  patch but would be nice to tidy up while we are here. (just introduce another local variable for the UI, javac will optimise this away).

The no-op if there is no OpenMaps component is a nice way of handling this. We should keep any eye on the mouseClicked method to make sure it doesn't get too large (possibility for re-factoring later, but it's great for now!).

Other than that the Open maps code looks fine to me! Great work!

Many thanks,
Blair



On 15 July 2014 19:57, Felix Natter [via Freeplane Developer] <[hidden email]> wrote:
"BArchibald [via Freeplane Developer]"
<[hidden email]> writes:

> Hi Felix,

hi Blair,

> I'll have a look at this tonight and get back to you asap! Is it ready for testing? (if so I'll try it as
> a patch else I'll just read through it)

Yes, it works for me :-)

Thanks and Best Regards,
--
Felix Natter



If you reply to this email, your message will be added to the discussion below:
http://freeplane-developer.996965.n3.nabble.com/OpenMaps-improvements-please-review-tp470p473.html
To start a new topic under Freeplane Developer, email [hidden email]
To unsubscribe from Freeplane Developer, click here.
NAML

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: OpenMaps improvements: please review

Felix Natter
Administrator
"BArchibald [via Freeplane Developer]"
<[hidden email]> writes:

> Hi Felix,

hello Blair,

> I've imported the code and it all seems to be working very nicely, fast and does what it says on the
> tin! (I use Xmonad as my window manager so the dialogue always opens in the top left of the screen, I
> guess this is a tiling window manager bug not in freeplane?)
>
> Code Review:
>
> In "isInIconRegion" I would defiantly change this line: "((ZoomableLabelUI)getUI()).getIconR(this);",
> it's confusing to look at. I realise this isn't part of the  patch but would be nice to tidy up while
> we are here. (just introduce another local variable for the UI, javac will optimise this away).

Thank you for the review + testing!

> The no-op if there is no OpenMaps component is a nice way of handling this. We should keep any eye on
> the mouseClicked method to make sure it doesn't get too large (possibility for re-factoring later, but
> it's great for now!).

In fact Dimitry suggested that we add a general listener for clicks on
icons and make links + openmaps implement it!

--> therefore I have created a branch "openmaps2" with the current
    changes and will talk to Dimitry about it on the weekend.
(of course we can discuss this here, too)

> Other than that the Open maps code looks fine to me! Great work!

Thanks :-)
--
Felix Natter
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: OpenMaps improvements: please review

Felix Natter
Administrator
In reply to this post by BArchibald
"BArchibald [via Freeplane Developer]"
<[hidden email]> writes:

> Hi Felix,

hello Blair,
hello other devs,

> I've imported the code and it all seems to be working very nicely, fast and does what it says on the
> tin! (I use Xmonad as my window manager so the dialogue always opens in the top left of the screen,
> I guess this is a tiling window manager bug not in freeplane?)
>
> Code Review:
>
> In "isInIconRegion" I would defiantly change this line: "((ZoomableLabelUI)getUI()).getIconR(this);
> ", it's confusing to look at. I realise this isn't part of the  patch but would be nice to tidy up
> while we are here. (just introduce another local variable for the UI, javac will optimise this
> away).
>
> The no-op if there is no OpenMaps component is a nice way of handling this. We should keep any eye
> on the mouseClicked method to make sure it doesn't get too large (possibility for re-factoring
> later, but it's great for now!).

Dimitry had the suggestion to add a generic mouse click listener for Icons
(on IconController), and use this to realize the OpenMaps view when
clicking on the globe/openmaps icon (this should make it easier to
realize something like this e.g. for new plugins or node-types).

See the last four commits here:
  https://github.com/freeplane/freeplane/commits/openmaps2
(Dimitry did most of the work, but I learned something :-))

--> Could you give the 'openmaps2' branch a bit of testing?
(see that a click on the global icon opens the openmaps dialog)

--> If you agree then will integrate this into 1.4.x.

Thanks and Best Regards,
--
Felix Natter
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Fwd: OpenMaps improvements: please review

BArchibald
Hi Felix

On 01/09/14, Felix Natter wrote:

> --> Could you give the 'openmaps2' branch a bit of testing?
> (see that a click on the global icon opens the openmaps dialog)

Tested this today: Built correctly and the icon clicking seems to work as
expected.

I also had a look at the patches, it all seems reasonable to me (with
limited knowledge of the inner workings) - definitely nicer to have this
generic interface to work with.

One thing I did notice is that the commits seems to have some trailing
whitespace issues, mainly with stray tabs on blank lines. (vim seems to
think so anyway).

>
> --> If you agree then will integrate this into 1.4.x.
>

I think this is ready for integration otherwise.

Many thanks,
Blair

P.s Hopefully this email sends correctly, I've recently changed email
client and not sure it's completely set up correctly.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Fwd: OpenMaps improvements: please review

Felix Natter
Administrator
"BArchibald [via Freeplane Developer]"
<[hidden email]> writes:

> Hi Felix

hi Blair,

> On 01/09/14, Felix Natter wrote:
>
>> --> Could you give the 'openmaps2' branch a bit of testing?
>> (see that a click on the global icon opens the openmaps dialog)
>
> Tested this today: Built correctly and the icon clicking seems to work as
> expected.
>
> I also had a look at the patches, it all seems reasonable to me (with
> limited knowledge of the inner workings) - definitely nicer to have this
> generic interface to work with.
>
> One thing I did notice is that the commits seems to have some trailing
> whitespace issues, mainly with stray tabs on blank lines. (vim seems to
> think so anyway).
>
>>
>> --> If you agree then will integrate this into 1.4.x.
>>
>
> I think this is ready for integration otherwise.

thanks for the quick reply and testing, I will integrate this into 1.4.x
tomorrow!

> Many thanks,
> Blair
>
> P.s Hopefully this email sends correctly, I've recently changed email
> client and not sure it's completely set up correctly.

Looks good here :-)

Best Regards,
--
Felix Natter
Loading...