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

fix JOSM remote control so url= is regular http(s) URL #5439

Open
wants to merge 1 commit into
base: version-3.x
Choose a base branch
from

Conversation

mnalis
Copy link

@mnalis mnalis commented Dec 3, 2022

Otherwise, as imageryUrl is (as required) in format like tms[20]:https://tms.osm-hr.org/knin-2007/{zoom}/{x}/{-y}.png, that value would be sent as url parameter to JOSM remote control imagery load command, which would thus fail to load imagery (in current testing JOSM version 18583 at least)

This PR fixes it so url= parameter to remote control is regular http(s) URL like https://tms.osm-hr.org/knin-2007/{zoom}/{x}/{-y}.png, as it should be.

and not "tms[20]:http://..." which fails to load in JOSM
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ramyaragupathy ramyaragupathy removed the request for review from prabinoid October 15, 2024 14:59
@ramyaragupathy
Copy link
Member

hey @mnalis - can you please share more details on your Tasking Manager instance? Looks like this PR is specific for v3.

@mnalis
Copy link
Author

mnalis commented Nov 13, 2024

Yes, that PR was made for v3, as that was what we were using (and where we had a problem) in 2022.

That specific instance (https://tasks.osm-hr.org) had since been upgraded to v4 (for other reasons, i.e. Oauth2) which (on a cursory glance) does not seem to have that problem (although I haven't done extensive testing).

@ramyaragupathy
Copy link
Member

@mnalis - do you know of other v3 instances that we could use to test this behaviour? If none available, let's verify the behaviour in v4 and close it here

cc @royallsilwallz @manjitapandey

@mnalis
Copy link
Author

mnalis commented Nov 20, 2024

do you know of other v3 instances that we could use to test this behaviour? If none available

As v3 only supports Oauth1, which is no longer supported by openstreetmap.org, if there are any running TM3 instances left, you wouldn't be able to log in into them any more (unless someone forked the TM3 and backported Oauth2 support into it, of course)

@ramyaragupathy
Copy link
Member

@mnalis - merging requires testing with V3, but v3 would not support login because of oauth compatibility issue. So only way to test could be output the url somewhere and cross verify in a browser? Not sure - but a full-fledged workflow through OSM login is not possible, I believe.

two things to test here:

  1. make sure the url is constructed right
  2. constructed url fetches some imagery

If we can verify the first step, I think we should be good to merge.

cc @royallsilwallz @manjitapandey

@mnalis
Copy link
Author

mnalis commented Jan 22, 2025

@ramyaragupathy I'm appreciative of your support, but I'm not sure if using your energy on this PR is warranted any more since 1 July 2024 - when TM V3 was rendered inoperative.

The fact that external imagery also doesn't work (which this PR addressed at the time) is of lesser importance now that V3 doesn't work at all. Also, V3 seems not be updated anymore anyway (last commit seems to have been almost 5 years ago), so it seems somewhat unlikely that someone will implement Oauth2 for it and new V3 release made; and unless it happens, any other improvements (such as this one) on V3 branch are mostly pointless, as nobody would be able to use them.

So, in my humble opinion - all PRs/issues that relate to V3-only (like this one) and are not applicable to V4, should probably best closed as outdated. (but if you do see some use for V3, I'd be delighted to hear!)

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.

2 participants