#6 Per application single-buffer mode

Open
jao wants to merge 2 commits from jao/navigel:single-buffer into master
jao commented 1 year ago
There is no content yet.
DamienCassou approved these changes 1 year ago
DamienCassou left a comment

I don't think I've spent enough time on your review to understand it and its consequences fully yet. Here is some early feedback anyway. Can you please take it into account and I will review again?

Anyway, I think I like what you did. Thank you.

navigel.el Outdated
(defun navigel--app-buffer (app)
"If APP is a single-buffer application, find or create its buffer."
(when (and app (navigel-single-buffer-app-p app))
(let ((buffer (cl-find-if (lambda (b)
DamienCassou commented 1 year ago

what about storing navigel buffers in a hashtable with the app as key? This would make it very fast to check if there is already a buffer and get it.

what about storing navigel buffers in a hashtable with the app as key? This would make it very fast to check if there is already a buffer and get it.
jao commented 1 year ago

yes, we can do that. one has to take care of updating the hash table on kill buffer, adding a hook i guess. i'll see to it.

yes, we can do that. one has to take care of updating the hash table on kill buffer, adding a hook i guess. i'll see to it.
DamienCassou commented 1 year ago

instead of updating the hashtable as soon as a buffer is killed, you could just check when the hashtable is read that the requested buffer hasn't been kiiled already. The code will be simpler.

An alternative is to cleanup the whole hashtable each time it is read. It's a bit costly as the code will have to iterate over every key for each read, but it's no big deal as there won't be more keys than the number of installed applications using navigel.

instead of updating the hashtable as soon as a buffer is killed, you could just check when the hashtable is read that the requested buffer hasn't been kiiled already. The code will be simpler. An alternative is to cleanup the whole hashtable each time it is read. It's a bit costly as the code will have to iterate over every key for each read, but it's no big deal as there won't be more keys than the number of installed applications using navigel.
navigel.el Outdated
(when (and app (navigel-single-buffer-app-p app))
(let ((buffer (cl-find-if (lambda (b)
(with-current-buffer b
(and (derived-mode-p 'navigel-tablist-mode)
DamienCassou commented 1 year ago

not all navigel buffers use navigel-tablist-mode. For example mpdel's song buffer doesn't.

not all navigel buffers use `navigel-tablist-mode`. For example mpdel's song buffer doesn't.
navigel.el Outdated
(cl-defgeneric navigel-single-buffer-name (app entity)
"Return a string representing ENTITY in the buffer's name, for single-buffer APP."
(let ((app (or app navigel-app 'navigel))
DamienCassou commented 1 year ago

in which cases the parameter app is nil? It seems weird to me.

in which cases the parameter `app` is nil? It seems weird to me.
jao commented 1 year ago

i was being overcatious. will be gone.

i was being overcatious. will be gone.
navigel.el Outdated
(cl-defgeneric navigel-single-buffer-name (app entity)
"Return a string representing ENTITY in the buffer's name, for single-buffer APP."
(let ((app (or app navigel-app 'navigel))
(entity-name (if entity (navigel-buffer-name entity) "empty")))
DamienCassou commented 1 year ago

in which cases the parameter entity is nil? If there are cases like that, I would expect (format "%s" app) instead of "%s - empty"

in which cases the parameter `entity` is nil? If there are cases like that, I would expect `(format "%s" app)` instead of `"%s - empty"`
navigel.el Outdated
(buffer (or app-buffer (navigel-entity-buffer entity))))
(with-current-buffer (get-buffer-create buffer)
(let ((state (when (not app-buffer) (navigel--save-state)))
(cached-state (when app-buffer (navigel--cached-state entity))))
DamienCassou commented 1 year ago

wouldn't the folowing be simpler?

(let ((state (if (navigel-single-buffer-app-p app)
                 (navigel--cached-state entity)
               (navigel--save-state))))
  ...)  
wouldn't the folowing be simpler? ```emacs (let ((state (if (navigel-single-buffer-app-p app) (navigel--cached-state entity) (navigel--save-state)))) ...) ```
jao commented 1 year ago

yes, and, actually, i've realized the code can be simplified in other ways.

yes, and, actually, i've realized the code can be simplified in other ways.
navigel.el Outdated
(buffer (get-buffer-create (navigel-entity-buffer entity))))
(let* ((app navigel-app)
(app-buffer (navigel--app-buffer app))
(buffer (get-buffer-create (or app-buffer
DamienCassou commented 1 year ago

could you please extract this code into a dedicated function? Something like (navigel-get-entity-buffer app entity) It is copy/pasted from navigel-refresh.

could you please extract this code into a dedicated function? Something like `(navigel-get-entity-buffer app entity)` It is copy/pasted from `navigel-refresh`.
jao commented 1 year ago
Poster

thanks for the review. i'll address all your comments above, and also fix a couple of glitches i've found these days using single-buffer for mpdel (maintaining the cache should be in list-children, and we can leave refresh mostly alone, except for finding the appropriate buffer).

thanks for the review. i'll address all your comments above, and also fix a couple of glitches i've found these days using single-buffer for mpdel (maintaining the cache should be in list-children, and we can leave refresh mostly alone, except for finding the appropriate buffer).
jao commented 1 year ago
Poster

i've just pushed a second version. it's not as concise as i had hoped for, but i think it's cleaner, and tries to follow your previous review. let me know what you think. my mpdel branch single-buffer uses this one, if you want to try (but glitches there could be mpdel's fault!).

i've just pushed a second version. it's not as concise as i had hoped for, but i think it's cleaner, and tries to follow your previous review. let me know what you think. my mpdel branch `single-buffer` uses this one, if you want to try (but glitches there could be mpdel's fault!).
jao commented 1 year ago
Poster

@DamienCassou please hold on, i've got more changes

@DamienCassou please hold on, i've got more changes
jao commented 1 year ago
Poster

okay, they say third's lucky: should be ready for a new review. this version has been working for me reasonably well with mpdel so far, and should address your previous comment.

okay, they say third's lucky: should be ready for a new review. this version has been working for me reasonably well with mpdel so far, and should address your previous comment.
DamienCassou approved these changes 1 year ago
DamienCassou left a comment

Some initial feedback. I'm still not done though.

Either a list of symbols denoting applications, t for all
applications or nil, the default, for none."
:type '(choice (const :tag "Never" nil)
DamienCassou commented 1 year ago

we are talking about "Applications using..." so I suggest "none" instead of "never".

we are talking about "Applications using..." so I suggest "none" instead of "never".
"Specify the application that was used to generate the buffer.")
(defvar navigel-single-buffers nil
"An alist of buffers displaying single-buffer apps.")
DamienCassou commented 1 year ago

please add to the docstring some info about the keys as well. Something like:

An alist of (APP . BUFFER) associating an application name to its buffer.
please add to the docstring some info about the keys as well. Something like: ```emacs An alist of (APP . BUFFER) associating an application name to its buffer. ```
navigel.el Outdated
overridden separately if necessary."
(format "%s" entity))
(cl-defgeneric navigel-entity-id (entity)
DamienCassou commented 1 year ago

can you please introduce that in a separate PR that we could quickly merge? I think navigel-equal could use this by default instead of equal.

can you please introduce that in a separate PR that we could quickly merge? I think `navigel-equal` could use this by default instead of `equal`.
jao commented 1 year ago

this function has a reason d'etre in this pr too, and a long docstring that doesn't make sense outside the pr... i'll do it if you prefer, though

this function has a reason d'etre in this pr too, and a long docstring that doesn't make sense outside the pr... i'll do it if you prefer, though
DamienCassou commented 1 year ago

ok, keep it there

ok, keep it there
(cl-defgeneric navigel-single-buffer-name (app entity)
"Return a string representing ENTITY in the buffer's name, for single-buffer APP."
(let ((app (or app navigel-app 'navigel))
DamienCassou commented 1 year ago

are there cases where app is nil? Where navigel-app is nil?

are there cases where `app` is nil? Where `navigel-app` is nil?
jao commented 1 year ago

i am not sure. using mpdel these days in single buffer mode, i've seen once something that resembled it, but that might be a problem on (my branch of) mpdel browser. all that said, the check for nil app, allows callers to use nil to signify "current-app", and the check for navigel-app allows using this function before the app has been set, so i'd say the checks don't hurt.

i am not sure. using mpdel these days in single buffer mode, i've seen once something that resembled it, but that might be a problem on (my branch of) mpdel browser. all that said, the check for nil app, allows callers to use nil to signify "current-app", and the check for navigel-app allows using this function before the app has been set, so i'd say the checks don't hurt.
DamienCassou commented 1 year ago

+1

+1
navigel.el Outdated
;;; Public functions
(defun navigel-single-buffer-app-p (app)
DamienCassou commented 1 year ago

I think this function could be made private with a double-dash.

I think this function could be made private with a double-dash.
jao commented 1 year ago

i use in mpdel, so i'd rather keep it public

i use in mpdel, so i'd rather keep it public
DamienCassou commented 1 year ago

ok

ok
(add-to-list 'navigel-single-buffer-apps app)))
(defun navigel-app-buffer (app)
"If APP is a single-buffer application, find its buffer."
DamienCassou commented 1 year ago

find => return

find => return
(defun navigel-app-buffer (app)
"If APP is a single-buffer application, find its buffer."
(navigel--app-buffer app t))
DamienCassou commented 1 year ago

I would inline the function navigel--app-buffer inside this one as I don't see the rationale for having 2 functions.

I would inline the function `navigel--app-buffer` inside this one as I don't see the rationale for having 2 functions.
jao commented 1 year ago

the optional argument, when the buffer is created, is for internal use only. the one without optional argument is public, for users of the library (again, i use it in mpdel), and i don't want to give the users the ability to mess things up by asking for the creation of the buffer (which is raw and needs further set up).

the optional argument, when the buffer is created, is for internal use only. the one without optional argument is public, for users of the library (again, i use it in mpdel), and i don't want to give the users the ability to mess things up by asking for the creation of the buffer (which is raw and needs further set up).
DamienCassou commented 1 year ago

it makes sense, thanks for the clarification

it makes sense, thanks for the clarification
DamienCassou reviewed 1 year ago
(defvar navigel-single-buffers nil
"An alist of buffers displaying single-buffer apps.")
(defvar-local navigel-state-cache nil
DamienCassou commented 1 year ago

I guess this variable should be made private

I guess this variable should be made private
jao commented 1 year ago

indeed

indeed
navigel.el Outdated
"An alist of buffers displaying single-buffer apps.")
(defvar-local navigel-state-cache nil
"Cache of entity states for single-buffer applications.")
DamienCassou commented 1 year ago

can you please describe some more what type of variable it is and what is stored in there?

can you please describe some more what type of variable it is and what is stored in there?
navigel.el Outdated
(tabulated-list-init-header)
(navigel-refresh
target
nil
DamienCassou commented 1 year ago

should the first parameter of navigel-refresh be deleted? It doesn't seem we use it anymore.

should the first parameter of `navigel-refresh` be deleted? It doesn't seem we use it anymore.
jao commented 1 year ago

it's a public function, so other users might be interested in it... in particular, i think mpdel uses it while refreshing the current song status.

it's a public function, so other users might be interested in it... in particular, i think mpdel uses it while refreshing the current song status.
navigel.el Outdated
(defun navigel--forget-single-buffer ()
"Remove the entry for the current buffer in `navigel-single-buffers."
(setf (alist-get navigel-app navigel-single-buffers nil t) nil))
DamienCassou commented 1 year ago

I think map-delete reveals intention better.

I think `map-delete` reveals intention better.
jao commented 1 year ago

it does. i didn't know about map-delete, thanks!

it does. i didn't know about map-delete, thanks!
navigel.el Outdated
not already exist."
(when (navigel-single-buffer-app-p app)
(let ((buffer (alist-get app navigel-single-buffers)))
(when (and (not (buffer-live-p buffer)) (not no-create))
DamienCassou commented 1 year ago
(unless (or (buffer-live-p buffer) no-create)

I would even extract the rest of the code in a dedicated function (e.g., navigel--single-app-buffer-create).

```emacs (unless (or (buffer-live-p buffer) no-create) ``` I would even extract the rest of the code in a dedicated function (e.g., `navigel--single-app-buffer-create`).
jao commented 1 year ago

makes sense

makes sense
(defun navigel--cache-state (entity)
"Save in the local cache the state of ENTITY, as displayed in the current buffer."
(let ((k (when entity (navigel-entity-id entity))))
DamienCassou commented 1 year ago

can you please rename k to something more explicit such as id?

can you please rename `k` to something more explicit such as `id`?
jao commented 1 year ago
Poster

@DamienCassou i've just pushed changes addressing your latest review (and commented when i didn't). Sorry for the delay!

@DamienCassou i've just pushed changes addressing your latest review (and commented when i didn't). Sorry for the delay!
Poster
Owner

Awesome. Please squash your 2 commits into one and merge it. Thank you!

Awesome. Please squash your 2 commits into one and merge it. Thank you!
All checks were successful
continuous-integration/drone/pr Build is passing
This pull request has changes conflicting with the target branch.
.gitignore
examples/navigel-ex-fs.el
navigel.el
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.