Reopen tab #365

4t0m · 2019-09-10T18:32:48Z

Please let me know if anything needs fixing. I wasn't totally sure about where to define things or what names to use.

For some reason, when I change the variables on my machine, something is failing to update somewhere and I am getting errors in slime because undefined variables are being invoked. I figure we can just change the name here when the code reaches an approvable state.

Fixes #253.

4t0m · 2019-09-10T20:48:15Z

Hmm. I'm currently working on bringing this up to date with master. Probably best to ignore this for now.

Ambrevar · 2019-09-10T21:01:14Z

Thanks so much for working on this, and welcome to the still very small circle of Next contributors! :)

I had a quick look and it's looking good! I'll go for a proper review after you've rebased.

I just had an idea: you could add a command which prompts the user with the minibuffer which URL to restore. Awesome, nah? :)

Also instead of storing just the URL, I recommend you store both the URL and the title so that it displays in the minibuffer just like for other buffers.

Ambrevar · 2019-09-10T21:05:36Z

Also as mentioned in #253, "buffer history" could be confused with the history of visited URLs for a given buffer. I suggest to use the term "recent buffers" instead, or something like that.

4t0m · 2019-09-10T21:24:55Z

Thanks for the notes! I'll change the variable shortly, and saving the title as well sounds like a good idea.

I was also thinking of doing something like what you describe with the minibuffer. In that case, should the buffers be removed from the ring (as they normally are)?

I don't think there's a neat way to remove things from the middle of the ring, but it's not as if there will be any performance issues as a result.

4t0m · 2019-09-10T22:23:55Z

Also instead of storing just the URL, I recommend you store both the URL and the title so that it displays in the minibuffer just like for other buffers.

Do you mean like how urls and titles are shown for the current buffer? I don't see where the minibuffer displays both title and url during selection.

I'm looking into what I'll have to do to display/return something other than a url string.

Ambrevar · 2019-09-11T07:43:24Z

Are you using 1.3.1 or master? Then in `M-x switch-buffer` you should see URL and titles. To customize the display of candidates, you need to create a class, say `dead-buffer`, and customize the object-string method. For the ring deletion, I suggest the following: - Create a ring-delete ((ring ring) index) procedure that removes element at index INDEX. If the INDEX is closer to HEAD-INDEX then the tail, shift all recent elements to the "right". Watch for the wrap around. If the INDEX is closer to the tail, shift all older elements to the "left". This should be the fastest. - The ring-pop procedure can then just call to ring-delete.

4t0m · 2019-09-11T22:44:08Z

Ah yes, it seems like I was looking at an older version. I see it now.

Do you mean shifting left/right by setting the value of the array at index for each element? Would it be faster to create a new ring with the starting array of the concatenation of all elements before the deleted value and all elements after the deleted value¹? Or set :items to such an array?

Another thing: How does one close the minibuffer? Right now it's remaining open after reopening a buffer, and I can't see what I'm doing differently from delete-buffer (which seems to close the minibuffer after running).

And finally: should adding the recent-buffer to the ring be part of rpc-delete as it currently is, or is it better as a hook?

Thanks!

¹ With an extra nil element in one of the arrays to preserve the total length, I guess?

4t0m · 2019-09-11T23:07:06Z

Okay it doesn't seem like concatenating is any better. I'll do as you described!

4t0m · 2019-09-11T23:50:22Z

Hmm, it seems kind of tedious to delete a buffer from the ring when there are duplicates, since I don't know how to determine the index. Is there a way for me to get the minibuffer to return the index of the selected item (the index in the original list passed to fuzzy-match)? Or maybe there's a better way.

Also, is this worth it? :P

libraries/ring/ring.lisp

Ambrevar · 2019-09-12T08:38:23Z

libraries/ring/package.lisp

+   #:make
+   #:pop-most-recent
+   #:item-count))
+


Spurious new line?

Ambrevar · 2019-09-12T08:39:14Z

next.asd

@@ -50,6 +50,7 @@
                 (:file "window")
                 (:file "minibuffer")
                 (:file "keymap")
+                 (:file "recent-buffer")


What about "recent-buffers" with an "s"?

Ambrevar · 2019-09-12T08:39:25Z

source/minibuffer.lisp

@@ -603,6 +603,7 @@ Return most recent entry in RING."
      (ring:insert ring clipboard-content)))
  (string (ring:ref ring 0)))

+


Spurious new line?

Should be fixed.

Ambrevar · 2019-09-12T08:40:02Z

source/recent-buffer.lisp

@@ -0,0 +1,37 @@
+;;; recent-buffer.lisp --- lisp subroutines for creating and managing recent buffers


Shorter: "Manage list of recent buffers"

Ambrevar · 2019-09-12T08:45:48Z

source/remote.lisp

+  (setf (recent-buffers interface)
+        (ring:make :size (recent-buffers-length interface))))
+        ;; (make-instance 'ring :items (make-array (recent-buffers-length interface)
+                                                ;; :initial-element nil))))


What about removing this initialize-instance, removing recent-buffers-length and setting the initform of recent-buffers:

:initform (ring:make :size 50)

?

In this case, people would set a custom length by putting something like the following in their init.lisp?

(setf (get-default 'remote-interface 'recent-buffersl) (ring:make :size n)

Yes, this is how we used to do it, but I want to deprecate (setf (get-default ...)).
Instead we can set it in after-init-hook (or any appropriate hook).

Also see #381

Ambrevar · 2019-09-12T08:46:21Z

source/remote.lisp

@@ -243,6 +243,10 @@ by Next when the user session dbus instance is not available.")
   (minibuffer-generic-history :accessor minibuffer-generic-history :initform (ring:make))
   (minibuffer-search-history :accessor minibuffer-search-history :initform (ring:make))
   (minibuffer-set-url-history :accessor minibuffer-set-url-history :initform (ring:make))
+   (recent-buffers-length :accessor recent-buffers-length :initform 50
+                          :documentation "The maximum length of the recent-buffers.")
+   (recent-buffers :accessor recent-buffers


You should always set an :initform (unless you know what you are doing) to prevent "unbound slot" errors.

I think fixed.

Ambrevar · 2019-09-12T08:46:45Z

source/remote.lisp

+        ;; (make-instance 'ring :items (make-array (recent-buffers-length interface)
+                                                ;; :initial-element nil))))
+
+(defmethod add-buffer-to-recent-buffers-ring ((interface remote-interface) (buffer buffer))


What about the shorter add-to-recent-buffers?

Ambrevar · 2019-09-12T08:47:15Z

source/remote.lisp

+                                                ;; :initial-element nil))))
+
+(defmethod add-buffer-to-recent-buffers-ring ((interface remote-interface) (buffer buffer))
+  "Add the url of a given buffer to the buffers ring."


url -> URL
"to the `recent-buffers' ring"

Ambrevar · 2019-09-12T08:49:57Z

source/recent-buffer.lisp

+  ((name :accessor name :initarg :name
+        :initform nil)
+   (title :accessor title :initarg :title
+          :initform nil)))


We are trying to increase typing in Next, so could you specify the slot types here?
:type string should do :)

I think fixed.

Ambrevar · 2019-09-12T09:02:46Z

Closing the minibuffer: Hmm this should not be a problem. I'll do more testing later.
Should we use hooks? Maybe, but I think this will be part of a bigger refactoring process around hooks later (I'm thinking of using hooks to customize everything). I suggest we leave it as it is for now.
Delete duplicates: I think it's a good idea to delete them, because duplicates serve no purpose here and their presence shortens the list of meaninful "dead buffers".

An easy way to do it would be

(delete-duplicates my-sequence
                   :test (lambda (rb1 rb2)
                           (and (string= (name rb1) (name rb2))
                                (string= (title rb1) (title rb2)))))

Or maybe less costly, you would only delete the elements equal to the one you are adding in add-to-recent-buffers.

Ambrevar · 2019-09-12T12:30:18Z

Regarding the minibuffer not closing: it's now fixed on master.

Ambrevar · 2019-09-12T13:33:41Z

Is `plusp` more readable than `(< 0 ...)`?

jmercouris · 2019-09-12T13:56:28Z

Is plusp more readable than (< 0 ...)?

plusp is not more readable, it is however more specific and should be used instead of the magic number of 0, it conveys a more specific meaning (like using when vs unless, etc).

…reopen-tab

…ng:delete-index.

… to recent-buffers.

4t0m · 2019-09-18T20:12:26Z

Okay, I think it's working and ~complete.

One thing I wasn't sure how to handle was echo-ing to the minibuffer. The following doesn't seem to be working for me.

(define-command undo-buffer-deletion ()
  "Open a new buffer with the URL of the most recently deleted buffer."

  (if (plusp (ring:item-count (recent-buffers *interface*)))
      (make-buffers (list (name (ring:pop-most-recent (recent-buffers *interface*)))))
      (echo "There are no recently-deleted buffers.")))

Also, should we add some keybindings?
Maybe 'u' and 'U' for VI-MODE? (And something else for normal mode?)

4t0m · 2019-09-18T20:19:51Z

libraries/ring/ring.lisp

@@ -17,6 +17,7 @@

 (defmethod index ((ring ring) index)
  "Return index converted to internal ring index, where items are ordered from newest to oldest."
+  ;; TODO: shouldn't that read "oldest to newest", since index 0 is the oldest?


To clarify, index 0 in the internal array is the oldest item, so this line is confusing to me.

The index method returns a "virtual" index. The internal index 0 is not the oldest item is there has been a wraparound.

For instance, in a ring of size 3,

in [a b]: a at index 0 is indeed the oldest element.

in [a b c], you add d, then you get [d b c]: there d at index 0 is the newest element.

Ambrevar · 2019-09-19T07:28:22Z

libraries/ring/ring.lisp

@@ -29,7 +30,7 @@ Return NEW-ITEM."
                     (mod (+ (head-index ring) (item-count ring))
                          (size ring)))
               new-item)
-    (if (= (item-count ring) (length (items ring)))
+    (if (= (item-count ring) (size ring))


Good catch! ;)

Ambrevar · 2019-09-19T07:30:45Z

libraries/ring/ring.lisp

+                       (mod index (size ring)))
+                 (aref (items ring)
+                       (mod (1+ index) (size ring)))))
+  (setf (aref (items ring) (mod end (size ring))) nil))


Could use something like (replace (items ring) (items ring) :start2 1).

I'm having sort of a hard time visualizing how this handles wrap arounds. Would you need to replace once from start to end, once from 0 to start, and then separately set element that wraps around?

Good point, this might not be worth the complexity of having to deal with the internal index.
I'll think about it.

Ambrevar · 2019-09-19T07:30:58Z

libraries/ring/ring.lisp

+                       (mod index (size ring)))
+                 (aref (items ring)
+                       (mod (1- index) (size ring)))))
+  (setf (aref (items ring) (mod end (size ring))) nil))


Same as above, with :start1.

Ambrevar · 2019-09-19T07:31:46Z

libraries/ring/ring.lisp

+  (setf (aref (items ring) (mod end (size ring))) nil))
+
+(defmethod delete-index ((ring ring) index)
+  "Delete the item at (internal oldest-first) index and move items to fill the space."


Hmmm, I think it's confusing that this is the only function where index is the internal index. Why not using the ring virtual index instead?

No big deal though, we might merge the ring library into cl-containers later.

How about this current version?

(defmethod external-index ((ring ring) internal-index) (1- (+ (- (head-index ring) internal-index) (item-count ring)))) (defmethod delete-index ((ring ring) index) "Delete the item at external index and move items to fill the space." (let* ((internal-index (index ring index)) (deleted-item (aref (items ring) (mod internal-index (size ring)))) (head (head-index ring)) (midpoint (mod (floor (+ head (item-count ring)) 2) (size ring)))) (if (< midpoint internal-index) (shift-left ring internal-index (+ head (item-count ring))) (progn (shift-right ring internal-index head) (setf (head-index ring) (mod (1+ (head-index ring)) (size ring))))) (decf (item-count ring)) deleted-item)) (defmethod delete-match ((ring ring) match-test) (let ((match-index (position-if match-test (items ring)))) (when match-index (delete-index ring (external-index ring match-index)))))

Hmm... I'm afraid external-index might add extra confusion to the library. I think we should try to keep it to just one "index" method and keep the "internal index" within that method only.
No problem, I'll merge and tweak the code until I find a nice and simple way to put this together.

Another idea: Maybe we should run `position-if' on the output of ring:recent-list, and then delete the index that returns?

Ambrevar · 2019-09-19T07:36:01Z

Good job! It's mostly ready for merging, the only nit is if you could try using the replace function instead, that could make the code significantly shorter.

Regarding the issue with echo: Don't worry about it, this is a known issue, it will be solved when we implement a status buffer and a Messages buffer. Stay tuned ;)

Ambrevar · 2019-09-19T16:20:36Z

Let me know when you grow tired of working on this issue, I can complete the finishing touches if need be, no problem! :)

…-tab

4t0m · 2019-09-19T18:47:21Z

Please feel free to make any edits/finishing touches. This is my first time using common lisp so I've been enjoying learning how it all works, but I am also eager to look at other issues.

Ambrevar · 2019-09-19T19:25:00Z

That's an option :)

Ambrevar · 2019-09-21T09:20:11Z

I've rebased and merged in befea72.
Thanks for this great and very useful contribution!

A small tip to ease the burden or merging: when catching up with master, dont "merge", but "rebase against master" instead. This way all your commits will be packed together instead of being spread around merge branches.

Also I noticed that you'd used different authors. I've picked one which does not seem to be recognized by GitHub, sorry :p

Last but not least: Would you like to be included in the list of contributors in M-x about?

4t0m · 2019-09-24T15:59:37Z

Awesome :). I'll rebase in the future and hopefully figure out what's going on with the authorship thing.

I'd love to be included on the list of contributors.

Ambrevar · 2019-09-25T13:18:00Z

Done in commit 1df811d.

4t0m added 5 commits September 5, 2019 17:58

successfully adds the urls of deleted buffers to history ring

c5b9924

allow customization of buffer-history-length

64cce29

enable the restoration of deleted tabs

4034ad6

remove some unnecessary comments

deb7607

fixing merge conflict

a202b06

Ambrevar mentioned this pull request Sep 10, 2019

Infinite buffer count: automatic webview unloading/reloading #366

Open

4t0m added 2 commits September 10, 2019 17:52

update variable names; move undo-buffer-deletion command

46f41a9

reopen deleted buffers through minibuffer fuzzy search

2d9c4c8

4t0m added 3 commits September 11, 2019 17:27

display name and title of recent buffers in minibuffer

95a5438

fix merge conflicts

a088c7f

actually fix merge conflicts

1059f3c

Ambrevar reviewed Sep 12, 2019

View reviewed changes

4t0m added 4 commits September 17, 2019 20:50

add deletion method to ring library

ae75c08

Merge branch 'master' of https://github.com/atlas-engineer/next into …

ec180f0

…reopen-tab

Reopening last-killed buffer and selected buffer functional. Added ri…

4f625df

…ng:delete-index.

add 'delete-match' method to ring and 'buffer-match-predicate' method…

6b59c1d

… to recent-buffers.

Remove newline

eac2046

4t0m commented Sep 18, 2019

View reviewed changes

4t0m requested a review from Ambrevar September 18, 2019 20:20

Ambrevar reviewed Sep 19, 2019

View reviewed changes

4t0m added 2 commits September 19, 2019 14:39

change delete-index to take external index as input

fcc92c9

Merge branch 'reopen-tab' of https://github.com/4t0m/next into reopen…

c6bcff8

…-tab

remove external index and use internal index in delete-match

0d98449

Ambrevar closed this Sep 21, 2019

		@@ -603,6 +603,7 @@ Return most recent entry in RING."
		(ring:insert ring clipboard-content)))
		(string (ring:ref ring 0)))

		@@ -0,0 +1,37 @@
		;;; recent-buffer.lisp --- lisp subroutines for creating and managing recent buffers

Reopen tab #365

Reopen tab #365

Conversation

4t0m commented Sep 10, 2019

4t0m commented Sep 10, 2019

Ambrevar commented Sep 10, 2019

Ambrevar commented Sep 10, 2019

4t0m commented Sep 10, 2019

4t0m commented Sep 10, 2019

Ambrevar commented Sep 11, 2019 via email

4t0m commented Sep 11, 2019 • edited Loading

4t0m commented Sep 11, 2019

4t0m commented Sep 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ambrevar commented Sep 12, 2019

Ambrevar commented Sep 12, 2019

Ambrevar commented Sep 12, 2019 via email

jmercouris commented Sep 12, 2019

4t0m commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ambrevar commented Sep 19, 2019

Ambrevar commented Sep 19, 2019

4t0m commented Sep 19, 2019

Ambrevar commented Sep 19, 2019 via email

Ambrevar commented Sep 21, 2019 • edited Loading

4t0m commented Sep 24, 2019

Ambrevar commented Sep 25, 2019 via email

4t0m commented Sep 11, 2019 •

edited

Loading

Ambrevar commented Sep 21, 2019 •

edited

Loading