Skip to content
This repository has been archived by the owner on Mar 14, 2023. It is now read-only.

Early WIP: "next-gen" client #27

Closed
wants to merge 139 commits into from
Closed

Early WIP: "next-gen" client #27

wants to merge 139 commits into from

Conversation

alphapapa
Copy link
Owner

Hi Jay,

If you have the time and interest, I would appreciate your feedback on this. This builds on the api-r0.3.0 branch, adding the matrix-client-ng.el file, as well as some changes to the matrix-api-r-0.3.0.el file. It's currently in a basic working state: you can log in, it opens rooms you've joined, and starts polling for new events. You can send messages and join rooms, as well as use /me and /who commands. It should appear to work the same as the old client code, minus the unimplemented parts and any bugs. Hopefully it's a bit simpler and easier to follow.

I wasn't sure if I wanted to add an event-handler-defining macro, similar to the one in the old client code, but when I saw how many of the same variables I'd have to bind in each handler, it seemed like a good idea, to reduce mistakes and boilerplate, so I did. Hopefully it is an aid and not a hindrance.

Rather than using a lot of hooks for lots of events, I'm using only a few hooks, and a list of new events that the client can process from a single hook. I wanted to avoid having to define two handlers for every event, one in the API and one in the client. So far it seems to be working well.

This is an early WIP, so there are lots of little FIXMEs and such here and there. Mainly I want to know if you approve of the general structure and the macros. If so, I can continue working on it, hopefully providing a foundation for moving the whole package forward.

Thanks!

The initial sync does not seem to continue polling, but if I manually
sync it once, it then does poll and receive new messages automatically.
Polling starts automatically on login now.

Might need to find a way to stop any outstanding requests upon logout...
Having a weird bug, some code only seems to work intermittently, or
always if edebugged.  Added a bunch of debug messages to try to track it down.
Seems the bugs were in the notifications file, which I hadn't updated
yet.  And for some reason I wasn't getting backtraces, which made it
nearly impossible to track down.  When I restarted Emacs, the backtraces
returned...
Spaces are not required between strings and symbols, so the overall
length is actually shorter, and it's arguably much clearer.
@eqyiel
Copy link

eqyiel commented Nov 27, 2017

@alphapapa this looks awesome, thanks for putting in the effort to revive matrix-client!

@alphapapa
Copy link
Owner Author

@eqyiel Thanks for the kind words. There is a long way to go still. This needs lots of testing and debugging. It is--barely--usable at the moment, so feel free to test basic sending/receiving/joining if you feel like it, but I'm not sure it's even worth anyone else testing yet. :) The master branch is working quite stably, and it's still what I'm using as my main client.

@@ -1,3 +1,122 @@
(defmacro format$ (string &rest objects)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oooh, this is really nice, I like it a lot :D

@jgkamat
Copy link
Collaborator

jgkamat commented Nov 30, 2017

rather than mapping keys to objects in global vars, I'm trying to stay inside objects, so I added this slot so that the client side of the package can store arbitrary data in the API side's objects. So far I'm only storing the buffer in there, and it's easily accessible with a with-room-buffer macro and with-slots*. Does this seem like a good approach to you?

Yup, I like this method a lot more than the previous layout, I think it's good idea!

Rather than using a lot of hooks for lots of events, I'm using only a few hooks, and a list of new events that the client can process from a single hook. I wanted to avoid having to define two handlers for every event, one in the API and one in the client. So far it seems to be working well.

I'm fine with this as well, the previous way was a lot of boilerplate. The previous hooks were a bit nice for following the structure, but I think that it's simpler if we just have one hook

I did a quick skim of this PR and it looks good overall to me, I'll try to get a more thourough review in later on (but my exams are coming up, so I might not get to it until after that). Thanks again for all this, it looks amazing 🎉

Optionally.  Disabled by default.
When syncing, it seems that room-related events can occur or be
processed before the room-joining event, so we should create the buffer
any time we process a room-related event and the buffer doesn't exist.
Can easily be extended to other filetypes later.

Squashed commit of the following:

commit 96dc95c
Author: Adam Porter <[email protected]>
Date:   Wed Oct 10 10:11:07 2018 -0500

    Change: Don't advise url-http-create-request

    Use cl-letf to override it where needed.

commit d7591f9
Author: Adam Porter <[email protected]>
Date:   Sun Sep 2 00:13:35 2018 -0500

    Tidy: upload callback

commit 0de8f93
Author: Adam Porter <[email protected]>
Date:   Sat Sep 1 23:57:27 2018 -0500

    Comment: Add comment about fixing the fix

commit b02190f
Author: Adam Porter <[email protected]>
Date:   Sun Aug 26 23:52:45 2018 -0500

    Add: /upload command

commit 48da805
Author: Adam Porter <[email protected]>
Date:   Sun Aug 26 23:52:20 2018 -0500

    Add/Change: Download HTTP paths, then upload

    Also convert function to method

commit e1f5eb5
Author: Adam Porter <[email protected]>
Date:   Sun Aug 26 23:50:36 2018 -0500

    Fix: Get image type from file header if no extension

commit a1b4c3f
Author: Adam Porter <[email protected]>
Date:   Thu Aug 23 00:33:09 2018 -0500

    WIP: Image upload

    Plus everyone's favorite, cowsay!
It's impossible to upload files with url.el unless
url-http-create-request is overridden with a fixed version, and that
must be done with function advice (which applies to everything in
Emacs), because doing it with cl-letf doesn't work (who knows why--it
works fine for other things).  Obviously we don't want to override
that function globally, even though it works fine now, because when
url.el changes in the future, that might break something.  And it's
not a good idea, anyway.

So let's switch back to request.el.  Frankly, I don't even remember
why I switched to my url-with-retrieve-async function.  There is this
pesky bug at tkf/emacs-request#92, but that
only applies to synchronous requests, which we aren't using.
@alphapapa
Copy link
Owner Author

@jgkamat FYI: 21c8af2

@alphapapa
Copy link
Owner Author

alphapapa commented Oct 10, 2018

@jgkamat Okay, well, now I remember what the problem with request is: sync requests start getting duplicated. The last message I received in a room arrived 21 times! When I first run matrix-client-ng-connect, it's fine. But over time, sync requests start getting duplicated. I don't know how it would be possible for that to happen, but it is happening.

And I don't know where to begin to debug it. Digging through the log buffer to find where a sync request is first duplicated is tedious and error-prone. Even if I found where, it wouldn't tell me why it happened. And request is labyrinthine and very difficult to follow by stepping through with edebug. And this never happens with url.

I guess our best option now is to keep using url-with-retrieve-async for everything that works for, and use request only for HTTP requests that don't work with url. Sigh. What a mess.

alphapapa and others added 24 commits October 10, 2018 13:54
This reverts commit 21c8af2.

request.el has a bug which causes our sync requests to get duplicated.
The last message I received in a room arrived 21 times because of 21
simultaneous sync requests.  I don't know what is causing it, and
request.el is very difficult to debug.  So we will have to go back to
using url.el for everything we can use it for.  For a few things that
don't work with url.el, like uploading files, we'll have to use
request.el.

What a mess.  And how disappointing that there is no HTTP library for
Emacs that doesn't have these kinds of virtually-impossible-to-debug
bugs.

See #27 (comment)
Also allows us to easily replay events into a buffer.
In large rooms (e.g. #emacs with > 600 users), initial sync is now
faster because 1) the room name is only computed twice (when the room
buffer is created, and when the initial sync is completed) rather than
for every user "join" event (and the initial computation is very fast,
because there are one or no users known to be in the room when the
buffer is first created), and 2) we short-circuit the computations
when possible, avoiding looking up displaynames for users
unnecessarily.
This may not be the best way to do this.  It is simple, and should be
fast enough, but there might be a cleaner way to handle this.
Probably need to refactor a bit, or add some functionality to
ordered-buffer to cleanly handle inserting from both ends.  But this
isn't bad.  The only issue now is that, when inserting old messages, a
new timestamp header is always inserted, even if the timestamp
difference is within the delta setting, which is not necessarily a bad
thing, because it makes it easier to see where the newly fetched
messages are.

It gets a little complicated because, when inserting old messages, if
we insert from the top (which makes sense), we need to insert the
messages in a different order so the prefix-fn looks at the right
event to determine whether to insert a new header.
Since some clients include newlines between and before and after the
HTML tags, we need a more flexible solution, and esxml is that,
thankfully.
See element-hq/element-ios#2080

Not fully tested yet, so feel free to revert.
@alphapapa
Copy link
Owner Author

Master is this branch now. Finally!

@alphapapa alphapapa closed this Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants