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.

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

More flexible format in 'Search Features' when searching by element id #7627

Closed
blackboxlogic opened this issue May 22, 2020 · 9 comments · Fixed by #7775
Closed

More flexible format in 'Search Features' when searching by element id #7627

blackboxlogic opened this issue May 22, 2020 · 9 comments · Fixed by #7775
Labels
good first issue Best for first-time contributors. No experience necessary! usability An issue with ease-of-use or design
Milestone

Comments

@blackboxlogic
Copy link
Contributor

blackboxlogic commented May 22, 2020

Currently, User can search for a specific feature with a 'Search Features' query like n1234. When I copy an element from JOSM (or other places) I get a string like node 1234. But when I search in iD for this string, I get nothing. When I search for 1234 it offers node 1234 as an option.

I suggest supporting search queries of the form node 1234.

After looking at the code...

var idMatch = q.match(/^([nwr])([0-9]+)$/);

it could use the regex
/^(node|way|relation|[nwr]) ?([0-9]+)$/i
and in four places immediately blow, replace idMatch[1] with idMatch[1][0]

But this would only support English. A step further would be to get the user's current language, and support the localized word for each element type.

@quincylvania quincylvania added the usability An issue with ease-of-use or design label May 22, 2020
@manfredbrandl
Copy link
Contributor

It would be great if we could allow more than one translated word for the regex, e.g. „Punkt“ and „Knoten“ for the german translation of node or maybe even „Weg“ and „Fläche“ for way.

@1ec5
Copy link
Collaborator

1ec5 commented May 22, 2020

But this would only support English. A step further would be to get the user's current language, and support the localized word for each element type.

I think this would be pretty straightforward. Ideally, iD would expose a translatable string for “node id” in Transifex. With the current locale’s translation in hand, iD could convert that into a regex dynamically.

It would be great if we could allow more than one translated word for the regex, e.g. „Punkt“ and „Knoten“ for the german translation of node or maybe even „Weg“ and „Fläche“ for way.

Or iD could expose two translatable lists of prefixes and suffixes (similar to the presets’ synonym lists), which would allow for variation. Hopefully no language needs both a prefix and suffix at the same time, though.

@boothym
Copy link
Contributor

boothym commented May 22, 2020

Would it be possible to update the regex to exclude spaces before or after element ids? As that would solve #7467.

@blackboxlogic
Copy link
Contributor Author

Ignore whitespace before, after or between:
/^\s*(node|way|relation|[nwr])\s*([0-9]+)\s*$/i

@blackboxlogic
Copy link
Contributor Author

Consider merging this issue with with #7282. Also, I had indicated I might make a pull request. I'm abandoning that for now.

@quincylvania quincylvania added the help wanted For intermediate contributors, requires investigation or knowledge of iD code label Jun 4, 2020
@quincylvania quincylvania added good first issue Best for first-time contributors. No experience necessary! and removed help wanted For intermediate contributors, requires investigation or knowledge of iD code labels Jun 19, 2020
@blackboxlogic
Copy link
Contributor Author

These are examples of positive matches:
n158867610
n 158867610
n/158867610
node158867610
node 158867610
node/158867610
https://www.openstreetmap.org/node/158867610
https://www.openstreetmap.org/node/158867610/history
https://www.openstreetmap.org/node/158867610/history#map=19/43.66140/-70.25415
And the same for ways and relations. By requiring tokens before the type and after the id I have minimized false positives.
This also resolves #7282

@blackboxlogic
Copy link
Contributor Author

blackboxlogic commented Aug 27, 2020

To record some unintended consequences of this change:
image
This seems like a common use-case, and I regret breaking it. Any thoughts on if it is worth addressing?

@kymckay
Copy link
Collaborator

kymckay commented Sep 15, 2020

I think we could just have locationMatch take priority over idMatch in

var idMatch = q.match(/(?:^|\W)(node|way|relation|[nwr])\W?0*([1-9]\d*)(?:\W|$)/i);
if (idMatch) {
var elemType = idMatch[1].charAt(0);
var elemId = idMatch[2];
result.push({
id: elemType + elemId,
geometry: elemType === 'n' ? 'point' : elemType === 'w' ? 'line' : 'relation',
type: elemType === 'n' ? t('inspector.node') : elemType === 'w' ? t('inspector.way') : t('inspector.relation'),
name: elemId
});
}
var locationMatch = sexagesimal.pair(q.toUpperCase()) || q.match(/^(-?\d+\.?\d*)\s+(-?\d+\.?\d*)$/);
if (locationMatch) {
var loc = [parseFloat(locationMatch[0]), parseFloat(locationMatch[1])];
result.push({
id: -1,
geometry: 'point',
type: t('inspector.location'),
name: dmsCoordinatePair([loc[1], loc[0]]),
location: loc
});
}

So check location first, don't bother checking id if location is matched. Thoughts?

@quincylvania
Copy link
Collaborator

So check location first, don't bother checking id if location is matched. Thoughts?

@SilentSpike Sounds fine, I'd welcome a PR for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Best for first-time contributors. No experience necessary! usability An issue with ease-of-use or design
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants