Add embed entity #101

SuperTux88 · 2018-04-09T01:05:07Z

Fixes #93

I did a few changes to the proposal:

I renamed link to url.
I added a nothing property for when nothing should be embedded and nothing should be scanned. I think this is better to be explicit in this case. When this is true the url property is optional, otherwise it is required.

cc @annando

annando

Just some copy&paste stuff. And some clarification would be great if Markdown is allowed in the description.

Additionally I would prefer to mention that the doesn't need to be present in the status message.

annando · 2018-04-09T05:17:44Z

docs/_entities/embed.md

+
+| Property      | Type (Length)            | Description                                                              |
+| ------------- | ------------------------ | ------------------------------------------------------------------------ |
+| `url`         | [URL][url] (65535)       | A string describing your location, e.g. a city name, a street name, etc. |


" A string describing your location"? Looks like some copy&paste ;-)

annando · 2018-04-09T05:18:31Z

docs/_entities/embed.md

+| Property      | Type (Length)            | Description                                                              |
+| ------------- | ------------------------ | ------------------------------------------------------------------------ |
+| `url`         | [URL][url] (65535)       | A string describing your location, e.g. a city name, a street name, etc. |
+| `title`       | [String][string] (255)   | A string describing your location, e.g. a city name, a street name, etc. |


Same as above

annando · 2018-04-09T05:18:37Z

docs/_entities/embed.md

+| ------------- | ------------------------ | ------------------------------------------------------------------------ |
+| `url`         | [URL][url] (65535)       | A string describing your location, e.g. a city name, a street name, etc. |
+| `title`       | [String][string] (255)   | A string describing your location, e.g. a city name, a street name, etc. |
+| `description` | [String][string] (65535) | The geographical latitude of your location.                              |


annando · 2018-04-09T05:18:53Z

docs/_entities/embed.md

+| `url`         | [URL][url] (65535)       | A string describing your location, e.g. a city name, a street name, etc. |
+| `title`       | [String][string] (255)   | A string describing your location, e.g. a city name, a street name, etc. |
+| `description` | [String][string] (65535) | The geographical latitude of your location.                              |
+| `image`       | [URL][url] (65535)       | The geographical longitude of your location.                             |


and again :-)

annando · 2018-04-09T05:19:05Z

docs/_entities/embed.md

+| `title`       | [String][string] (255)   | A string describing your location, e.g. a city name, a street name, etc. |
+| `description` | [String][string] (65535) | The geographical latitude of your location.                              |
+| `image`       | [URL][url] (65535)       | The geographical longitude of your location.                             |
+| `nothing`     | [Boolean][boolean]       | The geographical longitude of your location.                             |


And again ;-)

SuperTux88 · 2018-04-09T08:53:47Z

Oh yes, I forget to change the descriptions m( fixed that now.

Markdown is not allowed, it also wouldn't make any sense, because this is provided by websites for all sorts of use-cases and not only diaspora, so I don't think any website would provide markdown there.

The embed is in the optional section of the status_message entity, I think that is enough to show that it is optional.

annando · 2018-04-09T09:50:30Z

I would prefer having some "Markdown is not allowed" in the documentation, just to clarify this. And yes, if that hadn't been cleared, I had put it through our HTML to Markdown interpreter, since the fetched description sometimes contain HTML elements. BTW: Is only plaintext allowed or HTML elements?

With "optional" I meant that the "embed" could point to an address that doesn't exist in the status message at all. So you could include the links A, B and C in your post, but the "embed" points to the link "D". This is a little bit nitpicking to mention this, but one does never know how people are interpreting specifications.

SuperTux88 · 2018-04-09T11:08:43Z

Is only plaintext allowed or HTML elements?

Everything is "allowed" (websites can put what they want in that field), but only plaintext is rendered. It has the type String which allows just everything. We have many things that "doesn't allow" markdown, but we don't mention when something doesn't allow markdown, we mention when something explicitly allows Markdown.

With "optional" I meant that the "embed" could point to an address that doesn't exist in the status message at all.

While this is theoretically possible, I wouldn't recommend this so I don't think it is a good idea to mention that. When you link to a url that isn't in the status message at all, not everybody can see it (when not every software supports this entity, or old versions), so maybe people talk about a link in the comments, that not everybody can see. While when you include the link in the message too, some softwares maybe embed the wrong url or embed nothing at all (if they don't support embedding at all), but at least everybody can see all links. So I would always include the link in the message for accessibility reasons.

SuperTux88 · 2018-04-09T22:11:44Z

@annando I added a little paragraph about the second part now, I hope that it's clear know that the link should also be included in the text but doesn't need to match 1:1.

annando · 2018-04-09T22:27:58Z

I still am confused by:

Everything is "allowed" (websites can put what they want in that field), but only plaintext is rendered.

So how do I force a linefeed in the description? By doing some CR or with
?

SuperTux88 · 2018-04-10T00:17:55Z

So how do I force a linefeed in the description?

The description is only plain text (see spec), there are no newlines or other formatting.

SuperTux88 · 2018-05-15T23:11:41Z

@annando are there any open points here or can you approve this PR?

cmrd-senya

Some minor things, otherwise looks good to me!

docs/_entities/embed.md

cmrd-senya · 2018-10-05T16:52:57Z

docs/_entities/embed.md

+
+## Optional Properties
+
+All entities are "optional", but either `url` is required or `nothing` must be `true`.


All entities are
These are not entities, these are properties.

Nit: I'm not sure if we need to put "optional" in the quotes here. I think it looks better without quotes.

So it should be:

All properties are optional, but either `url` is required or `nothing` must be `true`.

cmrd-senya · 2018-10-05T16:54:22Z

docs/_entities/embed.md

+| `title`       | [String][string] (255)   | The title of the embedded URL.        |
+| `description` | [String][string] (65535) | The description of the embedded URL.  |
+| `image`       | [URL][url] (65535)       | The image of the embedded URL.        |
+| `nothing`     | [Boolean][boolean]       | `true' if nothing should be embedded. |


There are inconsistent quotes around true here

cmrd-senya · 2018-10-05T17:26:58Z

lib/diaspora_federation/entities/embed.rb

+      def validate
+        super
+
+        raise ValidationError, "URL set and 'nothing' is 'true'" if url && nothing


Nit: These two lines can be replaced with one by using xor:

raise ValidationError, "Either URL must be set or 'nothing' must be 'true'" unless nothing ^ url

Also, if we refer the properties' names then it is better to write them the same way, i.e. 'url' instead of URL, so that it matches 'nothing' in style. So: Either 'url' must be set or 'nothing' must be 'true'

spec/lib/diaspora_federation/validators/embed_validator_spec.rb

Fixes diaspora#93

Add embed entity

annando suggested changes Apr 9, 2018

View reviewed changes

SuperTux88 force-pushed the add-embed-entity branch from 9344178 to 8b6bc82 Compare April 9, 2018 08:46

SuperTux88 force-pushed the add-embed-entity branch from 8b6bc82 to 9688f3f Compare April 9, 2018 22:10

SuperTux88 requested a review from cmrd-senya May 15, 2018 23:11

annando approved these changes May 16, 2018

View reviewed changes

cmrd-senya requested changes Oct 5, 2018

View reviewed changes

Add "embed" entity

e0da26c

Fixes diaspora#93

SuperTux88 force-pushed the add-embed-entity branch from 9688f3f to 1fecfa3 Compare October 5, 2018 18:26

Add documentation for embed entity

12c1cd8

SuperTux88 force-pushed the add-embed-entity branch from 1fecfa3 to 12c1cd8 Compare October 5, 2018 18:38

cmrd-senya approved these changes Oct 5, 2018

View reviewed changes

SuperTux88 merged commit 12c1cd8 into diaspora:develop Oct 5, 2018

SuperTux88 added a commit that referenced this pull request Oct 5, 2018

Merge pull request #101 from SuperTux88/add-embed-entity

8d6006c

Add embed entity

SuperTux88 deleted the add-embed-entity branch October 5, 2018 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add embed entity #101

Add embed entity #101

SuperTux88 commented Apr 9, 2018

annando left a comment

annando Apr 9, 2018

annando Apr 9, 2018

annando Apr 9, 2018

annando Apr 9, 2018

annando Apr 9, 2018

SuperTux88 commented Apr 9, 2018

annando commented Apr 9, 2018

SuperTux88 commented Apr 9, 2018

SuperTux88 commented Apr 9, 2018

annando commented Apr 9, 2018

SuperTux88 commented Apr 10, 2018 •

edited

SuperTux88 commented May 15, 2018

cmrd-senya left a comment

cmrd-senya Oct 5, 2018

cmrd-senya Oct 5, 2018

cmrd-senya Oct 5, 2018

cmrd-senya Oct 5, 2018


		## Optional Properties

		All entities are "optional", but either `url` is required or `nothing` must be `true`.

Add embed entity #101

Add embed entity #101

Conversation

SuperTux88 commented Apr 9, 2018

annando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperTux88 commented Apr 9, 2018

annando commented Apr 9, 2018

SuperTux88 commented Apr 9, 2018

SuperTux88 commented Apr 9, 2018

annando commented Apr 9, 2018

SuperTux88 commented Apr 10, 2018 • edited

SuperTux88 commented May 15, 2018

cmrd-senya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperTux88 commented Apr 10, 2018 •

edited