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

Feature request: Spoiler messages #167

Open
treed opened this issue May 31, 2023 · 13 comments
Open

Feature request: Spoiler messages #167

treed opened this issue May 31, 2023 · 13 comments
Labels
compatibility enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@treed
Copy link
Contributor

treed commented May 31, 2023

I came across this in Element today, where the text was fuzzed until clicked on. I was surprised to learn that it's actually in the spec (Spoiler message) as of 1.1.

I'm not entirely sure how this could be implemented. Appearance seems relatively easy, but I can imagine it being hard to get the info out of shr. Maybe just detecting the data-mx-spoiler string and making the whole thing a spoiler in the worst case. I guess you'd also need to add another message action to reveal a spoiler.

A syntax for marking spoilers may be trickier. I don't think there's a way to do anything like it in org. Element implements it for a whole message with a preceding /spoiler, but no way to mark parts of messages even though the spec allows it. There's some discussion of various formats at element-hq/element-meta#870

Here's an example of a message containing a spoiler (with the actual spoiler removed even though it's not much of a spoiler without context):

((:id . "$RA3p_uXa0E0Qe8I0-QBqofW7H8Jfsfi-_P9t__wsKsw")
 (:sender . "@zhaofeng:zhaofeng.li")
 (:content
  (body . "[Spoiler](spoilered text here)")
  (format . "org.matrix.custom.html")
  (formatted_body . "<span data-mx-spoiler>spoilered text here</span>")
  (msgtype . "m.text"))
 (:origin-server-ts . 1685570405315)
 (:type . "m.room.message")
 (:state-key)
 (:unsigned)
 (:receipts)
 (:local))

Even if it takes longer to figure out how to send them, I think it's valuable to implement spoiler display. People using clients that support them could reasonably expect that the text they're sending is hidden to others without opt-in.

@alphapapa
Copy link
Owner

Hm, I hadn't seen that before.

Well, it might not be too hard to implement, actually. We could probably override the shr function for the span tag and then apply some text properties to make the text invisible unless hovered over or selected (i.e. set the text color to the background color, and use a mouseover or help-echo function to show the spoiler).

I'd be glad if someone else would like to work on this. I'll offer guidance, of course.

@alphapapa alphapapa added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers compatibility labels May 31, 2023
@alphapapa alphapapa added this to the Future milestone May 31, 2023
@9viz
Copy link
Contributor

9viz commented Jun 1, 2023

How about the patch below? It does not handle plain text bodies as I don't see any particular format being enforced so we might need to do some funny business if we decide to also support plain text bodies (who else other than me uses it BTW?)

diff --git a/ement-room.el b/ement-room.el
index 2b36967..47ef841 100644
--- a/ement-room.el
+++ b/ement-room.el
@@ -3484,12 +3490,35 @@ If FORMATTED-P, return the formatted body content, when available."
        (map-elt content 'body)))
    body))
 
