WIP: Persist buffer state to history (for modes, tags and more)

[Ambrevar]

Description

This persists buffer modes (and more buffer data) in history. This is useful so that on restart, modes are restored (which can be critical e.g. for privacy).

Fixes #2064 and #2583 (not yet).
Fixes #2036.

How to test

• Make sure your cl-prevalence is up-to-date.

Discussion

• Is the design sane?
  In my opinion, yes, as it makes only minimal modifications to the Htree library, while adding a Nyxt core class that's easily extensible to add more buffer-related data to persist in the future.

• Can we better serialize and deserialize URIs and timestamp? The problem is that until now we used to serialize them as strings, but then how can Nyxt know which string is a URI, a timestamp, or just a string?
  An option would be to use reader macros, for instance (local-time:enable-read-macros). But that would break history backward compatibility. Thoughts?

  • Another suggestion by @aartaka: Introduce uri-sepcifier and timestamp-specifier which accept strings, then write accessors to automatically replace the string with the structured version. With a bit of MOP we could derive a general solution.
  • Better option: coerce-slot, which automatically coerce the value to the desired type.

To do:

• Replace current dead buffers with serialized buffers. Benefit: less risk for memory leaks.
• Squash commits since this PR commit history is a mess.
• Update cl-prevalence in submodules and Guix once patch is merged upstream.
• Tests!
• Add a tags slot and persist it.
• Add tag-related commands. See Buffer IDs do not persist #2583 (comment)
• Fix auto-rules which prevents any good use of this PR:
  • Currently auto-rules remove modes from the list of disabling them.
  • Reloading a page removes the manually enabled modes. Example:
    • Go to https://en.wikipedia.org
    • Enable no-script-mode.
    • Reload: BOOM! no-script-mode is gone.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

• I have pulled from master before submitting this PR
• There are no merge conflicts.
• I've added the new dependencies as:
  • ASDF dependencies,
  • Git submodules,

    cd /path/to/nyxt/checkout
    git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser

  • and Guix dependencies.
• My code follows the style guidelines for Common Lisp code. See:
  • Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF)
  • Google Common Lisp Style Guide
• I have performed a self-review of my own code.
• My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
• Documentation:
  • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
  • I have updated the existing documentation to match my changes.
  • I have commented my code in hard-to-understand areas.
  • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
  • I have added a migration.lisp entry for all compatibility-breaking changes.
• Compilation and tests:
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works. (If possible.)
  • New and existing unit tests pass locally with my changes.

[aadcg]

@Ambrevar Could you please re-word the discussion section? It seems off.

[aadcg]

Could you please explain the concept of a tag in this context?

[aadcg]

I'm a bit confused by this. Could you explain?

[aadcg]

To be more precise: htree:data can be whatever, so it feels a bit arbitrary to use it to restore the modes that were active in a buffer.

[aartaka]

htree:data is an accessor for the arbitrarily-typed payload inside the owner/node. This is a design decision that we made to not tie ourselves to any particular design of histoy nodes and to make htree more reusable.

[Ambrevar]

@Ambrevar Could you please re-word the discussion section? It seems off.

Done.

Could you please explain the concept of a tag in this context?

A buffer tag is an arbitrary string (or keyword?). Users can then filter / categorize buffers.
See #2583 for a discussion.

[Ambrevar]

Thought more about it: manually serializing modes and tags might be a bit shortsighted.

Instead, we should probably add a s-serialization method for buffers, which could work independently from history altogether.

For instance, create buffer B, serialize it with the new method, kill the buffer, then deserialize the serialized content to recreate the buffer.

[aadcg]

That sounds better to me @Ambrevar!

[Ambrevar]

By the way, this would address the long-standing issue of how to abstract "dead buffers". Two (or three?) birds with one stone.

[aartaka]

Yes, serializing buffers to history was the initial solution I thought of for buffer tagging :D

[Ambrevar]

