Scroll position is not restored

Ambrevar commented 3 years ago

Recipe:

open https://github.com/atlas-engineer/nyxt/issues
go to the bottom
kill the buffer
open https://github.com/atlas-engineer/nyxt/issues again.

Scroll position should be restored to the bottom, but it's not. @aartaka Am I missing something?

aartaka commented 3 years ago

Yes, I can reproduce. That is weird...

Ambrevar commented 3 years ago

Do you think you can fix it before tomorrow? Otherwise we can remove it from the ChangeLog.

@jmercouris Thoughts?

jmercouris commented 3 years ago

Let's remove it from the changelog for now. It's OK.

jmercouris commented 3 years ago

Better to fix some bugs, than none at all, and better to have out 2.1.0 :-)

Ambrevar commented 3 years ago

Can you take care of the changelog?

jmercouris commented 3 years ago

OK!

aartaka commented 3 years ago

What I ended up discovering is that we somewhy create new history nodes for every request. Basically, scrolling position is not restored, because the history-entry it was stored in gets replaced 0_o

aartaka commented 3 years ago

Here'a a raw REPL output showing this:

NYXT> (with-data-unsafe (history (history-path (current-buffer)))
        (scroll-position (htree:data (find (quri:uri "https://github.com/issues") (alexandria:hash-table-values (htree:entries history)) :test #'quri:uri= :key (alexandria:compose #'url #'htree:data)))))
(1000 0)
WARNING: Key was bound to HISTORY-FORWARDS
WARNING: Key was bound to RELOAD-WITH-MODES
<INFO> [18:21:41] Loading "https://github.com/issues".
<INFO> [18:21:43] Finished loading "https://github.com/issues".
NYXT> (with-data-unsafe (history (history-path (current-buffer)))
        (scroll-position (htree:data (find (quri:uri "https://github.com/issues") (alexandria:hash-table-values (htree:entries history)) :test #'quri:uri= :key (alexandria:compose #'url #'htree:data)))))
NIL
NYXT> (with-data-unsafe (history (history-path (current-buffer)))
        (scroll-position (htree:data (find (quri:uri "https://github.com/issues") (alexandria:hash-table-values (htree:entries history)) :test #'quri:uri= :key (alexandria:compose #'url #'htree:data)))))
NIL
NYXT> (with-data-unsafe (history (history-path (current-buffer)))
(htree:current-owner-node history))
#<HISTORY-TREE:NODE {100D401FC3}>
<INFO> [18:22:43] Pressed keys: C-x
<INFO> [18:22:44] Pressed keys: C-x
NYXT> (with-data-unsafe (history (history-path (current-buffer)))
(htree:current-owner-node history))
#<HISTORY-TREE:NODE {1010E8D6C3}>
NYXT> (with-data-unsafe (history (history-path (current-buffer)))
        (scroll-position (htree:data (find (quri:uri "https://github.com/issues") (alexandria:hash-table-values (htree:entries history)) :test #'quri:uri= :key (alexandria:compose #'url #'htree:data)))))
(1051 0)

aartaka commented 3 years ago

OK, so two history nodes are equalp (and thus don't get rewritten) only is every slot is equalp, which is not the case with dummy history-entry-es we create in history-add and the ones we store scroll-position in. This results in an automatic rewriting of nodes.

Should we use equals as a test of history-tree then? This way entries don't get rewritten unless there's a serious mismatch.

Just checked it out: All the history entries in my history file have implicit-visits . 1. This is a solid proof that we rewrite these.

jmercouris commented 3 years ago

Hm, that's a problem! I guess equal does not descend into the node, maybe we need our own equality method

aartaka commented 3 years ago

Hm, that's a problem! I guess equal does not descend into the node, maybe we need our own equality method

Wait, but we user equalp and that should descent :)

What I suggest is using equalS (a Nyxt-specific equality predicate) for history-tree (at least when used in Nyxt), so that entries are only compared by their URL.

EDIT: Change predicate only when using htree in Nyxt.

jmercouris commented 3 years ago

Indeed, equalp does descend, equal does not. I think we are on the same page here, I'm just miscommunicating a little!

So, we are in full agreement, equalS is what we need :-)

Ambrevar commented 3 years ago

The analysis was mistaken: the make-history-tree function makes sure the history entries are compared with history-tree-key, so the comparison is correct.

The issue is different, it's a design one. Since the beginning of htree, we would replace the entry data systematically, even on match. The intent was to ensure that the page title would get updated in history.

The obvious drawback is that the other history-entry slots get reset each time!

I'll send a pull request just now.

aartaka commented 3 years ago

Fair point!

Ambrevar commented 3 years ago

See #1533.

Ambrevar commented 2 years ago

According to https://github.com/atlas-engineer/nyxt/commit/6c0ca250574c86ebc0b05f758c21648fed0c99fe it's broken again. Let's investigate.

aadcg commented 1 year ago

I can reproduce.

aartaka commented 1 year ago

Let's actually remove the scroll position storage/API, because it never worked :/

Ambrevar commented 1 year ago

It's too bad, it shouldn't be too hard to get to work.

Does any WebKit-based browser support this?

aartaka commented 1 year ago

Epiphany has nothing about that, as far as I searched on their bug tracker.
Vimb has global marks, but these are user-invoked and not automatic. So no :(

We can have Vimb-like position marking, sounds nice. But, I guess, not much beyond that.

Is it safe to remove the scroll-position slot from the history-entry, or should it be augmented with some cl-prevalence magic?

Ambrevar commented 1 year ago

To remove a class slot we need to tell cl-prevalence to ignore the slot on deserialization, otherwise it will error.

aartaka commented 1 year ago

Any idea how to do that? cl-prevalence doesn't mention it anywhere :(

Ambrevar commented 1 year ago

Look for instance at blocker-mode:

(defmethod s-serialization:serializable-slots ((object blocker-mode))
  "Discard hostlists which can get pretty big."
  (delete 'nyxt/mode/blocker::hostlists
          (mapcar #'closer-mop:slot-definition-name
                  (closer-mop:class-slots (class-of object)))))

Ambrevar commented 1 year ago

Sorry, that's the wrong side of the problem. For deserialization, you want to specialize s-serialization:deserialize-class.

Untested:

(let* ((no-initarg-slots '())
         (instance (make-history-tree nil)))
    (dolist (slot-name+value no-initarg-slots)
     (unless (eq slot-name 'scroll-position)
      (let ((slot-name (first slot-name+value))
            (slot-val (rest slot-name+value)))
        (when (slot-exists-p instance slot-name)
          (let ((writer (first (getf (mopu:slot-properties class-symbol slot-name) :writers)))
                (value (deserialize-sexp-slot instance slot-name slot-val deserialized-objects)))
            (if writer
                (funcall (fdefinition writer) value instance)
                (setf (slot-value instance slot-name) value)))))))
    instance)

Note: Just realized that history-deserialize-sexp-internal is out-of-date with the latest cl-prevalence, and this

(:object (destructuring-bind (id &key class slots) (rest sexp)
                   (let ((object (s-serialization::deserialize-class class slots deserialized-objects)))
                     (setf (gethash id deserialized-objects) object)
                     (dolist (slot slots)
                       (when (slot-exists-p object (first slot))
                         (setf (slot-value object (first slot))
                               (history-deserialize-sexp-internal (rest slot) deserialized-objects))))
                     object)))

should be updated.

aadcg commented 7 months ago

Unlike #3325, this issue is related to restoring the scroll position from history buffers.

atlas-engineer / nyxt

Scroll position is not restored #1513