search

OpenHistoricalMap / issues

File your issues here, regardless of repo until we get all our repos squared away; we don't want to miss anything.
Creative Commons Zero v1.0 Universal
18 stars 1 forks source link

Update iD to latest version #395

Closed danrademacher closed 1 year ago

danrademacher commented 2 years ago

We got ourselves all in a dither over in #388 that we needed to upgrade iD urgently when actually we had just collectively broken the dist artifacts and also the system that replaces the dist artifacts.

Now that that's repaired, we can be more deliberate about an iD upstream catchup.

Pulling from that other issue's long comment thread:

Looking at updates we will need to redo:

So basically we have:

Note that iD updates are no longer a blocker on upstream merge of osm-website

jeffreyameyer commented 2 years ago

Aha... I'd just add this for testing:

danrademacher commented 2 years ago

Getting back to setting this up for GIN team to work on.

Based on https://github.com/OpenHistoricalMap/issues/issues/198#issuecomment-1109115005, our approach for merging upstream OSM, I tried the following in a branch freshly split from staging:

git merge -Xours upstream/release

Seems like almost all the merge conflicts were related to files removed from upstream, because of the new presets repo. Also conflicts in the /dist branch but we should rebuild that anyway, and no more Travis for CI.

I did the following to plow through those

git rm -r data/presets/*
git rm .travis.yml
git rm data/locales/* -r
git rm dist/* -r -f

The outcome of that was an iD that couldn't do npm run all and failed at the build data step because it was still looking for data/presets/*, which it shouldn't be since that's no longer in the upstream release.

So instead since our iD changes were pretty small and localized, I tried this:

`git merge -Xtheirs upstream/release`

I still had to do this to fix merge conflicts on a ton of deleted files:

git rm -r data/presets/*
git rm .travis.yml
git rm data/locales/* -r
git rm dist/* -r -f

Now npm run all gets through build data but fails with errors like this:

 iD@2.21.1 build:js
> node config/esbuild.config.mjs

✘ [ERROR] No matching export in "node_modules/d3-selection/src/index.js" for import "event"

    modules/ui/sections/background_offset.js:2:4:
      2 │     event as d3_event,
        ╵     ~~~~~

I checked out a separate copy of https://github.com/openstreetmap/iD/tree/release and I could see that files that we never modified still differ from the upstream branch and cause these errors... I manually replaced a couple and those resolved specific issues, but others arose. This road would be whack-a-mole with no confidence that we really resolved every possible unintended difference.

Pretty clear that this upstream catchup is a bit painful at the moment. I am sure there is a better way. And we'll work on finding it. What we really want is to start over with current iD release, and then reapply the few changes in https://github.com/OpenHistoricalMap/iD/compare/558ee7c8585921e3cc37662c76018b126ce2c6fe...05ebef1cd0faba63bc1f7b381f59c99b1f9c358e to the new code.

1ec5 commented 2 years ago

I agree, starting over would probably be more efficient and reliable at this point. There have been so many major changes to the upstream iD since the last merge in OpenHistoricalMap/iD#18 but relatively few downstream changes. Some changes like OpenHistoricalMap/iD@338d66c63a64aa76d6ef566f77bda2f646c9e3bd would likely be obviated by OpenHistoricalMap/ohm-website#106, and OpenHistoricalMap/iD#138 replaces the date field validation implementation with one that would probably merge more easily.

Starting over could look like renaming the current staging branch, creating a new staging branch from upstream’s develop branch, then cherry-picking over anything that seems important. To make the process go more smoothly, we could temporarily ignore generated files in the dist/ directory. Alternatively, we could interactively rebase staging atop develop, squashing some commits along the way.

Either way, we’ll need to be careful about OHM-specific changes to preset files. Upstream, these files got moved to id-tagging-schema but have also been restructured quite a bit. A good first step would be to get OpenHistoricalMap/id-tagging-schema all caught up and manually reapply any changes from OpenHistoricalMap/iD. OpenHistoricalMap/ohm-editor-layer-index also needs to be caught up, but not as much has happened in the upstream ELI.

danrademacher commented 2 years ago

thanks @1ec5! I went ahead with the first option you suggest:

Now we have: https://github.com/OpenHistoricalMap/iD/tree/staging and https://github.com/OpenHistoricalMap/iD/tree/staging-old

Once we get our needed OHM mods into staging we'll make that the new default for the repo.

Other notes

A couple of changes were easier to just redo manually vs cherry picking old commits:

See core.yml for some things that are not straightforward

    view_on_osmose: View on osmose.openstreetmap.fr
    view_on_keepRight: View on keepright.at
    help_link_url: "https://wiki.openstreetmap.org/wiki/FAQ#I_have_just_made_some_changes_to_the_map._How_do_I_get_to_see_my_changes.3F"
      upload: "Before uploading your changes you must enter a [changeset comment](https://wiki.openstreetmap.org/wiki/Good_changeset_comments). Then press **{upload}** to send your changes to OpenHistoricalMap, where they will be merged into the map and publicly visible to everyone."
      tools: "The following tools are currently supported: [KeepRight](https://www.keepright.at/), [ImproveOSM](https://improveosm.org/en/) and [Osmose](https://osmose.openstreetmap.fr/)."

Lots of other ImproveOSM mentions and probably other things I missed that we should strip out of our iD, but for now just focused on a working thing with our date presets, filters, and recent PRs

Will come back to this for the real cherry picking to get our date filters and the improved date validations in palce.

I still have to wrap my head around the id-taggins-schema and how changes there end up in iD -- I assume we need to tie our forks together or the tagging repo has some dist output that needs to end up in our iD...

1ec5 commented 2 years ago

I still have to wrap my head around the id-taggins-schema and how changes there end up in iD -- I assume we need to tie our forks together or the tagging repo has some dist output that needs to end up in our iD...

The way it works in OSMland is a bit more complicated:

  1. id-tagging-schema is distributed as an NPM package.
  2. iD’s GitHub Actions configuration has a workflow that pushes id-tagging-schema strings to iD’s project on Transifex for translation.[^ci]
  3. iD’s release script downloads translations from Transifex rather manually. (I’m unsure why it doesn’t just use the tx tool like in step 2.) It also fetches preset icons from both the NPM CDN and the latest main branch on GitHub.
  4. At runtime, iD fetches presets and some tag-related validator data and more translations from the NPM CDN.

Fully extricating these processes from OSM would require addressing the localization problem for iD. If we don’t address steps 2 and 3, perhaps as part of #268, some UI strings could show up as cryptic error messages.

Similar to #446, I wonder if the overhead of maintaining a fork of id-tagging-schema is worth the trouble. It’s difficult to tell exactly what OHM changed regarding presets independently of OpenHistoricalMap/iD@4cd90bd8c17373641a73682d3d1bdc4cd9debdd4 and OpenHistoricalMap/iD#18 because they were file merges instead of proper Git merges. The closest I can get is from the following links, which exclude OpenHistoricalMap/iD@4cd90bd8c17373641a73682d3d1bdc4cd9debdd4 and OpenHistoricalMap/iD@e2afc96b34e6f989f4e06ed1b911a2970521d8c5:

As far as I can tell, the only intentional differences are:

We could probably accomplish these changes using an override file or some special cases in iD proper but otherwise using OSM’s id-tagging-schema. A better reason to fork id-tagging-schema would be if we intend to add a large number of presets for natural or artificial features that are no longer mappable in today’s world. From what’s currently documented on the wiki, there doesn’t seem to be much OHM-specific tagging development yet.

[^ci]: iD switched from Travis to GitHub Actions in openstreetmap/iD#8258, after the last merge. Currently, no continuous integration is running on the fork. We should decide on a CI provider and set that up at some point.

jeffreyameyer commented 2 years ago

From what’s currently documented on the wiki, there doesn’t seem to be much OHM-specific tagging development yet.

This may be correct, but I think there's a fair amount of end-user experimentation that may not have percolated to the wiki page or more widespread usage yet.

danrademacher commented 2 years ago

Closing the loop on the last comments in this issue about forking, or not forking, id-tagging-schema, if we can ditch our fork of that and instead manage with some local-to-iD overrides, that would be excellent. We've got enough forks to worry about already!

As for the core task here on iD, next steps:

Once all that's done, then next would be:

  • Making the Start Date, End Date, Source, and License fields universal
  • Giving the Start Date and End Date fields more specific placeholders
  • Simplifying the Source field into a plain text field, removing the dropdown suggestions
1ec5 commented 2 years ago

I rebased OpenHistoricalMap/iD#137 and OpenHistoricalMap/iD#138 onto the staging branch. They no longer have a dependency on the date filter or preset customizations we did on staging-old.

jeffreyameyer commented 2 years ago

nice work, team!

On Fri, Sep 30, 2022 at 8:27 AM Minh Nguyễn @.***> wrote:

I rebased OpenHistoricalMap/iD#137 https://github.com/OpenHistoricalMap/iD/pull/137 and OpenHistoricalMap/iD#138 https://github.com/OpenHistoricalMap/iD/pull/138 onto the staging branch. They no longer have a dependency on the date filter or preset customizations we did on staging-old.

— Reply to this email directly, view it on GitHub https://github.com/OpenHistoricalMap/issues/issues/395#issuecomment-1263716934, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALM4EV4IR3TNT2EGOY4EJ3WA4BEZANCNFSM52D6FHDQ . You are receiving this because you commented.Message ID: @.***>

-- Jeff Meyer 206-676-2347 osm: Open Historical Map (OHM) http://wiki.openstreetmap.org/wiki/Open_Historical_Map / my OSM user page http://www.openstreetmap.org/user/jeffmeyer t: @OpenHistMap

danrademacher commented 2 years ago

OK, @1ec5 PRs are merged into staging so now we are good to have @gregallensworth re-implement the Date filters:

  1. enlist @gregallensworth's help in either cherry-picking old commits or just reimplementing the Date Range filters on new staging branch that runs off current upstream iD code. The Date Range was merged in this commit on the old branch, https://github.com/OpenHistoricalMap/iD/commit/9ce4fcfc8121b95c0c6cc7feac1c9e8b1aa7d226
1ec5 commented 2 years ago

The two PRs had a merge conflict. OpenHistoricalMap/iD#141 fixes syntax errors that crept in during conflict resolution.

1ec5 commented 2 years ago

One of the changes we’re pulling into the staging branch is openstreetmap/iD#8258, which migrates from Travis to GitHub Actions. The configuration is in the repository now, but it would need to be enabled. Continuous integration would’ve caught the bad merge before #137 could be merged into staging. Does GitHub Actions sound like a good choice for OpenHistoricalMap’s continuous integration needs, or would you prefer a different service?

danrademacher commented 2 years ago

I'm a big fan of GitHub Actions! Nice to have it all in one place and it's well documented

gregallensworth commented 2 years ago

Work in progress, but progress.

The Hotel Seattle and the Occidental Hotel both visible due the the 20-year span, but the Sinking Ship not visible as it wasn't until 1865.

http://localhost:8080/#background=Bing&daterange=1860,1890&disable_features=boundaries&id=w198553375&map=18.08/47.60214/-122.33248

image

This still accepts only integer years and not full YYYY-MM-DD dates. In features.js there was a valueToInt() function which stripped all - characters in order to make dates into numbers. However, this approach gives wrong results when comparing dates of a different length (19530504 > 1983) and changes negative numbers (-4000 becomes 4000) In its place I put the original simple parseInt() which would leave -4000 intact and treat 1953-05-04 as integer 1953 so the year comparisons are still accurate. Adding decimaldate and some date-padding functions, will come next.

1ec5 commented 2 years ago

We’ve taken care of all the tasks in https://github.com/OpenHistoricalMap/issues/issues/395#issuecomment-1260386525 except:

  1. See about local override file to achieve what we did in the past by editing Presets, well summarized here:

https://github.com/OpenHistoricalMap/issues/issues/395#issuecomment-1255704276 explains the pipeline in detail, but we don’t need to hook into anything at that low level. The API documentation describes iD’s options for customizing the presets and presumably the fields within those presets.

danrademacher commented 1 year ago

So based on the documentation linked above we add something like this:

iD.fileFetcher.cache().presets.presets["aerialway/zipline"] = {
    geometry: ["line"],
    fields: ["incline"],
    tags: { "aerialway": "zip_line" },
    name: "Zipline"
};

Our old JSON preset had these:

  "start_date": {"key": "start_date", "type": "text", "universal": true, "label": "Start Date", "placeholder": "YYYY-MM-DD", "terms": ["inception"]},
  "end_date": {"key": "end_date", "type": "text", "universal": true, "label": "End Date", "placeholder": "YYYY-MM-DD"},

So I think we'd add something like this:

iD.fileFetcher.cache().presets.presets["start_date"] = {
    geometry: ["point", "vertex", "line", "area", "relation"],
    fields: ["start_date"],
    name: "Start Date",
    terms: ["inception"],
    universal: "true",
    placeholder: "YYYY-MM-DD"
};

and

iD.fileFetcher.cache().presets.presets["end_date"] = {
    geometry: ["point", "vertex", "line", "area", "relation"],
    fields: ["end_date"],
    name: "End Date",
    universal: "true",
    placeholder: "YYYY-MM-DD"
};

I'm not super clear where we put that code. I assume within our fork of iD somewhere, but not sure yet

1ec5 commented 1 year ago

If I’m not mistaken, we’d put the overrides right above this method call.

The Start Date and End Date fields are already universal, but all that means is they’ll show up in the More Fields menu regardless of the preset. To make these fields appear by default for all presets, we’d need to make sure every preset has them in its fields property. Unfortunately, the brute-force approach in OpenHistoricalMap/iD@12588352df2e3b3f990225a7bc3545f9309f545f breaks field inheritance among presets. I think a surgical fix is possible, but it’ll have to be at a lower level than this official API: https://github.com/OpenHistoricalMap/issues/issues/449#issuecomment-1277342777.

1ec5 commented 1 year ago

OpenHistoricalMap/iD#145 implements the universal OHM presets. Sorry if you were already working on it @danrademacher; I basically happened upon this approach while investigating #449.

1ec5 commented 1 year ago

With OpenHistoricalMap/iD#145, I think we’ve taken care of everything that was identified as a blocker for deployment in https://github.com/OpenHistoricalMap/issues/issues/395#issuecomment-1260386525. We should synchronize again with the latest v2.22.0 release upstream, in case there were any last-minute fixes for that release that we haven’t pulled in yet. Otherwise, I think we should be good to go.

danrademacher commented 1 year ago

This is done on staging and as soon as https://github.com/OpenHistoricalMap/ohm-deploy/pull/195 is merged in early January it will be done on procuditon