Remove "default" image description #493

ilja · 2023-03-06T11:48:29Z

ilja commented

2023-03-06 11:48:29 +00:00

When no image description is filled in, Pleroma allowed fallbacks. Those were (based on a setting) either the filename, or a fixed description. Neither seem good options for image descriptions to me, so here we remove this.

I tried to find why these options were added. It seems it was first added in https://git.pleroma.social/pleroma/pleroma/-/merge_requests/470/diffs#0da54a09a904d56b49c5a55e7c96f270264a0c77_32_25. I didn't dig deeper, but at first glance it looks like name was originally meant for the filename. This makes sense since the AP field is also called that and says it's meant for a plain-text human-readable name (https://www.w3.org/TR/activitystreams-vocabulary/#dfn-name). I assume Mastodon started using it for image description because "summary" is html in AP and plain-text makes more sense. So Pleroma also started doing this, but kept the filename as fallback. The fallback became an option in https://git.pleroma.social/pleroma/pleroma/-/commit/aabc26a57327b15c1aa5ee9980b7542c9e2f4899, who also introduced an option for a fixed description. I have no idea why that option was added.

EDIT: I just stumbled upon this https://git.pleroma.social/pleroma/pleroma/-/merge_requests/3854. It seems Mastodon understands summary, but uses name as fallback for incoming objects. The "proper" way to do this is then probably to always have a summary field, and either don't use name or fill it in with the filename. But that may mean that we have to rewrite every object in the database o.O (I still need to check this to be sure). I'm also not sure how other implementations (ie foundkey) handle this.

So I see three possible routes here:

Close this MR (won't fix), and I make an issue about the summary field so maybe it can be looked at on a later date.
Refocus this MR to look deeper into the whole summary thing. This can take a while and I'm unsure of the eventual result.
Merge this MR (after review ofc), and I make an issue about the summary field so maybe it can be looked at on a later date.

I leave that choice up to the discretion of the maintainers ;)

When no image description is filled in, Pleroma allowed fallbacks. Those were (based on a setting) either the filename, or a fixed description. Neither seem good options for image descriptions to me, so here we remove this. I tried to find why these options were added. It seems it was first added in <https://git.pleroma.social/pleroma/pleroma/-/merge_requests/470/diffs#0da54a09a904d56b49c5a55e7c96f270264a0c77_32_25>. I didn't dig deeper, but at first glance it looks like `name` was originally meant for the filename. This makes sense since the AP field is also called that and says it's meant for a plain-text human-readable name (<https://www.w3.org/TR/activitystreams-vocabulary/#dfn-name>). I assume Mastodon started using it for image description because "summary" is html in AP and plain-text makes more sense. So Pleroma also started doing this, but kept the filename as fallback. The fallback became an option in <https://git.pleroma.social/pleroma/pleroma/-/commit/aabc26a57327b15c1aa5ee9980b7542c9e2f4899>, who also introduced an option for a fixed description. I have no idea why that option was added. **EDIT:** I just stumbled upon this <https://git.pleroma.social/pleroma/pleroma/-/merge_requests/3854>. It seems Mastodon understands `summary`, but uses `name` as fallback for incoming objects. The "proper" way to do this is then probably to always have a `summary` field, and either don't use `name` or fill it in with the filename. But that may mean that we have to rewrite every object in the database o.O (I still need to check this to be sure). I'm also not sure how other implementations (ie foundkey) handle this. So I see three possible routes here: * Close this MR (won't fix), and I make an issue about the `summary` field so maybe it can be looked at on a later date. * Refocus this MR to look deeper into the whole `summary` thing. This can take a while and I'm unsure of the eventual result. * Merge this MR (after review ofc), and I make an issue about the `summary` field so maybe it can be looked at on a later date. I leave that choice up to the discretion of the maintainers ;)

ilja force-pushed remove_default_image_description from 301171cc7c

ci/woodpecker/pr/woodpecker Pipeline is pending

Details

to b57e0fe4e6

ci/woodpecker/pr/woodpecker Pipeline is pending

Details

2023-03-11 10:41:46 +00:00

Compare

ilja changed title from ~~WIP: Remove "default" image description~~ to Remove "default" image description

2023-03-11 10:53:09 +00:00

ilja reviewed

2023-03-11 19:38:26 +00:00

test/pleroma/upload_test.exs Outdated

					
				@ -166,4 +157,0 @@

				      {:ok, data} = Upload.store(file)

				      assert data["name"] == "an [image.jpg"

				    end

Note: This test broke with removing the filename from this field. But this is not what it tests, and as afaict it doesn't even actually tests what it's supposed to do. Therefore I decided to just remove it instead of leaving it in what seems to be a broken state.

ilja force-pushed remove_default_image_description from b57e0fe4e6

ci/woodpecker/pr/woodpecker Pipeline is pending

Details

to 6c396fcab4

ci/woodpecker/pr/woodpecker Pipeline is pending

Details

2023-03-12 07:42:48 +00:00

Compare

floatingghost reviewed

2023-03-21 10:15:14 +00:00

floatingghost left a comment

seems good! just one little nitpick

lib/pleroma/upload.ex

					
				@ -83,3 +74,3 @@

				         upload = %__MODULE__{upload | path: upload.path || "#{upload.id}/#{upload.name}"},

				         {:ok, upload} <- Pleroma.Upload.Filter.filter(opts.filters, upload),

				         description = get_description(opts, upload),

				         description = Map.get(opts, :description) || "",

might be cleaner is use Map.get/3 here instead

might be cleaner is use `Map.get/3` here instead

Map.get/3 doesn't work in this case. The key can contain nil. So in that case the description would be nil instead of an empty string (and the rest of the code expects a string).

iex(1)> Map.get(%{description: nil}, :description, "")
nil
iex(2)> Map.get(%{description: nil}, :description) || ""
""

`Map.get/3` doesn't work in this case. The key can contain `nil`. So in that case the description would be `nil` instead of an empty string (and the rest of the code expects a string). ```iex iex(1)> Map.get(%{description: nil}, :description, "") nil iex(2)> Map.get(%{description: nil}, :description) || "" "" ```