#23179 closed enhancement (fixed)
[PATCH] Include changeset in note comment if feasible
Reported by: | qeef | Owned by: | team |
---|---|---|---|
Priority: | normal | Milestone: | 23.11 |
Component: | Core | Version: | |
Keywords: | Cc: | qeef |
Description
When closing a note, check for the changeset in the changeset cache. If
there is a changeset with the comment that contains a link to the note
being closed, put the link to the changeset as the default text of the
closing note comment.
Attachments (6)
Change History (19)
by , 14 months ago
Attachment: | 0001-Include-changeset-in-note-comment-if-feasible.patch added |
---|
comment:1 by , 14 months ago
comment:2 by , 14 months ago
Feedback on the patch:
- We typically use camelCase for variables instead of underscores. So
note_url_short
->noteUrlShort
. - I think the
note_url_short
andnote_url_long
variables can be in theif
statement scope - We should probably be looking for
"osm.org/note/"
and"openstreetmap.org/note/"
instead of the full URL. In addition, we should probably be looking for the current OSM API endpoint, maybe special casing it for the shortosm.org
(use theOsmApi
class andJosmUrls.getDefaultOsmApiUrl
method) - Add
@since xxx
to the newshowNoteDialog
method javadocs
comment:3 by , 14 months ago
Thank you for the review and sorry for the recurring issues with snake_case variables.
In addition, we should probably be looking for the current OSM API endpoint, maybe special casing it for the short osm.org (use the OsmApi class and JosmUrls.getDefaultOsmApiUrl method)
I am sorry, but I am missing why. This patch downloads nothing from OSM, it just checks just uploaded changesets for a link to the note being closed (if I understand ChangesetCache correctly).
by , 14 months ago
Attachment: | v2-0001-Include-changeset-in-note-comment-if-feasible.patch added |
---|
comment:4 by , 14 months ago
JOSM isn't used solely on OSM.org.
For example, if someone fixes a note on OpenHistoricalMap, this code should still work if they reference the note in their changeset comment.
Example:
A changeset with the comment "I fixed https://openhistoricalmap.org/note/1" should autofill the closing comment with the changeset id.
by , 14 months ago
Attachment: | v3-0001-Include-changeset-in-note-comment-if-feasible.patch added |
---|
comment:5 by , 14 months ago
JOSM isn't used solely on OSM.org.
I was thinking about that. Please, check the v3. Thank you!
comment:6 by , 14 months ago
No worries -- it is something that we tend to try and think about. The one exception is validations, presets, and paintstyles, which are all configurable.
Anyway, why not
- if (cs.getComment().indexOf(noteUrlShort) > -1 || cs.getComment().indexOf(noteUrlLong) > -1) { + if ((Config.getUrls().getDefaultOsmApiUrl().equals(OsmApi.getOsmApi().getServerUrl()) && cs.getComment().indexOf(noteUrlShort) > -1) || cs.getComment().indexOf(noteUrlLong) > -1) {
If you've got a reason, go for it.
by , 14 months ago
Attachment: | v4-0001-Include-changeset-in-note-comment-if-feasible.patch added |
---|
comment:7 by , 14 months ago
I am sorry, it looks like I missed your point. Patch v4 applies default comment for the note to be closed only for the default OSM instance, i.e., osm.org or openstreetmap.org.
comment:8 by , 14 months ago
Milestone: | → 23.09 |
---|
LGTM. I'll pull it down and test it (and hopefully write some unit tests for it, but UI stuff can be a bit of a PITA to write tests for).
Side node: do you want me to use qeef
when attributing your patch (e.g. Fix #23179: Include changeset in note comment if feasible (patch by qeef)
)?
by , 13 months ago
Attachment: | 0002-Closing-note-comment-based-on-the-OSM-instance-URL.patch added |
---|
by , 13 months ago
Attachment: | v2-0002-Closing-note-comment-based-on-the-OSM-instance-UR.patch added |
---|
comment:10 by , 13 months ago
I have yet uploaded follow-up patch v2-0002-...
with the generalization to other instances if you think it's good idea. Thank you.
This change tends to improve the navigation between related changeset and note, originally raised here: https://lists.sr.ht/~qeef/damn-project/%3Ccce27ab08172b1a61983902570dd15c1%40gusl.org%3E