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

Support linking to object versions #4468

Closed
wants to merge 1 commit into from

Conversation

Cj-Malone
Copy link

Hey, this is my first time with Ruby, please be gentle. Also I've not tested this as I don't have a local dev environment set up, but I've looked at the erb docs, and it seems right.

My goal is to able to link to versions, eg https://www.openstreetmap.org/node/1/history#v1. I have no strong opinion on the anchor prefix, I've used v but it could be version- or anything.

@tomhughes
Copy link
Member

Can you give an example of why you want to do that? Knowing what the end goal is can help with determining the best solution...

@pnorman
Copy link
Contributor

pnorman commented Jan 10, 2024

#662 covers linking to object versions and suggests something like https://www.openstreetmap.org/relation/1812962/5

This is different than linking to a specific version on the overall history page.

@tomhughes
Copy link
Member

Yes that was the alternative approach I had in mind, to provide a view of a specific version, which has performance advantages for objects with large histories.

@AntonKhorev
Copy link
Contributor

If we ever add paging to element history, using #v1 won't work.

@Cj-Malone
Copy link
Author

Can you give an example of why you want to do that? Knowing what the end goal is can help with determining the best solution...

I'm struggling to answer this. I want to be able to share a link to a specific version of an object. Links are URLs, URLs are like URIs but I'm less interested in URIs, however this does kind of give OSM objects an id per version. I've seen people storing similar in databases.

The current system is Checkout version 1 of [this](https://www.openstreetmap.org/node/1/history) which isn't ideal. I guess there is also https://www.openstreetmap.org/node/1/history#:~:text=Version%20#1, but I use Firefox.

#662

Happy to close in favour of that, if we expect it to happen any time soon.

Performance

I've not made the links visible anywhere, and people are already linking to the history page. I don't think this will do anything to the performance.

@pnorman
Copy link
Contributor

pnorman commented Jan 11, 2024

If we don't make the links visible there's not much point, as they're not discoverable. A single object version URL is far more usable because for large objects, it will show only that version. History pages for large relations with lots of versions have significant usability issues and minor performance issues.

@Cj-Malone
Copy link
Author

There is no link to https://www.openstreetmap.org/traces#trace_list, yet it works.

@tomhughes
Copy link
Member

Yes, it's an internal implementation detail (the id is there for styling) that you are not supposed to use for external linking

@Cj-Malone
Copy link
Author

Sorry, my mistake. Did you know you can use class for that?

How about https://www.openstreetmap.org/about#open-data, is that exposed or used for styling?

@AntonKhorev
Copy link
Contributor

How about https://www.openstreetmap.org/about#open-data, is that exposed or used for styling?

You can always find ids that are not used for styling and try to link to them. Something like this requires ids:

<label for="input_id" ... > ... </label>
<input id="input_id" ... >

However there are some ids that are used for linking, despite not having visible links. One of them is changeset comment ids c123456. That's similar to what's being added here. Therefore it's potentially going to have the same problems if pagination is added to changeset discussion: a changeset page is loaded, but it doesn't have the comment you're trying to link to because it's on another page. Actually this is fixable with some javascript, but you'll have to convince the maintainers to add that javascript.

But all of those problems are less likely for changeset comments because changesets very rarely have a ton of comments. Relations with hundreds of versions however exist and so are requests to add pagination for them like #3524.

Here's a pull request adding visible links to changeset comments: #4134.

@Cj-Malone
Copy link
Author

I've gone to great lengths to maintain my own URLs with basically 0 users, I know it's hard and you have to think about it constantly. I can't imagine how hard that is with the pressure of the amount of users on this site.

Actually this is fixable with some javascript, but you'll have to convince the maintainers to add that javascript

There is already JS parsing the anchor, so if anything happens with #662 and the maintainers want to maintain urls, they will have to change that JS. But IMO that should happen with #662, not #4468.

@AntonKhorev
Copy link
Contributor

There is already JS parsing the anchor, so if anything happens with #662 and the maintainers want to maintain urls

I'm not talking about #662 affecting this pull request. Of course if element versions get their own routes, you may want to redirect to those routes from javascript. Or you may want to retain the links inside the full history page.

I'm talking about the situation if the full history page won't exist anymore, that is it won't show all of the versions. That can happen without #662.

@HolgerJeromin
Copy link
Contributor

However there are some ids that are used for linking, despite not having visible links.

Are we really discussing here to refactor the code to remove all id in DOM?
Only to prevent someone constructing URLs?

@AntonKhorev
Copy link
Contributor

Are we really discussing here to refactor the code to remove all id in DOM?

No.

Only to prevent someone constructing URLs?

No, that's impossible.

@pnorman
Copy link
Contributor

pnorman commented Jan 15, 2024

IDs are used for both CSS/JS and to make public-facing URLs from. The proposed addition would be neither - they won't be used for styling, and with no way to discover them, they won't be public facing.

If a user uses an ID that isn't linked to by viewing source and finding the ID they're linking to something there's no commitment to support. In practice, IDs change infrequently, but we shouldn't be developing for this use-case.

@gravitystorm
Copy link
Collaborator

If a user uses an ID that isn't linked to by viewing source and finding the ID they're linking to something there's no commitment to support.

I think this is worth emphasising. If we link to a fragment ourselves, then I would consider that a public link that we have to support (and I would hope it's covered by a test, so that we don't break user bookmarks etc by mistake). If other fragment-links happen to work due to implementation details of the site (e.g. ids on elements that are there to be used by css/javascript) then those won't be maintained when we refactor our code.

@Cj-Malone
Copy link
Author

Fixed by #4480. Thank you @AntonKhorev

@Cj-Malone Cj-Malone closed this Jan 24, 2024
@Cj-Malone Cj-Malone deleted the links branch January 24, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants