bug#41741: [PATCH] Save project list as lisp data

bug#41741: [PATCH] Save project list as lisp data

[Dmitry Gutov]

Hi Simen,

On 06.06.2020 19:40, Simen Heggestøyl wrote:
> I'm attaching a suggested patch for changing project.el's project list
> format from a line based one to proper Lisp data as discussed in the
> "New feature in project.el: Remembering the previously used projects"
> thread on emacs-devel.

Some comments below.

> No metadata is added at this point, but it makes it extensible for the
> future.

I'm not sure we'd really be able to store metadata in it. The list is 
more or less transient: the entries are added automatically, and removed 
automatically as well. If someone needs to know something about a 
project, it's better to leave that to an overridable method, and maybe 
add a project-vc- variable (if that value needs to be user-customizable).

For that reason, we should probably opt for storing directories only.

> -  "Initialize `project--list' from the project list file."
> +  "Initialize `project--list' from the file `project-list-file'."

I think the previous string was okay.

> -          (when (file-exists-p filename)
> +          (when (file-readable-p filename)

What's the difference? File exists, but belongs to another account? I'm 
not sure we'd want to silently fail in that case (kinda puzzling 
behavior), and writing to the file is likely to fail later too.

> +              (car (read-from-string (buffer-string))))))))

AKA:

      (goto-char (point-min))
      (read (current-buffer))

Though the practical difference will be tiny.

bug#41741: [PATCH] Save project list as lisp data

[Simen Heggestøyl]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 06.06.2020 19:40, Simen Heggestøyl wrote:
>
>> No metadata is added at this point, but it makes it extensible for the
>> future.
>
> I'm not sure we'd really be able to store metadata in it. The list is
> more or less transient: the entries are added automatically, and
> removed automatically as well. If someone needs to know something
> about a project, it's better to leave that to an overridable method,
> and maybe add a project-vc- variable (if that value needs to be
> user-customizable).
>
> For that reason, we should probably opt for storing directories only.

It is transient now, yes. But the biggest gain from going from the
one-directory-per-line format to the proposed alist format is (in my
view) having the possibility to extend its uses in the future without
breaking backward compatibility with existing project list files.

Just as an example I can imagine an optional feature in the future where
you could name a project interactively the first time it's seen
(i.e. when it's added to the project list for the first time). I
wouldn't like to maintain that in my config file; having the name live
in the project list only would be fine with me. That would require
extending project.el to keep project metadata when rearranging them.

>> -  "Initialize `project--list' from the project list file."
>> +  "Initialize `project--list' from the file `project-list-file'."
>
> I think the previous string was okay.

Alright, reverted.

>> -          (when (file-exists-p filename)
>> +          (when (file-readable-p filename)
>
> What's the difference? File exists, but belongs to another account?
> I'm not sure we'd want to silently fail in that case (kinda puzzling
> behavior), and writing to the file is likely to fail later too.

Hm, yes. I snatched that part from saveplace.el, but after you pointed
it out I now also think it's better to get an error in that
case. Reverted.

>> +              (car (read-from-string (buffer-string))))))))
>
> AKA:
>
>      (goto-char (point-min))
>      (read (current-buffer))
>
> Though the practical difference will be tiny.

I don't mind changing it, but could you explain the difference to me? My
understanding isn't deep enough to see anything but one line versus
two. :)

-- Simen

bug#41741: [PATCH] Save project list as lisp data

[Dmitry Gutov]

On 07.06.2020 22:55, Simen Heggestøyl wrote:

 > It is transient now, yes.

And later, it won't be?

> Just as an example I can imagine an optional feature in the future where
> you could name a project interactively the first time it's seen
> (i.e. when it's added to the project list for the first time).

And then lose all these annotations when a project root directory is 
moved/renamed/etc?

But OK, maybe it won't happen too often to really worry about.

>>> -          (when (file-exists-p filename)
>>> +          (when (file-readable-p filename)
>>
>> What's the difference? File exists, but belongs to another account?
>> I'm not sure we'd want to silently fail in that case (kinda puzzling
>> behavior), and writing to the file is likely to fail later too.
> 
> Hm, yes. I snatched that part from saveplace.el, but after you pointed
> it out I now also think it's better to get an error in that
> case. Reverted.

Perhaps we should change saveplace as well. :-)

>>> +              (car (read-from-string (buffer-string))))))))
>>
>> AKA:
>>
>>       (goto-char (point-min))
>>       (read (current-buffer))
>>
>> Though the practical difference will be tiny.
> 
> I don't mind changing it, but could you explain the difference to me? My
> understanding isn't deep enough to see anything but one line versus
> two. :)

