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

MSC3488: Extending events with location data #3488

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Nov 14, 2021

In which we define the m.location extensible event type, suitable for sharing static location data.

Rendered

Element Web implementation

matrix-org/matrix-react-sdk#7135

Now merged and live on https://develop.element.io behind the labs flag.

Matrix iOS SDK implementation

https://github.com/matrix-org/matrix-ios-sdk/tree/develop/MatrixSDK/LocationSharing

Element Android Implementation

https://github.com/vector-im/element-android/tree/develop/vector/src/main/java/im/vector/app/features/location

Preview: https://pr3488--matrix-org-previews.netlify.app

@ara4n ara4n added the proposal A matrix spec change proposal label Nov 14, 2021
@ara4n ara4n marked this pull request as draft November 14, 2021 20:54
ara4n added a commit to element-hq/element-web that referenced this pull request Dec 6, 2021
Adds static location share a la [MSC3488](matrix-org/matrix-spec-proposals#3488) behind a labs flag, supporting legacy `m.location` `msgtype` too.  Powered by matrix-org/matrix-react-sdk#7135.  Adds maplibre as a dependency.

To make this work, you have to add a valid `map_style_url` to your config.json.
@ara4n
Copy link
Member Author

ara4n commented Dec 6, 2021

The implementation of this has now landed in Element Develop and it seems to be working okay, and nobody has come up with any objections on the MSC. So I'm going to propose FCP merge so that we can start spitting out unprefixed events sooner than later.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Dec 6, 2021

This FCP proposal has been cancelled by #3488 (comment).

Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

  • Blocked on extensible events

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Dec 6, 2021
@deepbluev7
Copy link
Contributor

I don't think anyone came up with objections, because none looked at it, because it is still a draft?

@turt2live
Copy link
Member

Like polls and voice messages before it, this MSC doesn't feel as though it can make meaningful progress until extensible events is through its own FCP.

@mscbot concern Blocked on extensible events

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Dec 6, 2021
@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Dec 6, 2021
@ara4n ara4n marked this pull request as ready for review December 6, 2021 18:29
@ara4n
Copy link
Member Author

ara4n commented Dec 6, 2021

totally missed that I'd failed to unflag it as draft - apologies.

details of a calendar invite), which should be stored in their respective
extensible event types when available.

XXX: should description be localised?
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be resolved before we enter FCP.

FWIW, I don't think description needs to be localised. If we have some common descriptions that clients can display in the user's native language, maybe we can add an extra field that has a machine-readable identifier (e.g. m.destination).

hypothetical `m.asset` type) to give the object a type and ID.

If sharing time-sensitive data, one would add another subtype (e.g. a
hypothetical `m.ts` type) to spell out the exact time that the data in the
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear whether this MSC is trying to define m.ts. This line says it's a "hypothetical" type, but the "Unstable prefix" part seems to indicate that when this MSC lands, then m.ts will be an official type.

If it is trying to define m.ts, then it should indicate whether it is only intended for use in conjunction with m.location, or of not, how it would be interpreted when there is no m.location.

Alternatively, it might be clearer to just add a ts field under the m.location section, as in:

"m.location": {
  "uri": "geo:51.5008,0.1247;u=35",
  "description": "Our destination",
  "ts": 1234567890
},

Copy link
Member Author

Choose a reason for hiding this comment

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

It's trying to define an actual m.ts mixin that gives the timestamp that a given event describes (which is obviously totally unrelated to origin_server_ts). Such a field is not remotely specific to location data, but any kind of data.

Copy link
Member

Choose a reason for hiding this comment

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

If it's trying to define m.ts, then can the wording be changed so that it sounds like it's doing so? e.g. "... one would add another subtype, m.ts, to spell out the exact time ..."

Also, if we're defining a generic mixin here that could be used for other events, it opens up the question of whether just a single timestamp is sufficient, or whether we need to worry about things like allowing for a range of times, or specifying precision. I don't think it's a blocking concern, as we could always define a new mixin to handle those things, though it would kind of suck to have multiple time-related things.

Add `m.asset` tracked asset subtype and zoom levels.
Add section about tile server configuration through .well-known.
* Fix indentation.

* Clarify tile server .well-known configuration.
@turt2live
Copy link
Member

This is blocked on extensible events, and I don't think it can make meaningful progress without that. Removing from the SCT's "look at this now" pile:

@mscbot fcp cancel

@mscbot mscbot removed the proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. label Apr 8, 2022
```json5
{
"m.tile_server": {
"map_style_url": "https://www.example.com/style.json"
Copy link
Contributor

@Johennes Johennes Jul 8, 2022

Choose a reason for hiding this comment

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

We've found performance issues with using dynamic map tiles in the room timeline on Element Android and have resorted to static raster images images there. This is another URL endpoint though (https://api.maptiler.com/maps/basic/static/...). We should add a dedicated property for this:

"m.tile_server": { 
    "map_style_url": "...",
    "static_tile_base_url": "https://api.maptiler.com/maps/basic/static"

Clients should be advised to use the tile variant that works best for their specific conditions.

```json5
"m.location": {
"uri": "geo:51.5008,0.1247;u=35",
"description": "Our destination",
Copy link
Contributor

@HarHarLinks HarHarLinks May 2, 2023

Choose a reason for hiding this comment

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

showing descriptions like this one don't seem to be implemented yet, at least i can not get it to show descriptions for pin locations on web and android. i'm not confident to draw conclusions about ios from looking at the code and don't have a device to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

nor can i get the body field of the legacy version in 1.6 to work for that matter.

@@ -0,0 +1,199 @@
# MSC3488 - m.location: Extending events with location data
Copy link
Contributor

@zecakeh zecakeh Jun 23, 2023

Choose a reason for hiding this comment

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

It looks like this hasn't been updated to the latest version of extensible events. Is there a reason it hasn't been updated with the rest of the other MSCs?

Comment on lines +60 to +61
For the purposes of user location tracking `m.self` should be used in order to
avoid duplicating the mxid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Element clients use m.pin for locations that are selected by the user on a map and I couldn't find it defined anywhere. should it also be defined in this MSC?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something needs to be done before action can be taken on this PR/issue. proposal A matrix spec change proposal unresolved-concerns This proposal has at least one outstanding concern
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants