Add: WIP support for URL previews #63

rayes0 · 2022-04-14T04:46:40Z

Implements basic support for asynchronous URL previews, currently almost everything is working. Getting the preview, inserting the preview to the proper place, inserting later messages to the proper places, etc.

It can be turned on by the variable ement-room-url-previews, set to nil by default because some homeservers don't support URL previews (probably be best to mention something in the readme about this if it gets merged).

There are two things that currently don't work very well (probably more I haven't found yet though, haven't thoroughly tested this):

Type errors when sender headers are enabled coming from somewhere in this block:

ement.el/ement-room.el

Line 2379 in 4692961

(when ement-room-sender-headers

I'm not too sure why, all the macros make it hard to debug, though I suspect it is trivial. And everything appears to work fine except that it throws an error. The previews are inserted correctly, and even the headers are inserted correctly.
Sometimes there is a race condition when the pretty printer is called in quick succession and the previews haven't had time to be inserted into the EWOC yet causing two previews to be inserted. I'm not sure how to solve this one, it appears how you did it with inserting retro messages was by setting a variable while loading and not starting a process it the variable is set. I'm not sure how that would work with URL previews, especially multiple URLs in the same message. Any help/suggestions would be appreciated :)

Feel free to edit, change, or delete anything you wish :)

alphapapa · 2022-04-14T08:51:24Z

Thanks. I like the way you render the previews with the bar to the left, it looks nice.

A few notes:

As I mentioned in the chat room, before this could be accepted, I'd need you to do the FSF copyright assignment, because I plan to submit Ement to GNU ELPA. If you're uninterested or unable to do that, this could still be useful as a proof-of-concept.

For the implementation: Generally, we should avoid adding new types in the EWOC nodes, because that would require changes in several places that check node types and easily lead to new bugs (like the one you noted about sender headers). So rather than inserting a new node for each preview, it would be simpler to handle it like reactions, by updating the message formatter to append any previews to the end of a message event. The preview-getting callback would add the preview data to the event's local slot and invalidate the node in the EWOC, which would cause the message to be reformatted and display the preview at the end. That would also naturally keep the previews with the message content they are for, and it would probably avoid the race condition you mentioned, because fetching previews and displaying them should be idempotent actions.

alphapapa · 2022-04-15T14:31:46Z

As I mentioned in the chat room, what's probably needed is for me to implement a queue system in plz so that requests like this could be queued.

Regarding the current code in this PR: it's not quite what I had in mind. It could be simpler and integrate more naturally into the existing code. But I can't ask you to put more work into this until you've done the FSF CA, otherwise your work could be in vain, and I'll end up having to do it myself anyway. So I don't think I should give more specific feedback until that's done.

alphapapa · 2022-04-15T18:52:30Z

FYI, here's my WIP of using plz request queues:

alphapapa

Thanks for your work on this. I made some comments on the code.

ement-room.el

alphapapa · 2022-05-02T12:28:07Z

ement-room.el

-                 do (when-let ((inner-data (map-elt data (intern (concat "preview-" (number-to-string i))))))
-                      (unless (looking-at-p "^\n") (insert "\n"))
-                      (insert (ement-room--format-url-preview inner-data)))))
+        (dolist (preview previews)


This is looking better. Now it probably should go into its own function, similar to ement-room--format-m.image. And the code that calls it should probably go in ement-room--format-message-body, treating it as a kind of "appendix" to the message body.

I've currently left it how it is, mainly due to the fact that there is already a function named ement-room--format-url-preview. I think the most reasonable way to go about it is integrating the dolist into ement-room--format-url-preview and calling it from ement-room--format-message-body, but just wanted to confirm with you before I do it.

I'll also probably implement the message parsing in this combined function to insert the previews in the correct order rather than just iterating using dolist.

ement-room.el

rayes0 · 2022-05-13T01:21:32Z

Sorry for not working on this in a while, was mainly waiting for the FSF CA to get done. Finally got it complete the other day.

alphapapa · 2022-05-13T07:44:14Z

Sorry for not working on this in a while, was mainly waiting for the FSF CA to get done. Finally got it complete the other day.

No problem. That's great news. I don't have time for another full review, but I'll make a few comments...

alphapapa

Thanks.

alphapapa · 2022-05-13T07:44:46Z

ement-room.el

@@ -2573,25 +2573,25 @@ Formats according to `ement-room-message-format-spec', which see."
  ;; TODO: Like other events, pop to a buffer showing the raw reaction events when a key is pressed.
  (if-let ((reactions (map-elt (ement-event-local event) 'reactions)))
      (cl-labels ((format-reaction
-                   (ks) (pcase-let* ((`(,key . ,senders) ks)


