-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Tags editor #6758
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
base: master
Are you sure you want to change the base?
Tags editor #6758
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,217 @@ | ||
| (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 === -1) { | ||
| throw new Error(OSM.i18n.t("javascripts.object_tags_editor.errors.equals_not_found", { line: i + 1 })); | ||
| } | ||
| const k = line.substring(0, eqPos).trim(); | ||
| if (k === "") { | ||
| throw new Error(OSM.i18n.t("javascripts.object_tags_editor.errors.empty_key", { line: i + 1 })); | ||
| } | ||
| const v = line.substring(eqPos + 1).trim(); | ||
| if (v === "") { | ||
| throw new Error(OSM.i18n.t("javascripts.object_tags_editor.errors.empty_value", { line: i + 1 })); | ||
| } | ||
| tags.set(k, v.replaceAll("\\\\", "\n")); | ||
| } | ||
| return tags; | ||
| } | ||
|
|
||
| function validateChanges(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)) { | ||
| if (!k.match(/^[0-9a-zA-Z:_]+$/)) { | ||
| throw new Error(OSM.i18n.t("javascripts.object_tags_editor.errors.added_invalid_key", { key: k })); | ||
| } | ||
| addedTags.set(k, v); | ||
| } | ||
| }); | ||
| return addedTags.size + changedTags.size + removedTags.size !== 0; | ||
| } | ||
|
|
||
| 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(OSM.i18n.t("javascripts.object_tags_editor.errors.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(OSM.i18n.t("javascripts.object_tags_editor.errors.upload_failed", { status: 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 $commentInput = $("<input>") | ||
| .prop({ | ||
| type: "text", | ||
| placeholder: OSM.i18n.t("javascripts.object_tags_editor.changeset_comment_placeholder") | ||
| }) | ||
| .addClass("form-control mb-2"); | ||
|
|
||
| const $editorTextarea = $("<textarea>") | ||
| .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()); | ||
| if (!validateChanges(currentTags, newTags)) { | ||
| throw new Error(OSM.i18n.t("javascripts.object_tags_editor.errors.no_changes")); | ||
| } | ||
| const changesetId = await openOsmChangeset($commentInput.val().trim()); | ||
| 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.message ?? String(e)); | ||
| } | ||
| }); | ||
|
|
||
| const $saveButton = $("<input>") | ||
| .prop({ | ||
| type: "submit", | ||
| name: "save", | ||
| value: OSM.i18n.t("javascripts.object_tags_editor.save"), | ||
| disabled: true | ||
| }) | ||
| .addClass("btn btn-primary"); | ||
|
|
||
| $commentInput.on("input", () => $saveButton.prop("disabled", $commentInput.val().trim() === "")); | ||
|
|
||
| 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, $commentInput, $errorBox, $buttonsWrapper); | ||
| $browseSection.replaceWith($editorWrapper); | ||
| $editorTextarea.focus(); | ||
| }); | ||
| }()); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 %> | ||
| · | ||
| <%= tag.a t("browse.edit_object_tags"), :href => "", :class => "edit_object_tags p-0 border-0 bg-transparent shadow-none" %> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If we look at keys specifically, we can implement strict validation and prohibit spaces in keys. Or even prohibit adding anything but
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? %> | ||
| · | ||
| <%= link_to t("browse.view_unredacted_history"), :controller => "old_#{@type.pluralize}", :params => { :show_redactions => true } %> | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
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:
well, so far there is no CodeMirror even in the iD :)
There was a problem hiding this comment.
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: #1254.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we do offer a simple text area (and I suspect we shouldn't) then it definitely shouldn't be the default.