@aartaka It seems that auto-rules are broken. For instance reloading a page disables the manually enabled modes, which is wrong. (See the original post for an example).

Can you have a look?

[Ambrevar]

Good news! I'm able to serialize and restore buffers with all their modes!

[aartaka]

@aartaka It seems that auto-rules are broken. For instance reloading a page disables the manually enabled modes, which is wrong. (See the original post for an example).

Ahhhhhh, I might know the reason—I've removed the handler from (en|dis)able-mode-hook, and we lost the ability to preserve the most recent set of modes. I know how to fix it and will fix it tomorrow!

[aartaka]

@Ambrevar, toggled modes preservation fixed in 1648300!

[Ambrevar]

Thanks!

[Ambrevar]

Before going further we need to merge #2654 and 40ants/cl-prevalence#22.

[Ambrevar]

I've rebased and squashed.

Prevalance still needs a fix: 40ants/cl-prevalence#25

Then there are a few more issues to iron out with this patch (like internal page URLs failing for some reason), but we are close!

[aartaka]

Internal page URLs may fail because they are being loaded before the context has all the schemes initialized. But that's a wild guess :)

[Ambrevar]

I'm working on the problem that URLs / timestamps are serialized as strings and thus need some manual code to be deserialized properly. Of course the manual code is easy to write, the problem is that it needs to be set in many places which is cumbersome and error prone.

The following seems to work to ensure that buffer's url slot is always a quri:uri:

(defmethod (setf slot-value-using-class) :around (new-value
                                                  (class user-class)
                                                  (instance buffer)
                                                  slot)
  (if (eq 'url (c2mop:slot-definition-name slot))
      (call-next-method (ensure-url new-value) class instance slot)
      (call-next-method new-value class instance slot)))

Still, it needs to be done for all classes + it's verbose. Can we do better?

I'm thinking of a few options:

1. Add a :type-coercion slot option that does the above by calling the value (a function) on the slot value and assigning the slot to the result. Example

(define-class ...
  ((foo "blah"
    :type string
    :type-coercion #'princ-to-string)
    ...))

Way less verbose, but still needs to be done for all slots.

2. Add a class option that does just the same for some types.
3. Add a global option / method that's called by user classes. Then maybe this would do (untested):

(defmethod (setf slot-value-using-class) :around (new-value
                                                  (class user-class)
                                                  instance
                                                  slot)
  (typecase (c2mop:slot-definition-type slot)
    ((quri:uri)
     (call-next-method (ensure-url new-value) class instance slot))
    (t
     (call-next-method new-value class instance slot))))

The drawback is that this is global, so if we want a bit more flexibility for a specific slot, we would need to override the above method for the concerned given class. Or... just use a more flexible slot type. So maybe that's not much of a drawback after all.

Thoughts?

EDIT: Clarifications.

[aartaka]

I'm working on the problem that URLs / timestamps are serialized as strings

But why not override cl-prevalence serialization for these?

[Ambrevar]

But why not override cl-prevalence serialization for these?

I guess you meant DEserialization.

We cannot do it based on the type, because the type is string, and we obviously cannot deserialize all strings to quri:uri or local-time:timestamp.

So we would have to do it for every quri:uri / timestamp slot of every class. Which is what this pull request does. It's cumbersome and error prone since it's easy to forget some slots. Plus it's an extra burden for extension writers / user configuration.

[Ambrevar]

See ed72ca7 for the new coerce-slot generic function. MOP shining again! :)

This makes everything much simpler in my opinion. Plus now it succeeds while previously it would error on internal buffers. Only previous-url had to be tweaked manually because it is of type (or null quri:uri).

What do you people think?

It seems to be working and I'm able to restart the browser and restore buffers with their modes.

There are still some rough edges, in particular with auto-rules. Need to work on these.

In the mean time, feedback welcome!

[Ambrevar]

Before this can be tested properly #2895 must be addressed.

[jmercouris]

Please reopen when ready.