Please remove any whitespace-only hunks from the patch.

alphapapa · 2022-05-13T07:47:01Z

ement-room.el

+
+(defun ement-room--preview-urls (event rendered)
+  "Parse RENDERED body of EVENT for URLs and fetch previews for them."
+  (if (and (not (alist-get 'url-previews (ement-event-local event)))


I'm not sure what rendered is here.

alphapapa · 2022-05-13T07:47:34Z

ement-room.el

+  "Parse RENDERED body of EVENT for URLs and fetch previews for them."
+  (if (and (not (alist-get 'url-previews (ement-event-local event)))
+           rendered)
+      (let ((room-buffer (alist-get 'buffer (ement-room-local ement-room))))


If you can, please use pcase-let to destructure when possible. I'm trying to use it consistently (although sometimes using alist-get is still necessary).

alphapapa · 2022-05-13T07:48:43Z

ement-room.el

+          (insert rendered)
+          (goto-char (point-min))
+          (cl-loop with match = nil
+                   while (setf match (or (text-property-search-forward 'ement-url)


Why not just do a regexp search for URLs on the message's plain-text body? It will probably be faster, because Emacs's regexp engine is very well optimized, and the code will be much simpler.

alphapapa · 2022-05-13T07:49:33Z

ement-room.el

-(defun ement-room--insert-preview (message related num data)
-  "Insert link preview for DATA into MESSAGE from node RELATED if it isn't already present.
-NUM is the index for this link."
+(defun ement-room--insert-preview (message related key data)


Please write in terms of events rather than messages, because events are the logical unit we work in here.

alphapapa · 2022-05-13T07:50:37Z

ement-room.el

-NUM is the index for this link."
+(defun ement-room--insert-preview (message related key data)
+  "Insert link preview for HTTP response DATA into MESSAGE from node RELATED,
+if it isn't already present. KEY will be the key used for inserting."


I don't understand what "KEY will be the key used for inserting" means.

As well, "related" could be more clearly written as "related-event" or "related-node", etc.

alphapapa · 2022-05-13T07:51:21Z

ement-room.el

-NUM is the index for this link."
+(defun ement-room--insert-preview (message related key data)
+  "Insert link preview for HTTP response DATA into MESSAGE from node RELATED,
+if it isn't already present. KEY will be the key used for inserting."
  (cond ((alist-get 'errcode data) (ement-debug "Rate limit in url preview"))
        (data (let ((related-node (ement-room--ewoc-node-before ement-ewoc


Please avoid one-symbol-per-line formatting style. This function has (declare (indent defun)) specified to avoid this. :)

alphapapa · 2022-05-13T07:54:23Z

ement-room.el

-                  (cl-pushnew (cons 'url-previews nil) (ement-event-local related)))
-                (unless (alist-get key (map-elt (ement-event-local related) 'url-previews))
-                  (cl-pushnew (cons key data) (map-elt (ement-event-local related) 'url-previews)))
+                    (event (ement-event-local related)))


If you're binding the event's local alist slot, the binding should be named local, not event--event should be the event struct itself.

But since you need to push to that alist anyway, you probably shouldn't bind it. I may be mistaken, but I don't think setting the bound variable will also set the slot value; you need to set the place form's value (at least, that's been my experience).

alphapapa · 2022-05-13T07:55:33Z

ement-room.el

-                (unless (alist-get key (map-elt (ement-event-local related) 'url-previews))
-                  (cl-pushnew (cons key data) (map-elt (ement-event-local related) 'url-previews)))
+                    (event (ement-event-local related)))
+                (unless (alist-get 'url-previews event)


Please be consistent: use either map-elt or alist-get. :) (If performance matters, using alist-get will be faster, because map-elt is generic. But see also map-nested-elt, which can make for clearer code than nested alist-gets, and it's fast enough--although I don't think setf works with it, yet.)

alphapapa · 2022-05-13T07:57:44Z

ement-room.el

                (ewoc-invalidate ement-ewoc related-node)))))

 (defun ement-room--format-url-preview (data)
-  "Return a pretty-printed preview for DATA."
+  "Return a pretty-printed preview for DATA, an alist of parsed JSON from the homeserver."


Nitpick: To keep the first line of the docstring short, the second clause (which is just supporting information) can go in a second sentence on a second line. (Checkdoc will complain, otherwise (although getting rid of all Checkdoc warnings is sometimes impractical)).

Add: WIP support for URL previews

37fd15d

rayes0 added 2 commits April 14, 2022 20:00

Fix: First attempt at implementing by inserting after body

fabd473

Fix: Solve infinite recursion error

0411b60

alphapapa force-pushed the master branch 2 times, most recently from 12c2abf to 2e072ef Compare April 15, 2022 15:30

Fix/Change: Small fixes in places and simplify implementation

f5e77ae

alphapapa requested changes Apr 21, 2022

View reviewed changes

alphapapa force-pushed the master branch from 9ff0e30 to 70da19e Compare April 22, 2022 21:07

Fix/Change: Remove debug code, fix conventions, add line wrapping

8545864

rayes0 mentioned this pull request May 1, 2022

Change: Only highlight actual message body when replying #69

Draft

alphapapa requested changes May 2, 2022

View reviewed changes

alphapapa force-pushed the master branch from 79b4555 to 4b94c46 Compare May 7, 2022 14:44

Fix/Change: Key on URL string rather than index

69a7de6

alphapapa requested changes May 13, 2022

View reviewed changes

alphapapa force-pushed the master branch from 6859771 to 0b58768 Compare July 9, 2022 14:26

alphapapa force-pushed the master branch 3 times, most recently from 4a42004 to 481f571 Compare July 21, 2022 15:38

alphapapa force-pushed the master branch 2 times, most recently from a6ec9ad to fcbf1a5 Compare September 2, 2022 12:44

alphapapa added the waiting label Sep 9, 2022

alphapapa force-pushed the master branch 2 times, most recently from 9845fce to 338b99b Compare September 12, 2022 08:48

alphapapa force-pushed the master branch 2 times, most recently from 7d2e0f2 to c0b922b Compare May 14, 2023 04:24

alphapapa added this to the Future milestone Jul 9, 2023

alphapapa self-assigned this Jul 9, 2023

alphapapa added the enhancement New feature or request label Jul 9, 2023

alphapapa force-pushed the master branch 4 times, most recently from a68abe0 to 9da7b8e Compare September 7, 2023 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: WIP support for URL previews #63

Add: WIP support for URL previews #63

rayes0 commented Apr 14, 2022 •

edited

Loading

alphapapa commented Apr 14, 2022 •

edited

Loading

alphapapa commented Apr 15, 2022

alphapapa commented Apr 15, 2022

alphapapa left a comment

alphapapa May 2, 2022

rayes0 May 13, 2022

rayes0 May 13, 2022

rayes0 commented May 13, 2022

alphapapa commented May 13, 2022

alphapapa left a comment

alphapapa May 13, 2022

alphapapa May 13, 2022

alphapapa May 13, 2022

alphapapa May 13, 2022

alphapapa May 13, 2022

alphapapa May 13, 2022 •

edited

Loading

alphapapa May 13, 2022

alphapapa May 13, 2022

alphapapa May 13, 2022

alphapapa May 13, 2022

Add: WIP support for URL previews #63

Are you sure you want to change the base?

Add: WIP support for URL previews #63

Conversation

rayes0 commented Apr 14, 2022 • edited Loading

alphapapa commented Apr 14, 2022 • edited Loading

alphapapa commented Apr 15, 2022

alphapapa commented Apr 15, 2022

alphapapa 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

rayes0 commented May 13, 2022

alphapapa commented May 13, 2022

alphapapa 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

alphapapa May 13, 2022 • edited Loading

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

rayes0 commented Apr 14, 2022 •

edited

Loading

alphapapa commented Apr 14, 2022 •

edited

Loading

alphapapa May 13, 2022 •

edited

Loading