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

Map viewer / Remove Stamen background layers - no longer available #7715

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

josegar74
Copy link
Member

Stamen layers are no longer available, have been replaced by StadiaMaps.

New versions of OpenLayers support StadiaMaps: https://openlayers.org/en/latest/examples/stamen.html, but the version used in GeoNetwork doesn't have support for it.

For the time being, this change request removes Stamen layers.

Checklist

  • I have read the contribution guidelines
  • Pull request provided for main branch, backports managed with label
  • Good housekeeping of code, cleaning up comments, tests, and documentation
  • Clean commit history broken into understandable chucks, avoiding big commits with hundreds of files, cautious of reformatting and whitespace changes
  • Clean commit messages, longer verbose messages are encouraged
  • API Changes are identified in commit messages
  • Testing provided for features or enhancements using automatic tests
  • User documentation provided for new features or enhancements in manual
  • Build documentation provided for development instructions in README.md files
  • Library management using pom.xml dependency management. Update build documentation with intended library use and library tutorials or documentation

@jodygarnett
Copy link
Contributor

How urgent is this? Are maps broken presently?

Copy link
Contributor

@jodygarnett jodygarnett left a comment

Choose a reason for hiding this comment

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

This pull-request is successful in its stated aim of removing stamen. I understand that stadiamaps is not yet available to the project.

If I am reading the code correctly configurations previously created will now display an empty background?

Suggestion: Ideally we could offer a placeholder for stamen until we were in a position to use strradiamaps. This way existing configurations would continue to do something until we upgrade. As an example we could display OSM in grayscale to provide something for context.

@@ -177,7 +177,6 @@ This section is for configuring the map shown when viewing a record.
- **wmts**: generic WMTS layer, required properties: `name, url`.
- **tms**: generic TMS layer, required property: `url`.
- **osm**: OpenStreetMap default layer, no other property required.
- **stamen**: Stamen layers, required property: `name`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be adding stadiamaps here at the same time we remove stamen ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The version of OpenLayers used in GeoNetwork doesn't support StadiaMaps. We can upgrade OpenLayers, there is a pull request on-going #7421, but not sure about the status and if it's even possible with the GeoNetwork code to do such upgrade.

return new ol.layer.Tile({
source: source,
title: title || "Stamen"
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This switch/case does not appear to have a default...

If we are removing, rather than offering stadiamaps alternative we should default to osm or something that will display rather than being empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

OSM is the default AFAIK

Copy link
Contributor

@juanluisrp juanluisrp left a comment

Choose a reason for hiding this comment

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

LGTM.

@fxprunayre fxprunayre merged commit 176570e into geonetwork:main Feb 9, 2024
9 checks passed
@geonetworkbuild
Copy link
Collaborator

The backport to 3.12.x failed:

The process '/usr/bin/git' failed with exit code 1
stderr
error: could not apply 4343e99e59... Map viewer / Remove Stamen background layers - no longer available
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

stdout
Auto-merging docs/manual/docs/help/map/index.md
Auto-merging web-ui/src/main/resources/catalog/components/common/map/mapService.js
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/components/common/map/mapService.js
Auto-merging web-ui/src/main/resources/catalog/components/viewer/owscontext/OwsContextService.js
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/components/viewer/owscontext/OwsContextService.js
Auto-merging web-ui/src/main/resources/catalog/locales/ca-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/ca-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/cs-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/cs-admin.json
CONFLICT (modify/delete): web-ui/src/main/resources/catalog/locales/da-admin.json deleted in HEAD and modified in 4343e99e59 (Map viewer / Remove Stamen background layers - no longer available).  Version 4343e99e59 (Map viewer / Remove Stamen background layers - no longer available) of web-ui/src/main/resources/catalog/locales/da-admin.json left in tree.
Auto-merging web-ui/src/main/resources/catalog/locales/de-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/de-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/en-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/es-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/es-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/fi-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/fi-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/fr-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/fr-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/is-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/is-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/it-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/it-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/ko-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/ko-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/nl-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/nl-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/pt-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/pt-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/ru-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/ru-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/sk-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/sk-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/sv-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/sv-admin.json
Auto-merging web-ui/src/main/resources/catalog/locales/zh-admin.json
CONFLICT (content): Merge conflict in web-ui/src/main/resources/catalog/locales/zh-admin.json

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-3.12.x 3.12.x
# Navigate to the new working tree
cd .worktrees/backport-3.12.x
# Create a new branch
git switch --create backport-7715-to-3.12.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 4343e99e59f4ca7282cdb6b0963e3243162d435f
# Push it to GitHub
git push --set-upstream origin backport-7715-to-3.12.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-3.12.x

Then, create a pull request where the base branch is 3.12.x and the compare/head branch is backport-7715-to-3.12.x.

@jahow
Copy link
Contributor

jahow commented Feb 9, 2024

It may have been wise to at least show something (e.g. a StadiaMaps layer) instead of leaving the map empty. And StadiaMaps are simple XYZ layers so a new version of OL is not really needed to show them; OL 8 only gives a convenience class to use it.

@josegar74
Copy link
Member Author

@jahow for now only OSM is available with this change. It can be done another PR to add the StadiaMaps using XYZ layers.

@jahow
Copy link
Contributor

jahow commented Feb 9, 2024

@jahow for now only OSM is available with this change. It can be done another PR to add the StadiaMaps using XYZ layers.

I mean if someone loads a map context with a stamen layer in it, it won't show up right?

@josegar74
Copy link
Member Author

I haven't try that, but I don't think it will be processed the stamen layer.

@jodygarnett
Copy link
Contributor

That is what I was trying to ask in my feedback.

@josegar74
Copy link
Member Author

@jodygarnett not sure if I understand your last comment. The default map context file in GeoNetwork has been updated to offer only OSM, to avoid listing also the Stamen layers that doesn't work.

If a user loads a map context file with Stamen layer, or other type of layer not supported in GeoNetwork, we don't offer any fallback, the code AFAIK just ignores that type of layer. If we want to offer this kind of fallback, we need to modify the code that loads the map context files in GeoNetwork, but I'm afraid that is out of the scope of this change request.

@jodygarnett
Copy link
Contributor

I understand it is out of scope; we will offer update instructions.

Or is fixing this part of a migrating process? Because users make and save these context files in the catalogue…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants