Wip/fix 243 session saving #314
Conversation
alphapapa
added
bug
When disconnecting from a session, we now keep the slots necessary for reconnecting, and we persist those slots. Previously all of the session's data was discarded after writing, so if multiple accounts were disconnected but not at the same time, the first session to be disconnected was forgotten. Fixes #243. Reported-by: formula-spectre <https://github.com/formula-spectre>
alphapapa
force-pushed
the
wip/fix-243-session-saving
branch
from
edefcb5 to
df91863
Compare
|
Can do, but probably not before the end of next week, if that's ok. I've lisped a bit more than I should have today already, and the next few days aren't going to be an option. |
| ;; TODO: Should call this hook for each session with the session as argument. | ||
| (run-hooks 'ement-disconnect-hook) |
There was a problem hiding this comment.
The TODO sounds sensible. Alternatively, call it for each session with ement-session let-bound to that session? Then you wouldn't need to switch to an abnormal hook. Or keep that hook as-is and add a new abnormal hook to run per-session with the argument.
| (let* ((session (buffer-local-value 'ement-session buffer)) | ||
| (connectedp (and session (not (ement-session-has-synced-p session))))) | ||
| (unless (or connectedp (and any-connected-p (not session))) |
There was a problem hiding this comment.
Is the not here correct?:
(connectedp (and session (not (ement-session-has-synced-p session))))
A session which hasn't synced doesn't seem connected?
| (_ (let* ((ids (mapcar #'car sessions)) | ||
| (selected-id (completing-read prompt ids nil t))) | ||
| (alist-get selected-id sessions nil nil #'equal)))))))) |
There was a problem hiding this comment.
I think a lot of the ement-complete-session uses could reasonably be expected to act on the session for the current room buffer. How about we make that call to completing-read specify the id for ement-session as its DEFault argument?
We'd then want to make the prompt (format "%s [%s]: " prompt id) in the case when there was a default.
There was a problem hiding this comment.
I was also wondering whether there could be cases in practice where the user thought they were in multiple sessions, but in fact only one of them was connected, and the "don't prompt when only one session is connected" behaviour caused them to do something in the wrong session.
It might be reasonable to have a user option to say "prompt whenever there are multiple sessions, even if only one of them is connected".
@phil-s This is intended to fix #243. A few unrelated changes are in here, which I intend to split out before merging. But to fix the bug required more extensive changes than I...well, I kind of knew it would require a lot of minor changes, which is why I've put it off for so long.
Anyway, it seems to work now, tested in a clean config with multiple accounts. Would you mind giving the patch a quick visual inspection and letting me know if anything stands out as wrong? I'm not asking you to test it. :)