Per application single-buffer mode

There is no content yet.

DamienCassou approved these changes 3 years ago

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 3 years 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.

jao commented 3 years 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.

DamienCassou commented 3 years 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.

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 3 years ago

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 3 years ago

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

jao commented 3 years ago

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 3 years 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"

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 3 years ago

wouldn't the folowing be simpler?

(let ((state (if (navigel-single-buffer-app-p app)
                 (navigel--cached-state entity)
               (navigel--save-state))))
  ...)

jao commented 3 years ago

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 3 years 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.

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).

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!).

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

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 3 years ago

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

navigel.el

												
					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 3 years ago

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

navigel.el

												
					  "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 3 years 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.

navigel.el Outdated

												
					overridden separately if necessary."
				
					  (format "%s" entity))
				
					(cl-defgeneric navigel-entity-id (entity)

DamienCassou commented 3 years 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.

jao commented 3 years 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

DamienCassou commented 3 years ago

ok, keep it there

navigel.el

												
					(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 3 years ago

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

jao commented 3 years 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.

DamienCassou commented 3 years ago

navigel.el Outdated

												
					;;; Public functions
				
					(defun navigel-single-buffer-app-p (app)

DamienCassou commented 3 years ago

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

jao commented 3 years ago

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

DamienCassou commented 3 years ago

navigel.el

												
					      (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 3 years ago

find => return

navigel.el

												
					(defun navigel-app-buffer (app)
				
					  "If APP is a single-buffer application, find its buffer."
				
					  (navigel--app-buffer app t))

DamienCassou commented 3 years ago

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

jao commented 3 years 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).

DamienCassou commented 3 years ago

it makes sense, thanks for the clarification

DamienCassou reviewed 3 years ago

navigel.el

												
					(defvar navigel-single-buffers nil
				
					  "An alist of buffers displaying single-buffer apps.")
				
					(defvar-local navigel-state-cache nil

DamienCassou commented 3 years ago

I guess this variable should be made private

jao commented 3 years ago

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 3 years ago

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 3 years ago

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

jao commented 3 years 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.

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 3 years ago

I think map-delete reveals intention better.

jao commented 3 years ago

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 3 years 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).

jao commented 3 years ago

makes sense

navigel.el

												
					(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 3 years ago

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

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

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

This repo is archived. You cannot comment on pull requests.

No reviewers

No Label

No Milestone

No Assignees

2 Participants

Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.


			;;; Public functions

			(defun navigel-single-buffer-app-p (app)

#6 Per application single-buffer mode