url-history.el: variable url-history-track defined but not used

url-history.el: variable url-history-track defined but not used

[Klaus Straubinger]

In url-history.el, the customizable variable url-history-track is
defined. However, it is not used anywhere.

My suggestion is to take its value into account in the functions
url-history-update-url and url-history-save-history. For performance
reasons, it could be advisable to run url-history-timer only when
url-history-track is t.

-- 
Klaus Straubinger

Re: url-history.el: variable url-history-track defined but not used

[Klaus Straubinger]

> In url-history.el, the customizable variable url-history-track is
> defined. However, it is not used anywhere.

The following patch changes this:

--- url-history.el.orig	2005-07-04 14:17:32.000000000 +0200
+++ url-history.el	2005-11-28 16:05:03.000000000 +0100
@@ -37,11 +37,20 @@
   :group 'url)
 
 (defcustom url-history-track nil
-  "*Controls whether to keep a list of all the URLS being visited.
-If non-nil, url will keep track of all the URLS visited.
+  "*Controls whether to keep a list of all the URLs being visited.
+If non-nil, the URL package will keep track of all the URLs visited.
 If set to t, then the list is saved to disk at the end of each Emacs
 session."
-  :type 'boolean
+  :set #'(lambda (var val)
+	   (set-default var val)
+	   (and (featurep 'url)
+		(fboundp #'url-history-setup-save-timer)
+		(let ((def (symbol-function 'url-history-setup-save-timer)))
+		  (not (and (listp def) (eq 'autoload (car def)))))
+		(url-history-setup-save-timer)))
+  :type '(choice (const :tag "off" nil)
+		 (const :tag "on" t)
+		 (const :tag "within session" 'session))
   :group 'url-history)
 
 (defcustom url-history-file nil
@@ -88,7 +97,7 @@
     (cond ((fboundp 'cancel-timer) (cancel-timer url-history-timer))
 	  ((fboundp 'delete-itimer) (delete-itimer url-history-timer))))
   (setq url-history-timer nil)
-  (if url-history-save-interval
+  (if (and (eq url-history-track t) url-history-save-interval)
       (setq url-history-timer
 	    (cond
 	     ((fboundp 'run-at-time)

--- url.el.orig	2005-10-21 09:22:28.000000000 +0200
+++ url.el	2005-11-28 16:02:57.000000000 +0100
@@ -148,7 +148,8 @@
       (if buffer
 	  (with-current-buffer buffer
 	    (apply callback cbargs))))
-    (url-history-update-url url (current-time))
+    (if url-history-track
+	(url-history-update-url url (current-time)))
     buffer))
 
 (defun url-retrieve-synchronously (url)

-- 
Klaus Straubinger

Re: url-history.el: variable url-history-track defined but not used

[Richard M. Stallman]

    +	   (and (featurep 'url)
    +		(fboundp #'url-history-setup-save-timer)

Those tests are rather undesirable; it would be better for
this to work regardless of what else is loaded.

What is the reason for those tests?  What problem do they solve?
Let's look for another solution.

Re: url-history.el: variable url-history-track defined but not used

[Klaus Straubinger]

>    +	   (and (featurep 'url)
>    +		(fboundp #'url-history-setup-save-timer)
>
> Those tests are rather undesirable; it would be better for
> this to work regardless of what else is loaded.

Now that I come to think about it, in my opinion the whole of the
:set function could be reduced to

  :set #'(lambda (var val)
	   (set-default var val)
           (if (bound-and-true-p url-setup-done)
	       (url-history-setup-save-timer)))

because the test

			(fboundp 'url-history-setup-save-timer)
                        (let ((def (symbol-function
                                    'url-history-setup-save-timer)))
                          (not (and (listp def) (eq 'autoload (car def)))))

seems to only test if url-history-setup-save-timer is really available.

> What is the reason for those tests?

I suspect the variable url-history-save-interval previously had been
defined somewhere else, probably in url-vars.el.

> What problem do they solve?

That function to be called is not available and that the URL library is
not loaded when the customize :set function gets called. You don't want
to save periodically the URL history if the URL library is not even
initialized.

The function url-do-setup calls (url-history-setup-save-timer)
anyway.

> Let's look for another solution.

The test for (featurep 'url) could be replaced by a test for the
variable url-setup-done as I suggested above.

All changes should be made to the :set function in the defcustom for
url-history-save-interval as well because I copied it from there in
the first place.

-- 
Klaus Straubinger

Re: url-history.el: variable url-history-track defined but not used

[Richard M. Stallman]

I installed your patch.  Thanks.

Re: url-history.el: variable url-history-track defined but not used

[Klaus Straubinger]

> I installed your patch.  Thanks.

Thank you.

I had suggested that the improved :set function could also be of use
for the defcustom of the variable url-history-save-interval where I
copied my first suggestion from.

And while looking at the code, I found two new possible improvements:

The variable url-history-list is defined but not used and could be
deleted. The variable url-history-hash-table is its replacement.

The code in url-history-setup-save-timer checks for the availability of
the functions run-at-time and start-itimer, as far as I understand it
to be able to run on XEmacs as well. XEmacs compatibility has been
removed elsewhere, so this could be simplified as well.

I would propose the following patch:


--- url-history.el.orig	2005-12-02 08:10:18.000000000 +0100
+++ url-history.el	2005-12-02 16:35:01.000000000 +0100
@@ -63,22 +63,15 @@
 Default is 1 hour.  Note that if you change this variable outside of
 the `customize' interface after `url-do-setup' has been run, you need
 to run the `url-history-setup-save-timer' function manually."
-  :set (function (lambda (var val)
-		   (set-default var val)
-		   (and (featurep 'url)
-			(fboundp 'url-history-setup-save-timer)
-                        (let ((def (symbol-function
-                                    'url-history-setup-save-timer)))
-                          (not (and (listp def) (eq 'autoload (car def)))))
-			(url-history-setup-save-timer))))
+  :set #'(lambda (var val)
+	   (set-default var val)
+	   (if (bound-and-true-p url-setup-done)
+	       (url-history-setup-save-timer)))
   :type 'integer
   :group 'url-history)
 
 (defvar url-history-timer nil)
 
-(defvar url-history-list nil
-  "List of urls visited this session.")
-
 (defvar url-history-changed-since-last-save nil
   "Whether the history list has changed since the last save operation.")
 
@@ -91,21 +84,12 @@
 (defun url-history-setup-save-timer ()
   "Reset the history list timer."
   (interactive)
-  (ignore-errors
-    (cond ((fboundp 'cancel-timer) (cancel-timer url-history-timer))
-	  ((fboundp 'delete-itimer) (delete-itimer url-history-timer))))
+  (ignore-errors (cancel-timer url-history-timer))
   (setq url-history-timer nil)
   (if (and (eq url-history-track t) url-history-save-interval)
-      (setq url-history-timer
-	    (cond
-	     ((fboundp 'run-at-time)
-	      (run-at-time url-history-save-interval
-			   url-history-save-interval
-			   'url-history-save-history))
-	     ((fboundp 'start-itimer)
-	      (start-itimer "url-history-saver" 'url-history-save-history
-			    url-history-save-interval
-			    url-history-save-interval))))))
+      (setq url-history-timer (run-at-time url-history-save-interval
+					   url-history-save-interval
+					   'url-history-save-history))))
 
 ;;;###autoload
 (defun url-history-parse-history (&optional fname)

Re: url-history.el: variable url-history-track defined but not used

[Richard M. Stallman]

I installed that simplification.  Thanks.

Re: url-history.el: variable url-history-track defined but not used

[Klaus Straubinger]

> I installed that simplification.  Thanks.

Unfortunately, url-history.el still has the definition of the variable
url-history-list despite the ChangeLog item

	* url-history.el (url-history-list): Var deleted.


Furthermore, I would like to suggest that the simplications done to
url-history.el could also be applied to url-cookie.el as in the
following patch:


--- url-cookie.el.orig	2005-07-04 14:17:02.000000000 +0200
+++ url-cookie.el	2005-12-07 16:36:00.000000000 +0100
@@ -448,11 +448,10 @@
 Default is 1 hour.  Note that if you change this variable outside of
 the `customize' interface after `url-do-setup' has been run, you need
 to run the `url-cookie-setup-save-timer' function manually."
-  :set (function (lambda (var val)
-		   (set-default var val)
-		   (and (featurep 'url)
-			(fboundp 'url-cookie-setup-save-timer)
-			(url-cookie-setup-save-timer))))
+  :set #'(lambda (var val)
+	   (set-default var val)
+	   (if (bound-and-true-p url-setup-done)
+	       (url-cookie-setup-save-timer)))
   :type 'integer
   :group 'url)
 
@@ -460,21 +459,12 @@
 (defun url-cookie-setup-save-timer ()
   "Reset the cookie saver timer."
   (interactive)
-  (ignore-errors
-    (cond ((fboundp 'cancel-timer) (cancel-timer url-cookie-timer))
-	  ((fboundp 'delete-itimer) (delete-itimer url-cookie-timer))))
+  (ignore-errors (cancel-timer url-cookie-timer))
   (setq url-cookie-timer nil)
   (if url-cookie-save-interval
-      (setq url-cookie-timer
-	    (cond
-	     ((fboundp 'run-at-time)
-	      (run-at-time url-cookie-save-interval
-			   url-cookie-save-interval
-			   'url-cookie-write-file))
-	     ((fboundp 'start-itimer)
-	      (start-itimer "url-cookie-saver" 'url-cookie-write-file
-			    url-cookie-save-interval
-			    url-cookie-save-interval))))))
+      (setq url-cookie-timer (run-at-time url-cookie-save-interval
+					  url-cookie-save-interval
+					  #'url-cookie-write-file))))
 
 (provide 'url-cookie)

Re: url-history.el: variable url-history-track defined but not used

[Richard M. Stallman]

Thanks.