Just an extra string allocation, I guess.

bug#41741: [PATCH] Save project list as lisp data

[Simen Heggestøyl]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 07.06.2020 22:55, Simen Heggestøyl wrote:
>
>> It is transient now, yes.
>
> And later, it won't be?

Yes, later too (sorry, I think I misunderstood your original message).

>> Just as an example I can imagine an optional feature in the future where
>> you could name a project interactively the first time it's seen
>> (i.e. when it's added to the project list for the first time).
>
> And then lose all these annotations when a project root directory is
> moved/renamed/etc?
>
> But OK, maybe it won't happen too often to really worry about.

Yes. I think it should only be used to store metadata that isn't a
hassle to lose.

Another example from the top of my head: recording project visit
counts. That could be used for an option to show the most popular
projects first in the completion list, for instance.

>>> AKA:
>>>
>>>       (goto-char (point-min))
>>>       (read (current-buffer))
>>>
>>> Though the practical difference will be tiny.
>> I don't mind changing it, but could you explain the difference to
>> me? My
>> understanding isn't deep enough to see anything but one line versus
>> two. :)
>
> Just an extra string allocation, I guess.

Sure, I've changed it.

Updated patch attached!

-- Simen


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Save-project-list-as-lisp-data.patch --]
[-- Type: text/x-diff, Size: 2972 bytes --]

From 6a907d09b9a5b341f5c01f93fd031cb45229f320 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Fri, 5 Jun 2020 19:32:30 +0200
Subject: [PATCH] Save project list as lisp data

Save the project list file as lisp data instead of line separated
strings to make it more extendable in the future.

* lisp/progmodes/project.el (project--read-project-list)
(project--write-project-list, project--add-to-project-list-front)
(project--remove-from-project-list): Adjust to `project--list' now
being an alist.
---
 lisp/progmodes/project.el | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 4d57fb25fd..0cca518d26 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -763,13 +763,8 @@ project--read-project-list
           (when (file-exists-p filename)
             (with-temp-buffer
               (insert-file-contents filename)
-              (let ((dirs (split-string (buffer-string) "\n" t))
-                    (project-list '()))
-                (dolist (dir dirs)
-                  (cl-pushnew (file-name-as-directory dir)
-                              project-list
-                              :test #'equal))
-                (reverse project-list)))))))
+              (goto-char (point-min))
+              (read (current-buffer)))))))
 
 (defun project--ensure-read-project-list ()
   "Initialize `project--list' if it hasn't already been."
@@ -780,7 +775,8 @@ project--write-project-list
   "Persist `project--list' to the project list file."
   (let ((filename project-list-file))
     (with-temp-buffer
-      (insert (string-join project--list "\n"))
+      (insert ";;; -*- lisp-data -*-\n")
+      (pp project--list (current-buffer))
       (write-region nil nil filename nil 'silent))))
 
 (defun project--add-to-project-list-front (pr)
@@ -788,9 +784,9 @@ project--add-to-project-list-front
 Save the result to disk if the project list was changed."
   (project--ensure-read-project-list)
   (let ((dir (project-root pr)))
-    (unless (equal (car project--list) dir)
-      (setq project--list (delete dir project--list))
-      (push dir project--list)
+    (unless (equal (caar project--list) dir)
+      (setq project--list (assoc-delete-all dir project--list))
+      (push (list dir) project--list)
       (project--write-project-list))))
 
 (defun project--remove-from-project-list (pr-dir)
@@ -798,8 +794,8 @@ project--remove-from-project-list
 If the directory was in the list before the removal, save the
 result to disk."
   (project--ensure-read-project-list)
-  (when (member pr-dir project--list)
-    (setq project--list (delete pr-dir project--list))
+  (when (assoc pr-dir project--list)
+    (setq project--list (assoc-delete-all pr-dir project--list))
     (message "Project `%s' not found; removed from list" pr-dir)
     (project--write-project-list)))
 
-- 
2.26.2

bug#41741: [PATCH] Save project list as lisp data

[Dmitry Gutov]

On 08.06.2020 22:00, Simen Heggestøyl wrote:
> Another example from the top of my head: recording project visit
> counts. That could be used for an option to show the most popular
> projects first in the completion list, for instance.

OK then.

> Updated patch attached!

All good!

bug#41741: [PATCH] Save project list as lisp data

[Simen Heggestøyl]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 08.06.2020 22:00, Simen Heggestøyl wrote:
>
>> Updated patch attached!
>
> All good!

Installed! Thanks for reviewing.

-- Simen

bug#41741: [PATCH] Save project list as lisp data

[Juri Linkov]

> Another example from the top of my head: recording project visit
> counts. That could be used for an option to show the most popular
> projects first in the completion list, for instance.

Or record project visit time, and then sort by time,
so that the most recently used projects are first.

bug#41741: [PATCH] Save project list as lisp data

[Dmitry Gutov]

On 09.06.2020 02:09, Juri Linkov wrote:
> Or record project visit time, and then sort by time,
> so that the most recently used projects are first.

We currently do that by changing the order of the entries.

bug#41741: [PATCH] Save project list as lisp data

[Drew Adams]

> > Another example from the top of my head: recording project visit
> > counts. That could be used for an option to show the most popular
> > projects first in the completion list, for instance.
> 
> Or record project visit time, and then sort by time,
> so that the most recently used projects are first.

Haven't been following this thread; sorry.

But this is one of the things bookmarks are great for.
Whatever it is, bookmark it, and use the bookmark to
access it.  Each time you do, the bookmark updates
the date+time visited and the number of visits.

Nothing else is needed, just the ability to bookmark
something.  And you can do that for pretty much
anything you can visit.  If no existing bookmark type
is relevant you can easily define a new bookmark type.

bug#41741: [PATCH] Save project list as lisp data

[Simen Heggestøyl]

[-- Attachment #1: Type: text/plain, Size: 329 bytes --]

Hi.

I'm attaching a suggested patch for changing project.el's project list
format from a line based one to proper Lisp data as discussed in the
"New feature in project.el: Remembering the previously used projects"
thread on emacs-devel.

No metadata is added at this point, but it makes it extensible for the
future.

-- Simen


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Save-project-list-as-lisp-data.patch --]
[-- Type: text/x-diff, Size: 3258 bytes --]

From 3084f651d6c0f5e6b4b3ac699b59742f98af2248 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Fri, 5 Jun 2020 19:32:30 +0200
Subject: [PATCH] Save project list as lisp data

Save the project list file as lisp data instead of line separated
strings to make it more extendable in the future.

* lisp/progmodes/project.el (project--read-project-list)
(project--write-project-list, project--add-to-project-list-front)
(project--remove-from-project-list): Adjust to `project--list' now
being an alist.
---
 lisp/progmodes/project.el | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 4d57fb25fd..3b007bc8dc 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -757,19 +757,13 @@ project--list
   "List of known project directories.")
 
 (defun project--read-project-list ()
-  "Initialize `project--list' from the project list file."
+  "Initialize `project--list' from the file `project-list-file'."
   (let ((filename project-list-file))
     (setq project--list
-          (when (file-exists-p filename)
+          (when (file-readable-p filename)
             (with-temp-buffer
               (insert-file-contents filename)
-              (let ((dirs (split-string (buffer-string) "\n" t))
-                    (project-list '()))
-                (dolist (dir dirs)
-                  (cl-pushnew (file-name-as-directory dir)
-                              project-list
-                              :test #'equal))
-                (reverse project-list)))))))
+              (car (read-from-string (buffer-string))))))))
 
 (defun project--ensure-read-project-list ()
   "Initialize `project--list' if it hasn't already been."
@@ -780,7 +774,8 @@ project--write-project-list
   "Persist `project--list' to the project list file."
   (let ((filename project-list-file))
     (with-temp-buffer
-      (insert (string-join project--list "\n"))
+      (insert ";;; -*- lisp-data -*-\n")
+      (pp project--list (current-buffer))
       (write-region nil nil filename nil 'silent))))
 
 (defun project--add-to-project-list-front (pr)
@@ -788,9 +783,9 @@ project--add-to-project-list-front
 Save the result to disk if the project list was changed."
   (project--ensure-read-project-list)
   (let ((dir (project-root pr)))
-    (unless (equal (car project--list) dir)
-      (setq project--list (delete dir project--list))
-      (push dir project--list)
+    (unless (equal (caar project--list) dir)
+      (setq project--list (assoc-delete-all dir project--list))
+      (push (list dir) project--list)
       (project--write-project-list))))
 
 (defun project--remove-from-project-list (pr-dir)
@@ -798,8 +793,8 @@ project--remove-from-project-list
 If the directory was in the list before the removal, save the
 result to disk."
   (project--ensure-read-project-list)
-  (when (member pr-dir project--list)
-    (setq project--list (delete pr-dir project--list))
+  (when (assoc pr-dir project--list)
+    (setq project--list (assoc-delete-all pr-dir project--list))
     (message "Project `%s' not found; removed from list" pr-dir)
     (project--write-project-list)))
 
-- 
2.26.2