Re: [mkgmap-dev] Preparing patches (Re: POIs for areas)
Marko Mäkelä schrieb am 03.03.2011 22:54: Could you elaborate what your patch does? Does it remove the option add-pois-to-areas altogether? Even though an add_poi action would replace add-pois-to-areas, I think that we should support both forms for some time. Sadly this patch disables the add-pois-to-areas option completely. I also would like to have both capabilities for backward compatibiliy, but in the style processing the command line arguments are not visible at the moment. So there is no easy way for testing either whether the addpoi command is set in the style or whether the command line argument is set. The trunk version generates in the style processing the basis for the POI generation for ALL objects regardless the command line argument. During the map generation the command line argument is test, for whether the available POIs are generated or not. In my patch during the style creation only the basis for the POI generation of the objects with the addpoi command is created. And during the map generation a POI is ALWAYS created, if for an object the corresponding basis is found. You may ignore the rest of this message. I did a too quick read of the patch and thought that you had changed accidentally white space in some lines. You did that on purpose, because you removed one level of indentation when removing the check for the add-pois-to-areas parameter. I have read the rest of your message, but probably only understood half of it. (I am neither familiar with eclipse nor with svn, so I hardly now what I am doing when building my own mkgmap.) What is the desired approach on the indentation when a patch has such a structure change like removing an if-bracketing? In my change I also corrected the indentation of otherwise unchanged lines. Shouldn't such changes be included in the patch? If they are not included, how is the indentation then corrected? Gruss Torsten ___ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Re: [mkgmap-dev] Preparing patches (Re: POIs for areas)
On Fri, Mar 4, 2011 at 8:48 AM, Torsten Leistikow de_m...@gmx.de wrote: What is the desired approach on the indentation when a patch has such a structure change like removing an if-bracketing? Changing indentation when removing a nesting level is fine, but it should be confined to the area where the nesting was removed. In my change I also corrected the indentation of otherwise unchanged lines. Shouldn't such changes be included in the patch? If they are not included, how is the indentation then corrected? Whitespace-only changes should be included in a separate patch that does not change any functionality. That makes it easier to review changes. -- Jeff Ollie ___ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Re: [mkgmap-dev] Preparing patches (Re: POIs for areas)
Jeffrey Ollie schrieb am 04.03.2011 16:09: Whitespace-only changes should be included in a separate patch that does not change any functionality. That makes it easier to review changes. Thanks for the clarification. Gruss Torsten ___ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Re: [mkgmap-dev] Preparing patches (Re: POIs for areas)
Moin,
my mkgmap build process is very shaky, but finally I managed to create a patch
of my changes. Perhaps it is even working?
Gruss
Torsten
Index: src/uk/me/parabola/mkgmap/osmstyle/TypeReader.java
===
--- src/uk/me/parabola/mkgmap/osmstyle/TypeReader.java (Revision 1878)
+++ src/uk/me/parabola/mkgmap/osmstyle/TypeReader.java (Arbeitskopie)
@@ -69,6 +69,8 @@
gt.propagateActions(true);
} else if (w.equals(no_propagate)) {
gt.propagateActions(false);
+ } else if (w.equals(add_poi)) {
+gt.setAddPoi(true);
} else if (w.equals(oneway)) {
// reserved
} else if (w.equals(access)) {
Index: src/uk/me/parabola/mkgmap/osmstyle/StyledConverter.java
===
--- src/uk/me/parabola/mkgmap/osmstyle/StyledConverter.java (Revision 1878)
+++ src/uk/me/parabola/mkgmap/osmstyle/StyledConverter.java (Arbeitskopie)
@@ -541,12 +541,14 @@
clipper.clipShape(shape, collector);
- nodeRules.resolveType(way, new TypeResult() {
- public void add(Element el, GType type) {
-if(type != null)
- shape.setPoiType(type.getType());
- }
- });
+ if (gt.isAddPoi()) {
+ nodeRules.resolveType(way, new TypeResult() {
+public void add(Element el, GType type) {
+ if(type != null)
+ shape.setPoiType(type.getType());
+}
+ });
+ }
}
private void addPoint(Node node, GType gt) {
Index: src/uk/me/parabola/mkgmap/main/MapMaker.java
===
--- src/uk/me/parabola/mkgmap/main/MapMaker.java (Revision 1878)
+++ src/uk/me/parabola/mkgmap/main/MapMaker.java (Arbeitskopie)
@@ -151,47 +151,39 @@
}
private void makeAreaPOIs(CommandArgs args, LoadableMapDataSource src) {
- String s = args.get(add-pois-to-areas, null);
- if (s != null) {
- MapPointFastFindMap poiMap = new MapPointFastFindMap();
+ MapPointFastFindMap poiMap = new MapPointFastFindMap();
- for (MapPoint point : src.getPoints())
- {
-if(!point.isRoadNamePOI()) // Don't put road pois in this list
- poiMap.put(null, point);
- }
+ for (MapPoint point : src.getPoints())
+ {
+ if(!point.isRoadNamePOI()) // Don't put road pois in this list
+poiMap.put(null, point);
+ }
- for (MapShape shape : src.getShapes()) {
-String shapeName = shape.getName();
+ for (MapShape shape : src.getShapes()) {
+ String shapeName = shape.getName();
-int pointType = shape.getPoiType();
+ int pointType = shape.getPoiType();
-// only make a point if the shape has a name and we know what type of point to make
-if (pointType == 0)
- continue;
+ // only make a point if the shape has a name and we know what type of point to make
+ if (pointType == 0)
+continue;
-
-// We don't want to add unnamed cities !!
-if(MapPoint.isCityType(pointType) shapeName == null)
- continue;
-
-// check if there is not already a poi in that shape
+ // check if there is not already a poi in that shape
-if(poiMap.findPointInShape(shape, pointType, shapeName) == null) {
- MapPoint newPoint = new MapPoint();
+ if(poiMap.findPointInShape(shape, pointType, shapeName) == null) {
+MapPoint newPoint = new MapPoint();
- newPoint.setName(shapeName);
- newPoint.setType(pointType);
+newPoint.setName(shapeName);
+newPoint.setType(pointType);
- copyAddressInformation(shape, newPoint);
+copyAddressInformation(shape, newPoint);
- newPoint.setLocation(shape.getLocation()); // TODO use centroid
+newPoint.setLocation(shape.getLocation()); // TODO use centroid
- src.getPoints().add(newPoint);
+src.getPoints().add(newPoint);
- log.info(created POI , shapeName, from shape);
-}
+log.info(created POI , shapeName, from shape);
}
}
Index: src/uk/me/parabola/mkgmap/reader/osm/GType.java
===
--- src/uk/me/parabola/mkgmap/reader/osm/GType.java (Revision 1878)
+++ src/uk/me/parabola/mkgmap/reader/osm/GType.java (Arbeitskopie)
@@ -59,6 +59,9 @@
// actions will always be executed
private boolean propogateActionsOnContinue;
+ // only applicable for ways (roads, lines and shapes)
+ private boolean addPoi = false;
+
public GType(int featureKind, String type) {
this.featureKind = featureKind;
try {
@@ -198,4 +201,12 @@
public void setContinueSearch(boolean continueSearch) {
this.continueSearch = continueSearch;
}
+
+ public void setAddPoi(boolean addPoi) {
+ this.addPoi = addPoi;
+ }
+
+ public boolean isAddPoi() {
+ return addPoi;
+ }
}
___
mkgmap-dev mailing list
mkgmap-dev@lists.mkgmap.org.uk
http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Re: [mkgmap-dev] Preparing patches (Re: POIs for areas)
On Thu, Mar 03, 2011 at 04:59:37PM +0100, Torsten Leistikow wrote: my mkgmap build process is very shaky, but finally I managed to create a patch of my changes. Perhaps it is even working? Could you elaborate what your patch does? Does it remove the option add-pois-to-areas altogether? Even though an add_poi action would replace add-pois-to-areas, I think that we should support both forms for some time. You may ignore the rest of this message. I did a too quick read of the patch and thought that you had changed accidentally white space in some lines. You did that on purpose, because you removed one level of indentation when removing the check for the add-pois-to-areas parameter. Usually, I would do something like this: svn diff add_poi.patch emacs add_poi.patch and then fix any mistakes in the patch, either manually or by reverting unintended changes with C-u C-c C-a (diff-apply-hunk with a prefix argument). When reviewing a patch, you could save some work by telling svn diff to ignore the white space changes: svn diff x -wpu add_poi-w.patch Old versions of svn do not accept the -x option, unless you also specify --diff-cmd: svn diff --diff-cmd diff -x -wpu add_poi-w.patch On a related note, the patch -l option is useful when there are white space differences between the patch and the source file, such as when TABs have been converted to spaces in the patch or in the source file. Marko ___ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
[mkgmap-dev] Preparing patches (Re: POIs for areas)
On Tue, Mar 01, 2011 at 06:32:23PM +0100, Torsten Leistikow wrote: I removed my change-markings from the files and attached the cleaned source files. (I still haven't figured out how to provide a proper patch) If you have checked out the source tree with 'svn checkout', then you can just do 'svn diff' to create a patch. The output of that command will also show the svn revision against which the patch was prepared (but sadly not the full path to the repository). See also http://svnbook.red-bean.com/. Marko ___ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