+(defun ement-room--normalise-spoiler-tag ()
+  "Normalise spoiler <span> tag to always have a value.
+We need it because `libxml-parse-region' does not include
+`data-mx-spoiler' in the attribute list if the tag is simply
+`<span data-mx-spoiler>'."
+  (save-excursion
+    (goto-char (point-min))
+    (while (search-forward "span data-mx-spoiler" nil 'noerror)
+      (when (eq (char-after) ?>)
+        (insert "=\"\"")))))
+
+(defun ement-room--tag-span (dom)
+  "Render <span> DOM specially if it is a spoiler."
+  (if-let ((spoiler-reason (dom-attr dom 'data-mx-spoiler)))
+      (insert (propertize "Spoiler"
+                          'face 'warning
+                          'help-echo
+                          (concat (unless (equal spoiler-reason "")
+                                    (concat "Reason: " spoiler-reason "\n\n"))
+                                  (dom-text dom))))
+    (shr-tag-span dom)))
+
 (defun ement-room--render-html (string)
   "Return rendered version of HTML STRING.
 HTML is rendered to Emacs text using `shr-insert-document'."
   (with-temp-buffer
     (insert string)
     (save-excursion
+      (ement-room--normalise-spoiler-tag)
       ;; NOTE: We workaround `shr`'s not indenting the blockquote properly (it
       ;; doesn't seem to compensate for the margin).  I don't know exactly how
       ;; `shr-tag-blockquote' and `shr-mark-fill' and `shr-fill-line' and
@@ -3498,6 +3527,8 @@ HTML is rendered to Emacs text using `shr-insert-document'."
       ;; resized (i.e. the wrapping is adjusted automatically by redisplay
       ;; rather than requiring the message to be re-rendered to HTML).
       (let ((shr-use-fonts ement-room-shr-use-fonts)
+            (shr-external-rendering-functions (cons (cons 'span #'ement-room--tag-span)
+                                                    shr-external-rendering-functions))
             (old-fn (symbol-function 'shr-tag-blockquote))) ;; Bind to a var to avoid unknown-function linting errors.
         (cl-letf (((symbol-function 'shr-fill-line) #'ignore)
                   ((symbol-function 'shr-tag-blockquote)

@alphapapa
Copy link
Owner

@Vizs That looks like a good start, thanks. BTW, I'm not sure what you mean about plain-text bodies. All I know is what is shown in the spec.

@9viz
Copy link
Contributor

9viz commented Jun 1, 2023

Even though the example event given by treed and the Matrix spec suggest that there is a fixed format for spoiler boundary markers in plain text bodies, that seems to be not the case.

For example, notice how there's no [Spoiler](actual text here) in the plain text body in the below event.

((:id . "$2hRcN3OyAguIHxJWpGZNAW1mwmy8_ZHK2zLByJgU3ws")
 (:sender . "@suckless_shill:matrix.org")
 (:content
  (body . "tttt")
  (format . "org.matrix.custom.html")
  (formatted_body . "<span data-mx-spoiler>tttt</span>")
  (msgtype . "m.text"))
 (:origin-server-ts . 1685593931268)
 (:type . "m.room.message")
 (:state-key)
 (:unsigned
  (age . 134))
 (:receipts)
 (:local))

Here's another example of a message with spoiler generated by the t2bot discord bridge:

{
 "content": {
   "body": "trans because too lazy to search to mangadex \n(Spoiler:  So, what's the current situation? )\n(Spoiler:  We going to the moon )",
   "format": "org.matrix.custom.html",
   "formatted_body": "trans because too lazy to search to mangadex <br><span data-mx-spoiler> So, what&#x27;s the current situation? </span><br><span data-mx-spoiler> We going to the moon </span>",
   "msgtype": "m.text"
 },
 "origin_server_ts": 1657878408585,
 "room_id": "!KuqykPKpfZseOssmhX:matrix.org",
 "sender": "@_discord_257954736689512448:t2bot.io",
 "type": "m.room.message",
 "unsigned": {},
 "event_id": "$DrIF8IDT0oyC5YsYTlks96RfJAzaVvyQR-ttuxaT0dk",
 "user_id": "@_discord_257954736689512448:t2bot.io"
}

@alphapapa
Copy link
Owner

@Vizs I see, thanks. IMO we should strictly adhere to the spec here, and any clients that don't follow it are sending spoilers at their own risk. :)

@9viz
Copy link
Contributor

9viz commented Jun 2, 2023

The first example I showed above was a message sent using Element. If Element does not care about doing the right thing TM, then anything goes for other clients. (:

In the end, it is your call as to decide whether we should support markdown-style spoiler message or just inconvenience plain text body users.

@alphapapa
Copy link
Owner

IIUC even Element is "inconveniencing plain text body users," right?

It's hard enough to support the many features of the spec without also having to support pre-spec, proposed/experimental behaviors too. Let's just follow the spec here. They'll probably update Element to follow it eventually.

@9viz
Copy link
Contributor

9viz commented Jun 2, 2023

IIUC even Element is "inconveniencing plain text body users," right?

Yes, that is the case. A lot of times, you lose out when you use plain text body. Personally, I gave up on plain text body, and change shr-external-render-functions to render HTML like plain text.

@alphapapa
Copy link
Owner

IIUC even Element is "inconveniencing plain text body users," right?

Yes, that is the case. A lot of times, you lose out when you use plain text body. Personally, I gave up on plain text body, and change shr-external-render-functions to render HTML like plain text.

I don't understand what you mean about that.

@9viz
Copy link
Contributor

9viz commented Jul 13, 2023

I worked more on rendering spoilers in plain text bodies---falling back to search-and-replace when the plain text body does not include the spoiler as a MD link. This seems to work in my minimal testing, but it is rather hard to present the changes since my patch is on top of the rich-reply changes in #150 (and a rather old copy of that PR, at that). I would happy be to make a PR for spoiler message rendering once the rich-reply support is merged in. In any case, if you are interested the patch is at https://gist.github.com/vizs/458a3a95d6d93a128182030c588bfabd.

As for sending spoiler messages, I think a command like the reaction command would be in order? I'm not sure how you want to implement it.

@alphapapa
Copy link
Owner

alphapapa commented Jul 13, 2023

I worked more on rendering spoilers in plain text bodies---falling back to search-and-replace when the plain text body does not include the spoiler as a MD link. This seems to work in my minimal testing, but it is rather hard to present the changes since my patch is on top of the rich-reply changes in #150 (and a rather old copy of that PR, at that). I would happy be to make a PR for spoiler message rendering once the rich-reply support is merged in. In any case, if you are interested the patch is at https://gist.github.com/vizs/458a3a95d6d93a128182030c588bfabd.

Ok. I'm still not sure what the best course of action is with regard to the rich replies. I guess it should be gated behind an option.

As for sending spoiler messages, I think a command like the reaction command would be in order? I'm not sure how you want to implement it.

The best idea I have would be to use Org's strikethrough syntax and rebind the Org export HTML function for that syntax to output the spoiler tags. Of course, that would preclude writing messages with actual strikethrough syntax, so maybe we should use something else.

@9viz
Copy link
Contributor

9viz commented Jul 14, 2023

I'm still not sure what the best course of action is with regard to the rich replies. I guess it should be gated behind an option.

Sending rich replies can be gated under an option but I hope rendering won't be. As per the protocol, one should include the fallback in the event body when sending a rich reply but clients can be lazy and not include them (which is why I made the PR in the first place).

The best idea I have would be to use Org's strikethrough syntax and rebind the Org export HTML function for that syntax to output the spoiler tags. Of course, that would preclude writing messages with actual strikethrough syntax, so maybe we should use something else.

I don't think you can think up new markup syntax in org (like = + and friends) but write a special block like #+BEGIN_SPOILER. Unfortunately, this would be well beyond my abilities to implement.

@alphapapa
Copy link
Owner

I'm still not sure what the best course of action is with regard to the rich replies. I guess it should be gated behind an option.

Sending rich replies can be gated under an option but I hope rendering won't be. As per the protocol, one should include the fallback in the event body when sending a rich reply but clients can be lazy and not include them (which is why I made the PR in the first place).

Yes, that's what I meant.

The best idea I have would be to use Org's strikethrough syntax and rebind the Org export HTML function for that syntax to output the spoiler tags. Of course, that would preclude writing messages with actual strikethrough syntax, so maybe we should use something else.

I don't think you can think up new markup syntax in org (like = + and friends) but write a special block like #+BEGIN_SPOILER. Unfortunately, this would be well beyond my abilities to implement.

Maybe, but see variable org-emphasis-alist. Anyway, I think a block would be generally better, although that would seem to require using multiple lines.

I wonder if we should just implement our own, very simple syntax, something like:

Hi this is a message with a spoiler//hidden spoiler//spoiler part

that would render like

Hi this is a message with a [spoiler] part

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants