Buffers panel: highlight current button/buffer

[MaxGyver83]

Description

While switch-buffer is very efficient for finding a specific buffer among many, it requires that you remember what to search for. Having a list of open buffers always visible (as p.e. in Firefox) helps to keep an overview. Nyxt's buffer panel is very similar to a tab bar in other browsers.

In this pull request, I try to make the buffer panel even a bit more useful.

Changes:

• ✔ Make the reload button stand out by painting it green and moving it above the header.
• ✔ Highlight the current buffer by painting its button blue (currently only updated when the buffer is changed using the buffer panel).
• ✔ Reload the buffer panel via window-set-buffer-hook.
• ✔ Reload the buffer panel via buffer-loaded-hook.

Before:

After:

Discussion

Please tell me if such a changed is welcome at all. If yes, you might prefer a different design. I actually want to use the default prompt-buffer highlight color instead of a simple blue but I couldn't manage to select this theme color. I also don't yet understand how to set a hook.

One detail: In line 100, I create either a regular or a blue :nbutton depending on the highlight argument. Actually, I would prefer to write less duplicate code but this does not work:

(:nbutton :text (title buffer)
                  :buffer panel-buffer
                  @(if highlight '(:style "background-color:blue;"))
                 ...

I don't know why. Probably easy to solve but I'm a Common Lisp beginner. → @aartaka showed me how to fix this.

Checklist:

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

• Git hygiene:
  • I have pulled from master before submitting this PR
  • There are no merge conflicts.
• 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).
    • Changelog update should be a separate commit.
  • I have added a migration.lisp entry for all compatibility-breaking changes.
  • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainers to do so.
• 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.)
  • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

[aartaka]

I like the idea! I've left some comments, mostly cosmetic and helping with Spinneret and our custom tags, like :nbutton.

[aartaka]

This might go into the body of the panel command as:

(reload-buffer panel-buffer)

So no need for a separate function that works out of context.

[MaxGyver83]

Thanks for your comments! Would this be a flet expression? This function is to be called by the window-set-buffer-hook. I guess I need a free-standing function for the hook!?

[aartaka]

Oh, right. I haven't taken the window hook into account. For that, a separate function might be necessary. But here, in buttons, you can benefit from the panel buffer accessibility.

[MaxGyver83]

I have added the window hook in commit bec09b2. Now I call reload-panel-buffer in the hook. Is it correct to call this top-level function also in the buffers panel? Or should I create a flet expression nevertheless?

[aartaka]

I have tried this. It works for `:class "action"` but not for `:class "success"`. Where is the according CSS defined?

It's buffer.lisp, line 167 on master.

[MaxGyver83]

So far, my best try to add reload-panel-buffer to the window-set-buffer-hook:

(defun buffers-panel-handler (window buffer)
  (reload-panel-buffer (first (nyxt::panel-buffers (current-window)))))

(define-configuration window
  ((window-set-buffer-hook
    (hooks:add-hook %slot-default% 'buffers-panel-handler))))

AFAIU, this warning lets the compilation fail:

Compilation warning: The variable WINDOW is defined but never used.

When I remove the function arguments window and buffer, I get a different error.

Update: I think I should use window instead of calling (current-window). And I don't get a compilation warning when I add a pointless (print buffer):

(defun buffers-panel-handler (window buffer)
  (print buffer)
  (reload-panel-buffer (first (nyxt::panel-buffers window))))

But this causes a crash on startup.

Update 2: Fixed in commit bec09b2.

[aartaka]

Compilation warning: The variable WINDOW is defined but never used

It's just a compiler being picky. Add (declare (ignore window)) to the handler right after argument list and it'll work.

[aartaka]

@aadcg, take a look. Most of my programming-related questions are resolved, so what's left is the feature assessment.

[aadcg]

Thanks @MaxGyver83. I've left some comments.

When all of conversations are closed I'll give this feature a try.

[aadcg]

I'll take a final look tomorrow and merge.

[aadcg]

@MaxGyver83, while this is a very welcomed feature, the PR doesn't seem ready.

When opening a new buffer, see how the panel buffer gets updated.

Steps to reproduce:

• (nyxt:start :urls '("https://en.wikipedia.org/wiki/Tomato") :failsafe t)
• Invoke command buffers-panel
• Invoke command set-url-new-buffer
• Notice the behavior above.

If we have a mechanism that updates the panel buffer automatically, then we can rid of the update button. Thanks!

[MaxGyver83]

Steps to reproduce:

* `(nyxt:start :urls '("https://en.wikipedia.org/wiki/Tomato") :failsafe t)`

* Invoke command `buffers-panel`

* Invoke command `set-url-new-buffer`

* Notice the behavior above.

True. It looks like my testing was too narrow. I have committed two changes:

• Print "Loading..." instead of a tiny empty button when the title of a new buffer is not yet known.
• I have added a buffer-loaded-hook. I think both hooks are necessary.

I'm still trying to add the window-set-buffer-hook directly in window.lisp in initialize-instance.

EDIT:
Another possible bug: When the buffers panel shows three buffers:

• Open a second window (which contains only the "new" buffer).
• Add a buffers panel. This buffers panel shows 4 buffers: "new" plus the 3 buffers of the first window.
• Switch to the first window: This buffers panel doesn't show yet the "new" buffer of window 2. Pressing the reload button makes the "new" buffer show up.

I'm wondering if it's correct at all that one window's buffers panel shows all windows' buffers. If this is intended, I might need a third hook.

[aadcg]

@MaxGyver83 I'm mostly happy with this PR. In terms of the UI, I'd suggest doing the change below.

                  :text-overflow ellipsis))
       (:body
-       (:nbutton :text "Update ↺"
-         :buffer panel-buffer
-         :class "success"
-         `(reload-panel-buffer ,panel-buffer))
        (:h1 "Buffers")
        (loop for buffer in (buffer-list)
-             collect (:raw (buffer-markup buffer (eq buffer (current-buffer)))))))))
+             collect (:raw (buffer-markup buffer (eq buffer (current-buffer)))))
+       (:nbutton :text "↺"
+         :buffer panel-buffer
+         :style "position: absolute; bottom: 10px; right: 10px;"
+         :class "success"
+         `(reload-panel-buffer ,panel-buffer))))))

Which results in:

---

I'm still trying to add the window-set-buffer-hook directly in window.lisp in initialize-instance.

I think the way you did is good enough for now.

Another possible bug: When the buffers panel shows three buffers (...) I'm wondering if it's correct at all that one window's buffers panel shows all windows' buffers. If this is intended, I might need a third hook.

Let's discuss that topic on a separate issue.

---

@MaxGyver83 would you like to make the changes yourself or shall I do it? Thanks.

[MaxGyver83]

@MaxGyver83 would you like to make the changes yourself or shall I do it? Thanks.

I have committed your suggested change, updated the changelog and rebased.

[aadcg]

@MaxGyver83 I've merged your work via commits f9361ad and 772c844.

Thanks for this important contribution!