Global history polishing

[Ambrevar]

Once #1110 is merged, we will have the following points to address:

1. Replace some uses of open-urls with make-buffer. Then we can remove the parent-buffer argument from open-urls.
2. Prompt the user for the history file to restore on startup.
3. Delete session.lisp if there is nothing else to use in there.
4. Make sure history data access is thread safe (use locks properly with with-data-access macro and friends.)
5. Restore last current buffer on startup.
6. Restore history tree of dead buffers.
   This probably requires the storage of (new) nodes in a buffer slot. We could then implement a helper in htree to add all the children given by the nodes passed in argument.

• Can we inherit history-entry from entry?
  Without it, imported file is 6.4M.
  3.3M with deduplication fix.

  2.3M with obj deduplication fix.

[Ambrevar]

@aartaka Interested by some of the above tasks?

[aartaka]

1, 2, 4, 5 — definitely! Other ones — can't yet wrap my head around these. Maybe you'll be better at completing 3 and 6 :)

[Ambrevar]

6 I understand. Out of curiosity, what puzzles you about 3?

[jmercouris]

I would also like to add: replace binding class with struct.

[Ambrevar]

John Mercouris <[email protected]> writes:

I would also like to add: replace binding class with struct.

I tried to do it, but in the end it resulted in little benefits (for quite a few changes). Instead, I've unexported the binding class, which is what we want here I believe: a small structure that's just an implementation detail. At this point I find that the difference between structs and classes in Common Lisp is mostly due to some weird (historical) idiosyncrasies in the standard. `define-class` fully supersedes structs, and the `export` options give us the fine control we want. Thoughts?

[Ambrevar]

7\. (Optional, only after next pre-release) Remove `value` slot from `entry` class and force user to subclass `entry` instead. This involves quite a few changes to history-tree. Benefits: - One level of indirection less. - Easier to navigate the inspector :) - A serialized file that's 50% smaller. - Possibly faster serialization (I haven't done any benchmark though).

[aartaka]

6 I understand. Out of curiosity, what puzzles you about 3?

I don't understand who should delete that -- the user of the browser, and is it as easy as I think it is?

[jmercouris]

John Mercouris [email protected] writes:
I would also like to add: replace binding class with struct.
I tried to do it, but in the end it resulted in little benefits (for quite a few changes). Instead, I've unexported the binding class, which is what we want here I believe: a small structure that's just an implementation detail. At this point I find that the difference between structs and classes in Common Lisp is mostly due to some weird (historical) idiosyncrasies in the standard. define-class fully supersedes structs, and the export options give us the fine control we want. Thoughts?

I can agree with your reasoning! No problem!

[Ambrevar]

I don't understand who should delete that -- the user of the browser, and is it as easy as I think it is?

Once we add the feature to prompt for which history file to restore, the session feature will be fully superseded.
Let's have a final look at the session.lisp file and make sure there is no feature that we would be missing.

[aartaka]

Oh wait, I confused session.lisp for the data file $XDG_DATA_HOME/nyxt/session.lisp. Removing the source file doesn't sound good to me -- (re)store-session-by-name are useful commands, so they definitely need to stay.

EDIT: this -> the for less confusion.

[aartaka]

restore-session-by-name is actually what we used (and can use now) to implement 2.

[Ambrevar]

Oh wait, I confused `session.lisp` for the data file `$XDG_DATA_HOME/nyxt/session.lisp`. Removing this source file doesn't sound good to me -- `(re)store-session-by-name` are useful commands, so they definitely need to stay.

That's what I meant: we can adapt (re)store-session-by-name to fix 2., then we should be OK with deleting session.lisp.

[aartaka]

Then I'm in!

[Ambrevar]

Thanks! :)

[aartaka]

3 is done in 70aa8f1. The problem that one encounters is that, when using restore-history-by-name (new name for restore-session-by-name), Nyxt start constantly prompting "Restore session?" until user responds "No".

This stems from us prompting the user about session restoration from inside restore. This is not right, as restore should be a mere data restoration function (that's why initial GHT implementation added the restore-session-p flag to it -- to separate concerns and make data restoration more controllable) and it shouldn't prompt the user. I'm not sure where to put the prompting part, though -- there was a reason to remove it from startup function, wasn't there?

[Ambrevar]

The reason I moved the prompt to `restore` is because if left in make-startup-function, it would restore it twice! :p Now that I'm looking at the code again, maybe my head was too deep into the GHT swamp and I fixed this poorly: looks like we can replace this line ``` (get-user-data (data-profile buffer) (history-path buffer)) ``` from `make-startup-function` with the restore prompt. Wanna send a pull request?

[aartaka]

Wanna send a pull request?

Yes! Let me look into it, and I'll send a pull request in a week!

[aartaka]

open-urls is patched in 134f9f5 :)

[aartaka]

How do we ensure that history access is thread-safe? I want to tick this box (number 4 in the original comment), but I'm not sure it's a measurable task :)

[Ambrevar]

I don't remember the issue precisely. `with-data-access` uses a mutex. Maybe the problem is that there are places where we don't use `with-data-access` while we should. Do you remember what the problem is?

[Ambrevar]

Thanks for the `open-urls` patch!

[aartaka]

I don't remember the issue precisely. with-data-access uses a mutex. Maybe the problem is that there are places where we don't use with-data-access while we should.

I believe all such places are covered, especially with the introduction of with-data-unsafe -- it's now clearer where to lock the data.

Do you remember what the problem is?

Not really :) I guess it was just a not of caution to re-review things once more.

I've read through all the data-access places while implementing nosave-buffer, so it should be alright now.

[Ambrevar]

OK! Well, worse case if an issue arises in the future, we know where to look! I've been having issues with my GHT on my laptop, typically: - huge CPU bottleneck for a few seconds on startup; - maybe crashes or restoration errors. There may be a few bugs lurking around... I don't enough yet though.

[Ambrevar]

There are still issues that remain to be fixed with the GHT, but it's good enough for 2.0 I believe. I'll remove the 2.0 tag.

[aartaka]

nfiles should fix point 4 (thread-safety of the access) for good.

[Ambrevar]

Good catch!

[aadcg]

No longer important, due to #3598.