Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve display names of route relations #75

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Improve display names of route relations #75

wants to merge 3 commits into from

Conversation

matthijskooijman
Copy link

This makes two changes:

  1. Include the type of a route in its display name, to easily distinguish e.g. hiking routes from cycling routes in the UI.
  2. For objects (not necessarily limited to routes) without a name or ref, but with a from and/or to attribute, use "from-to" in the display name.

The initial one seems like an clear improvement. The second one might need a bit more consideration (i.e. it adds a bit of a special case).

These changes were made when working with hiking routes in the Swiss hiking route network, but should be generally usable as well.

@matthijskooijman
Copy link
Author

Is there any interest in merging this change? I'm in Switzerland again, and running into this issue again (routes that are nicely tagged with from and to, but then no longer easy to identify in josm). I'm resorting to adding note tags, but that's tagging for renderers, which isn't the best solution...

@stoecker
Copy link

stoecker commented Aug 1, 2023

This is only a MIRROR! Report bugs/enhancements in https://josm.openstreetmap.de/!

@tsmock
Copy link

tsmock commented Aug 1, 2023

To add on to what stoecker said, once you open a ticket on https://josm.openstreetmap.de, add a link from that ticket to this PR and add [PATCH] to the ticket title on the JOSM website. You should also either modify the title of this PR to have the ticket number in it or link back to the ticket from first post. This makes it easier to check and see if this PR has been merged, and will let someone close it if it got forgotten.

Please read https://josm.openstreetmap.de/wiki/DevelopersGuide/PatchGuide -- please note that I consider GitHub PR links to be good enough (add .patch to the URL), but other maintainers may not.

I also haven't checked your patch -- have you run ant pmd checkstyle and fixed the warnings?

Something that I will ask if you do not register on the ticket system (we do allow for "anonymous" tickets) is how you want the patch attributed, since we cannot merge patches from GitHub into SVN; this means that most patches from external patches have a (patch by <foo>) addendum to the first line of the commit message.

One of the reasons why we like to have bugs/feature requests filed on our system is just in case an external system decides to make life difficult for us; by having the tickets and (hopefully) most of the conversation on our own systems, we can refer back to that even if the external system is closed down.

I also tend to be a bit more verbose in the commit messages/code comments since those are much less likely to be accidentally destroyed, and they tend to go with all the mirrors of the code, but that won't cover all the conversation/why something was decided. Hopefully no one is cursing my name in 10 years for poorly documenting why I did something. :)

tsmock
tsmock requested changes Aug 1, 2023
View reviewed changes
src/org/openstreetmap/josm/data/osm/DefaultNameFormatter.java Outdated Show resolved Hide resolved
src/org/openstreetmap/josm/data/osm/DefaultNameFormatter.java
String from = trcLazy("from", I18n.escape(relation.get("from")));
String to = trcLazy("to", I18n.escape(relation.get("to")));
if (from != null || to != null)
return (from != null ? from : "?") + "-" + (to != null ? to : "?");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend it to return -<TO> when from is null and <FROM>- when to is null?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This intends (and I think implements) to return ?-<TO> when from is null and <FROM>-? when to is null.

@matthijskooijman
Copy link
Author

First off, thanks for taking your time to respond!

To add on to what stoecker said, once you open a ticket on https://josm.openstreetmap.de, add a link from that ticket to this PR and add [PATCH] to the ticket title on the JOSM website. You should also either modify the title of this PR to have the ticket number in it or link back to the ticket from first post. This makes it easier to check and see if this PR has been merged, and will let someone close it if it got forgotten.

Seem I did that for an earlier PR, not sure why I did not do this here. Will do that now.

I also haven't checked your patch -- have you run ant pmd checkstyle and fixed the warnings?

I'm not sure, will do so now.

Something that I will ask if you do not register on the ticket system

I registered an account, probably easier than filling in my details every time :-)

One of the reasons why we like to have bugs/feature requests filed on our system is just in case an external system decides to make life difficult for us; by having the tickets and (hopefully) most of the conversation on our own systems, we can refer back to that even if the external system is closed down.

Makes sense, thanks for explaining.

I also tend to be a bit more verbose in the commit messages/code comments since those are much less likely to be accidentally destroyed, and they tend to go with all the mirrors of the code, but that won't cover all the conversation/why something was decided. Hopefully no one is cursing my name in 10 years for poorly documenting why I did something. :)

Not sure if you looked at my commit messages yet, but I have the same tendency (this is also why I prefer a PR to patch files - easier to split changes into different commits and specify commit messages).

@matthijskooijman
Copy link
Author

I also haven't checked your patch -- have you run ant pmd checkstyle and fixed the warnings?

I went ahead and documented this at https://josm.openstreetmap.de/wiki/DevelopersGuide/PatchGuide?action=diff&version=12 (since I think pmd was not mentioned at all, and checkstyle only elsewhere).

@matthijskooijman
show route type in route relation display name
90da9fe 
This displays e.g. `hiking route ("foo-bar")` instead of just `route
("foo-bar")`, making it easier to distinguish different route relations.
@matthijskooijman
add from-to as an option for display names
db91cb8 
Some routes (in particular node networks) have no proper name of their
own, but do have a from and/or to attribute set that gives at least some
description of the route.

Note that these routes often do have the `name` or `note` attribute set
using an explicit "from-to" value, but these attributes are not really
meant for that, so for example the Swiss hiking route tagging now
recommends settings only `from` and `to`, not `name` or `note`.

See e.g. https://wiki.openstreetmap.org/wiki/DE:Switzerland/HikingNetwork#Wanderwegenetz

As an example of such a route, see https://www.openstreetmap.org/relation/13126425

For these routes, setting just `from` or just `to` is also possible (in
particular for node network routes that start at an unnamed
node/guidepost). In this case, the missing attribute is replaced by `?`.

This commit modifies the `relation.nameOrder` preference, adding one
extra special value `from-to` which is handled specially.
@matthijskooijman
Copy link
Author

@tsmock I've made the changes you suggested, and also fixed a small formatting bug (missing space) in a new commit. The code now passes ant checkstyle pmd test.

When making a ticket on trac about this, I noticed an existing related ticket: https://josm.openstreetmap.de/ticket/4596#comment:9 Discussion there (from a few years ago) suggested the use of presets with name templates instead of modifying the code, which I investigated and seems a reasonable alternative to the changes in this PR, but with some significant limitations as well. See the ticket for further details, probably good to continue discussion there before further considering the code in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsmock tsmock tsmock requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matthijskooijman @stoecker @tsmock