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
19 stars 1 forks source link

BCE dates are misinterpreted as CE dates #746

Closed 1ec5 closed 6 months ago

1ec5 commented 7 months ago

As reported on the forum, the Pyramid of Khufu is reported as being built in 2570 CE, even though its start_date=-2570 should mean 2571 BCE, as discussed in OpenHistoricalMap/issues#624.

The code currently strips out the negative sign but displays the year verbatim after that. Instead, it should rely on Intl.DateTimeFormat to format any date, respecting the locale and correctly interpreting negative year values.

https://github.com/OpenHistoricalMap/ohm-inspector/blob/901cd7e0f7063b3abb677e6a38cb66c9841fbc9d/openhistoricalmap-inspector.js#L270

gregallensworth commented 7 months ago

Looks like the intent here was to do three things to the date:

But you're right, that instead of tacking on "BCE" I'll go with Intl.DateTimeFormat to get the year + epoch, and display the epoch only if either year was negative (since "1937 AD - 1982 AD" would be unusual).

1ec5 commented 7 months ago

Intl.DateTimeFormat.formatRange() can take care of deduplicating elements of the date range output. iD manipulates this method to implement open-ended date ranges and only show AD for a date range spanning both eras.

gregallensworth commented 7 months ago

Playing around with a few cases of dates spanning BC and AD, or some that are all-BC or all-AD, there are cases where I think it best to include the era even when they're the same. The dates are just displayed with a hyphen, so it may not be clear they are even dates if you're not sure what you're looking at.

For example, 23 - 143 is a date but if we don't add the era it can be confusing at first glance.

image

I propose this:

I have this programmed up too, could test various combinations and send back screenshots, etc. if you like.

1ec5 commented 7 months ago

That’s fair, iD’s use case is slightly different – appending the date range to every feature label in view, such that the user would be able to intuit the purpose more easily than if it only appears in one place on screen.

By the way, I’d suggest using the built-in formatter regardless. I was surprised to see the degree of variation in punctuation and choice of omitted symbols across languages. You’d still have some stylistic control over whether the era is shown, but the improved localization would be pretty important. (For example, no amount of era names would help a Japanese or Chinese speaker recognize it as a date range if it lacks the 年 after the year number.)

But regardless, anything would be an improvement over the current time-bending behavior. 😅

gregallensworth commented 7 months ago

Thanks for the feedback. In that case, the built-in formatter sounds like the better way to go even if we lose some formatting control. I'll do that.

It occurs to me too, that the words "Starting" and "Until" are programmed into the control, hardcoded Engllsh text. That's less than ideal. Do you think it would make sense to omit these, and display JUST the date range without such preamble?

1ec5 commented 7 months ago

Yes, omitting one of the dates to form an open range is a bit of a hack, but it holds up across languages as far as I know so far.

By the way, I’m half-expecting that eventually we’ll move this functionality out of the JavaScript inspector code and into the Rails code proper in order to label elements by date range in Nominatim results, relation member lists, etc.: #431. We’d have an opportunity to surround the dates in brackets, which seems to be a common convention (now followed by iD as well).

gregallensworth commented 7 months ago

Here we go... Using purely Intl.DateTimeFormat.formatRange() and Intl.DateTimeFormat.format() here are some outputs. I have them (here on my local-dev) with brackets as well, and without the hardcoded English text.

Some example dates and ranges:

What do you think of that?

A Second Change: Moving the sources

Now, there is one other change relevant to this being just generated by Intl.DateTimeFormat

Previously, there was a fair bit of fussing to add the source for the start date and for the end date, as hyperlinks with "[Source:]" hardcoded onto it. For example if one had both sources this could read:

1872 [Source: Encyclopedia of 19th Century America] - 1968 [Source: Seattle Gazette article about the fire]

These fields are:

Now that we want the date string to be automatically generated, I suggest moving these two start/end links when they exist, into the same section below as all the other links.

image

What do you think of this second item?

1ec5 commented 7 months ago

Previously, there was a fair bit of fussing to add the source for the start date and for the end date, as hyperlinks with "[Source:]" hardcoded onto it.

Oh interesting, I hadn’t thought of this, but it would be perfect for the same formatRangeToParts() method that iD is using.

I would probably lean toward making the date range more prominent rather than less, since we (or at least I) would like mappers to stop stuffing dates inside name, duplicating the date tags.

gregallensworth commented 7 months ago

Previously, there was a fair bit of fussing to add the source for the start date and for the end date, as hyperlinks with "[Source:]" hardcoded onto it.

Oh interesting, I hadn’t thought of this, but it would be perfect for the same formatRangeToParts() method that iD is using. I would probably lean toward making the date range more prominent rather than less, since we (or at least I) would like mappers to stop stuffing dates inside name, duplicating the date tags.

That doesn't make sense to me. You were very specific to use formatRange() because it knows best, instead of second-guessing the format, by grabbing parts and then concatenating them into a string (a string seems fine to Americans and some Europeans, and not to many others).

If that really is a goal, then it seems wiser to move the source links down with the other sources as I indicated.

image

This has other benefits:

1ec5 commented 7 months ago

My suggestion was only about how to implement the nested source links while respecting localization. formatRangeToParts() returns the same thing as formatRange(), but with each part annotated for both human and machine readability. However, I agree that splicing the source links into the date range would be much less intuitive.

We can move it down to the source row for now, but once we implement #431, the dates would likely be attached to the name in labels throughout the interface, just like in iD. At that point, I don’t know if the inspector ought to suppress the date range from the label or duplicate the date range, but we can cross that bridge when we get there.

gregallensworth commented 7 months ago

All righty https://github.com/OpenHistoricalMap/ohm-inspector/pull/51 will change the date readout (singular or range) to Intl.DateTimeFormat for:

I can merge this whenever is desired, which would effectively deploy it to staging, live, etc. I think https://github.com/OpenHistoricalMap/issues/issues/626 and https://github.com/OpenHistoricalMap/issues/issues/624 are related, and are also on hold as they depend on a server-side change to the PL/PgSQL decimaldate.

danrademacher commented 6 months ago

This PGSQL date function change has been merged and is now live in the data on staging.openhistoricalmap.org.

Gregor, if you could go ahead and merge the relevant PRs that depended on that, then we can deploy all to staging, make sure its working, tehn move to prod and close out!

gregallensworth commented 6 months ago

I have merged https://github.com/OpenHistoricalMap/leaflet-ohm-timeslider-v2/pull/11

I see at https://staging.openhistoricalmap.org/ that dates are now represented "one off from intuition" as should be expected. The date range January 1 300 BC through December 31 200 BC, is now represented as date=-299-01-01&daterange=-299-01-01,-199-12-31

danrademacher commented 6 months ago

Hmm, but one specific issue with the Inspector that is highlighted here is not addressed: https://staging.openhistoricalmap.org/way/198560373#map=19/29.97917/31.13423&layers=O&date=1924-05-24&daterange=1824-01-01,2024-12-31

The pyramid still reads as starting in 2570 instead of -2570 or 2570 BC image

gregallensworth commented 6 months ago

I see that https://github.com/OpenHistoricalMap/ohm-inspector/pull/51 was not yet merged. This PR was relevant to using the locale to display the date, which was also the fix for the "-" part effectively being dropped.

gregallensworth commented 6 months ago

There we go:

image