Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/assets/javascripts/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//= require_self
//= require numbered_pagination
//= require object_tags_editor
//= require leaflet.sidebar
//= require leaflet.sidebar-pane
//= require leaflet.locate
Expand Down
278 changes: 278 additions & 0 deletions app/assets/javascripts/object_tags_editor.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,278 @@
(function () {
function parseTags(text) {
const lines = text.split("\n");
const tags = new Map();
for (let i = 0; i < lines.length; i++) {
const line = lines[i].trim();
if (line === "") {
continue;
}
const eqPos = line.indexOf("=");
if (eqPos <= 0 || eqPos === line.length - 1) {
throw new Error(`= not found on ${i + 1} line`);
}
const k = line.substring(0, eqPos).trim();
if (k === "") {
throw new Error(`empty key on ${i + 1} line`);
}
const v = line.substring(eqPos + 1).trim();
if (v === "") {
throw new Error(`empty value on ${i + 1} line`);
}
tags.set(k, v.replaceAll("\\\\", "\n"));
}
return tags;
}

function makeChangesSummary(objectType, objectId, prevTags, newTags) {
const addedTags = new Map();
const changedTags = new Map();
const removedTags = new Map();
prevTags.entries().forEach(([k, v]) => {
if (!newTags.has(k)) {
removedTags.set(k, v);
} else if (newTags.get(k) !== v) {
changedTags.set(k, v);
}
});
newTags.entries().forEach(([k, v]) => {
if (!prevTags.has(k)) {
addedTags.set(k, v);
}
});
if (addedTags.size + changedTags.size + removedTags.size === 0) {
return;
}

function makeDetailedPrefix() {
let res = "";
if (addedTags.size) {
res += "Add " + addedTags.entries().map(([k, v]) => `${k}=${v}`).toArray().join(", ") + "; ";
}
if (changedTags.size) {
res += "Changed " + changedTags.entries().map(([k, v]) => `${k}=${v}\u200b→\u200b${newTags.get(k)}`).toArray().join(", ") + "; ";
}
if (removedTags.size) {
res += "Removed " + removedTags.entries().map(([k, v]) => `${k}=${v}`).toArray().join(", ");
}
return res;
}

function makeOnlyKeysPrefix() {
let text = "";
if (addedTags.size) {
text += "Add " + addedTags.map(([k]) => k).join(", ") + "; ";
}
if (changedTags.size) {
text += "Changed " + changedTags.map(([k]) => k).join(", ") + "; ";
}
if (removedTags.size) {
text += "Removed " + removedTags.map(([k]) => k).join(", ");
}
return text;
}

function makeUniversalSummary() {
return `Update tags of ${objectType}/${objectId}`;
}

function makeMainTagsHint() {
const mainTags = new Set([
"shop", "building", "amenity", "man_made", "highway", "natural",
"aeroway", "historic", "railway", "tourism", "landuse", "leisure"
Comment thread
deevroman marked this conversation as resolved.
Outdated
]);
let mainTagsHint = "";

for (const [k, v] of prevTags.entries()) {
if (mainTags.has(k) && !removedTags.has(k) && !changedTags.has(k)) {
mainTagsHint += ` ${k}=${v}`;
break;
}
}
for (const [k, v] of prevTags.entries()) {
if (k === "name" && !removedTags.has("name") && !changedTags.has("name")) {
mainTagsHint += ` ${k}=${v}`;
break;
}
}
return mainTagsHint;
}

let summary = makeDetailedPrefix();
const mainTagsHint = makeMainTagsHint();
if (summary.length > 200 || changedTags.size > 1) {
summary = makeOnlyKeysPrefix();
}
summary = summary.replace(/; $/, "");
if (mainTagsHint === "") {
summary += ` for ${objectType}/${objectId}`;
} else if (removedTags.size) {
summary += " from" + mainTagsHint;
} else if (changedTags.size) {
summary += " of" + mainTagsHint;
} else if (addedTags.size) {
summary += " to" + mainTagsHint;
}
Comment thread
deevroman marked this conversation as resolved.
Outdated
return summary && summary.length < 255 ? summary : makeUniversalSummary();
}

function makeAuthHeaders() {
return { Authorization: `Bearer ${document.head.dataset.oauthToken}` };
}

async function openOsmChangeset(comment) {
const changesetPayload = document.implementation.createDocument(null, "osm");
const changesetElem = changesetPayload.createElement("changeset");
changesetPayload.documentElement.appendChild(changesetElem);

Object.entries({
created_by: "Tags editor on osm.org",
comment: comment
}).forEach(([k, v]) => {
const tag = changesetPayload.createElement("tag");
tag.setAttribute("k", k);
tag.setAttribute("v", v);
changesetElem.appendChild(tag);
});

const res = await fetch("/api/0.6/changeset/create", {
method: "PUT",
headers: makeAuthHeaders(),
body: new XMLSerializer().serializeToString(changesetPayload)
});
if (!res.ok) {
throw new Error("Failed changeset creation");
}
return await res.text();
}

async function uploadNewVersion(objectType, objectId, objectInfo, changesetId) {
try {
objectInfo.children[0].children[0].setAttribute("changeset", changesetId);
const objectInfoStr = new XMLSerializer().serializeToString(objectInfo).replace(/xmlns="[^"]+"/, "");
const res = await fetch(`/api/0.6/${objectType}/${objectId}`, {
method: "PUT",
headers: makeAuthHeaders(),
body: objectInfoStr
});
if (!res.ok) {
throw new Error(`Error when uploading changes: HTTP ${res.status}`);
}
} finally {
await fetch(`/api/0.6/changeset/${changesetId}/close`, {
method: "PUT",
headers: makeAuthHeaders()
});
}
}

function applyNewTags(objectInfo, newTags) {
const objectXML = objectInfo.querySelector("node,way,relation");
objectXML.querySelectorAll("tag").forEach(i => i.remove());
newTags.entries().forEach(([k, v]) => {
const tag = objectInfo.createElement("tag");
tag.setAttribute("k", k);
tag.setAttribute("v", v);
objectXML.appendChild(tag);
});
}

async function downloadObjectInfo(type, id) {
const res = await fetch(`/api/0.6/${type}/${id}.xml`);
const objectInfo = new DOMParser().parseFromString(await res.text(), "text/xml");
if (objectInfo.querySelector("parsererror")) {
throw new Error("invalid API response");
}
return objectInfo;
}

function extractTagsFromObjectInfo(objectInfo) {
const tags = new Map();
objectInfo.querySelectorAll("tag").forEach(t => {
tags.set(t.getAttribute("k"), t.getAttribute("v"));
});
return tags;
}

$(document).on("click", "a.edit_object_tags", async function (e) {
e.preventDefault();
e.stopPropagation();

e.target.setAttribute("disabled", true);

const [, type, id] = location.pathname.match(/\/(node|way|relation)\/([0-9]+)/);
const objectInfo = await downloadObjectInfo(type, id);
const currentTags = extractTagsFromObjectInfo(objectInfo);

const $browseSection = $("#sidebar_content h2 + div").first();

const $errorBox = $("<p>");

const $editorTextarea = $("<textarea>")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The big question: should this be a simple text area or something more structured? If it’s a simple text area that depends on a format of some sort, we need to provide hard-to-miss access to documentation about the format. It isn’t quite Level0L, it’s the multiline key-value syntax that iD exposes in its text view.

Most of the mappers you know already know the syntax by heart or will it figure on their own, but this PR would put “Edit Tags” in front of a broader user base, including users who’ve never edited the map before. Maybe this text area could be a steppingstone to something more structured like iD’s default raw tags view, a table of editable fields. After all, unlike Level0 (#2391), this inline text editor doesn’t really need to serve a mail-merge-like use case where you copy-paste the result of some text processing on the command line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A text field definitely has advantages over something like a table (it's not for nothing that iD has two editing modes)

We might need something like a Help button with a tooltip

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it's not for nothing that iD has two editing modes

Right, the main motivation was to enable mappers to copy-paste tags between elements: openstreetmap/iD#6302. A JOSM-like UI for that would’ve been inconvenient because the sidebar doesn’t have a custom context menu. As a side benefit, the single text area makes it easier to copy-paste tag combinations from the wiki or nsi.guide.

I wouldn’t say the single text area is a showstopper, but it would be pretty explicitly an advanced thing that we wouldn’t want newer users to think of as the main tool for editing tags. It’s essentially a code editor (which I guess implies a future enhancement for syntax highlighting).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe the motivation was really like that, but in my practice there are still a few advantages:

  1. A wider field. There are no additional buttons, and the keys column does not take up a fixed width
  2. Works arrow navigation on the keyboard

future enhancement for syntax highlighting

well, so far there is no CodeMirror even in the iD :)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A wider field. There are no additional buttons, and the keys column does not take up a fixed width

Yeah, the sidebar width can be constraining since it isn’t resizable: #1254.

Works arrow navigation on the keyboard

Personally I am worried that, the third time I use this feature, I’m going to press the adjacent and buttons on my laptop keyboard and eject a tag by accident. But I probably should be more worried about what’ll happen when I use this editor on my phone, since you mentioned that as a motivating factor. MediaWiki presents mobile users with an extra mandatory preview due to all the possible accidents that can occur, like accidentally tapping on the Save button while trying to select text. Maybe that confirmation screen isn’t such a bad idea after all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the sidebar width can be constraining since it isn’t resizable

In fact, it's high time we increased the width of the sidebar. And you can make it dynamic with a single line in CSS. But this is a separate discussion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The big question: should this be a simple text area or something more structured? If it’s a simple text area that depends on a format of some sort, we need to provide hard-to-miss access to documentation about the format. It isn’t quite Level0L, it’s the multiline key-value syntax that iD exposes in its text view.

Even if we do offer a simple text area (and I suspect we shouldn't) then it definitely shouldn't be the default.

.addClass("form-control font-monospace mb-3")
.prop({
rows: 10,
cols: 40
});

$editorTextarea.val(
currentTags
.entries()
.map(([k, v]) => `${k} = ${v.replaceAll("\n", "\\\\")}`)
.toArray()
.join("\n")
);

async function submitTags() {
const newTags = parseTags($editorTextarea.val());
const summary = makeChangesSummary(type, id, currentTags, newTags);
if (!summary) {
throw OSM.i18n.t("javascripts.object_tags_editor.no_changes");
}
const changesetId = await openOsmChangeset(summary);
applyNewTags(objectInfo, newTags);
await uploadNewVersion(type, id, objectInfo, changesetId);
}

const $editorWrapper = $("<form>")
.on("submit", async (e) => {
e.preventDefault();
$errorBox.text("");
try {
await submitTags();
location.reload();
} catch (e) {
$errorBox.text(e);
}
});

const $saveButton = $("<input>")
.prop({
type: "submit",
name: "save",
value: OSM.i18n.t("javascripts.object_tags_editor.save")
})
.addClass("btn btn-primary");

const $cancelButton = $("<input>")
.prop({
type: "submit",
name: "cancel",
value: OSM.i18n.t("javascripts.object_tags_editor.cancel")
})
.addClass("btn btn-danger")
.on("click", () => {
e.target.removeAttribute("disabled");
$editorWrapper.replaceWith($browseSection);
});

const $buttonsWrapper = $("<div>")
.addClass("d-flex gap-2 mt-3");

$buttonsWrapper.append($saveButton, $cancelButton);

$editorWrapper.append($editorTextarea, $errorBox, $buttonsWrapper);
$browseSection.replaceWith($editorWrapper);
$editorTextarea.focus();
});
}());
4 changes: 4 additions & 0 deletions app/views/elements/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
<% if @feature.visible? %>
<div class='secondary-actions mb-3'>
<%= link_to(t("browse.download_xml"), :controller => "api/#{@type.pluralize}", :action => :show) %>
<% if current_user %>
&middot;
<%= tag.a t("browse.edit_object_tags"), :href => "", :class => "edit_object_tags p-0 border-0 bg-transparent shadow-none" %>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From my perspective as an iD developer, I’d expect some volume of well-meaning but misguided contributions from people who don’t speak English or OSM English, as well as even messier contributions from SEOs. Think whole sentences as keys, that sort of thing.

If we’re concerned about this possibility, we could make inline editing an option alongside the Preferred Editor preference. It would still be more discoverable than the current browser extension, but just high enough of a hurdle that we’d have a better idea of who’s using the feature initially.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Think whole sentences as keys, that sort of thing.

If we look at keys specifically, we can implement strict validation and prohibit spaces in keys. Or even prohibit adding anything but 0-9a-zA-Z:_ to a new keys.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moreover, it is not difficult to implement autocomplete and validation based on Taginfo, but we can think about this later.

<% end %>
<% if current_user&.moderator? %>
&middot;
<%= link_to t("browse.view_unredacted_history"), :controller => "old_#{@type.pluralize}", :params => { :show_redactions => true } %>
Expand Down
5 changes: 5 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ en:
one: "%{count} way"
other: "%{count} ways"
download_xml: "Download XML"
edit_object_tags: "Edit Tags"
view_unredacted_history: "View Unredacted History"
location: "Location:"
common_details:
Expand Down Expand Up @@ -3730,6 +3731,10 @@ en:
id_not_configured:
title: "iD has not been configured"
body: "Please see CONFIGURE.md for more information"
object_tags_editor:
save: "Save"
cancel: "Cancel"
no_changes: "No changes"
redactions:
edit:
heading: "Edit Redaction"
Expand Down