defadvice question.

defadvice question.

[Michal]

Hallo group members.
when doing C-u M-x cvs-checkout 
I wanted to always have in minibuffer history ring, my cvs root, just to
use up arrow key to get it. So I did:

(setenv "CVSROOT" "/my/cvs/root")

(defadvice cvs-checkout
  (before cvs-checkout-cvs-root-add-to-history-ring)
  (add-to-list 'minibuffer-history (getenv "CVSROOT")))

(ad-activate 'cvs-checkout)


but it does not work. Have You an idea why?

best regards,
Michal

Re: defadvice question.

[Andreas Politz]

Michal <rabbitko@tenbit.pl> writes:

> Hallo group members.
> when doing C-u M-x cvs-checkout 
> I wanted to always have in minibuffer history ring, my cvs root, just to
> use up arrow key to get it. So I did:
>
> (setenv "CVSROOT" "/my/cvs/root")
>
> (defadvice cvs-checkout
>   (before cvs-checkout-cvs-root-add-to-history-ring)
>   (add-to-list 'minibuffer-history (getenv "CVSROOT")))
>
> (ad-activate 'cvs-checkout)
>
>
> but it does not work. Have You an idea why?
>
> best regards,
> Michal

I suspect that it does work, just not the way you expect it :
`add-to-list' does not insert duplicate elements into a list. That
means, if CVSROOT is already at some position in the
`minibuffer-history', this advice does nothing.

On the other hand, is this even necessary ? Peeking at the
`cvs-get-cvsroot' suggests, that CVSROOT is the default value anyway
(when calling `cvs-checkout' w/o prefix), unless you are already in a
cvs directory or the `cvs-root' variable is set.

-ap

Re: defadvice question.

[Michal]

hallo Adreas

Andreas Politz <politza@fh-trier.de> writes:

> I suspect that it does work, just not the way you expect it :
> `add-to-list' does not insert duplicate elements into a list. That
> means, if CVSROOT is already at some position in the
> `minibuffer-history', this advice does nothing.

this is not on the list yet.

>
> On the other hand, is this even necessary ? Peeking at the
> `cvs-get-cvsroot' suggests, that CVSROOT is the default value anyway
> (when calling `cvs-checkout' w/o prefix), unless you are already in a
> cvs directory or the `cvs-root' variable is set.

I am calling cvs-checkout with prefix, because I need to add some
parameters to cvs ( -r BRANCH_NAME), so it also asks about the cvs root.

best regards,
MIchal

Re: defadvice question.

[Kevin Rodgers]

Michal wrote:
> Hallo group members.
> when doing C-u M-x cvs-checkout 
> I wanted to always have in minibuffer history ring, my cvs root, just to
> use up arrow key to get it. So I did:
> 
> (setenv "CVSROOT" "/my/cvs/root")
> 
> (defadvice cvs-checkout
>   (before cvs-checkout-cvs-root-add-to-history-ring)
>   (add-to-list 'minibuffer-history (getenv "CVSROOT")))
> 
> (ad-activate 'cvs-checkout)
> 
> 
> but it does not work. Have You an idea why?

`before' advice runs after the interactive arguments are read.  See the
"Combined Definition" node of the Elisp manual (under "Advising Functions").

You can modify the interactive form, like this:

(defadvice cvs-checkout (before minibuffer-history)
   "Add CVSROOT environment variable to `minibuffer-history'."
   (interactive (let ((minibuffer-history
		      (cons (getenv "CVSROOT") minibuffer-history)))
		 ...)))

The tricky part is the "...".  You could copy the entire interactive form from
cvs-checkout source in pcvs.el, but that defeats the whole purpose of using
advice.  You might be able to work around it like this:

(defvar cvs-checkout-interactive-form
   (interactive-form 'cvs-checkout))

(defadvice cvs-checkout (before minibuffer-history)
   "Add CVSROOT environment variable to `minibuffer-history'."
   (interactive (let ((minibuffer-history
		      (cons (getenv "CVSROOT") minibuffer-history)))
		 (call-interactively `(lambda (&rest cvs-checkout-args)
					,cvs-checkout-interactive-form
					cvs-checkout-args)))))

-- 
Kevin Rodgers
Denver, Colorado, USA

Re: defadvice question.

[Michal]

Hallo Kevin.
Thank You very much for the answer.
Unfortunately I have problems with both solutions.

Did You check if it works on YOur site, or this is rather Your suggestion?

Kevin Rodgers <kevin.d.rodgers@gmail.com> writes:

>
> (defadvice cvs-checkout (before minibuffer-history)
>   "Add CVSROOT environment variable to `minibuffer-history'."
>   (interactive (let ((minibuffer-history
> 		      (cons (getenv "CVSROOT") minibuffer-history)))
> 		 ...)))
>
> The tricky part is the "...".  You could copy the entire interactive form from
> cvs-checkout source in pcvs.el, but that defeats the whole purpose of using
> advice.  You might be able to work around it like this:

I C-u M-x cvs-checkout

call-interactively: Symbol's value as variable is void: \.\.\.


>
> (defvar cvs-checkout-interactive-form
>   (interactive-form 'cvs-checkout))
>
> (defadvice cvs-checkout (before minibuffer-history)
>   "Add CVSROOT environment variable to `minibuffer-history'."
>   (interactive (let ((minibuffer-history
> 		      (cons (getenv "CVSROOT") minibuffer-history)))
> 		 (call-interactively `(lambda (&rest cvs-checkout-args)
> 					,cvs-checkout-interactive-form
> 					cvs-checkout-args)))))

While this does not produce any error message, but my cvs root is not
put to minibuffer history.

best regards

Re: defadvice question.

[Kevin Rodgers]

Michal wrote:
> Hallo Kevin.
> Thank You very much for the answer.
> Unfortunately I have problems with both solutions.
> 
> Did You check if it works on YOur site, or this is rather Your suggestion?

I actually tested this suggestion.

> Kevin Rodgers <kevin.d.rodgers@gmail.com> writes:
> 
>> (defadvice cvs-checkout (before minibuffer-history)
>>   "Add CVSROOT environment variable to `minibuffer-history'."
>>   (interactive (let ((minibuffer-history
>> 		      (cons (getenv "CVSROOT") minibuffer-history)))
>> 		 ...)))
>>
>> The tricky part is the "...".  You could copy the entire interactive form from
>> cvs-checkout source in pcvs.el, but that defeats the whole purpose of using
>> advice.  You might be able to work around it like this:
> 
> I C-u M-x cvs-checkout
> 
> call-interactively: Symbol's value as variable is void: \.\.\.

... was not meant to be evaluated literally.  Also, I did not attempt to handle
a prefix argument -- to do that, just add this binding to the let form:

	(prefix-arg current-prefix-arg)

>> (defvar cvs-checkout-interactive-form
>>   (interactive-form 'cvs-checkout))
>>
>> (defadvice cvs-checkout (before minibuffer-history)
>>   "Add CVSROOT environment variable to `minibuffer-history'."
>>   (interactive (let ((minibuffer-history
>> 		      (cons (getenv "CVSROOT") minibuffer-history)))
>> 		 (call-interactively `(lambda (&rest cvs-checkout-args)
>> 					,cvs-checkout-interactive-form
>> 					cvs-checkout-args)))))
> 
> While this does not produce any error message, but my cvs root is not
> put to minibuffer history.

What does M-: (getenv "CVSROOT") display?

cvs-checkout reads several arguments interactively, with different functions
that use different history variables.  Which prompt are you interested in,
"CVS Root: ", "Module(s): ", or "CVS Checkout Directory: "?

-- 
Kevin Rodgers
Denver, Colorado, USA

Re: defadvice question.

[Michal]

Hallo Kevin!

Kevin Rodgers <kevin.d.rodgers@gmail.com> writes:

>>> The tricky part is the "...".  You could copy the entire interactive form from
>>> cvs-checkout source in pcvs.el, but that defeats the whole purpose of using
>>> advice.  You might be able to work around it like this:

then You rather meant just to copy these parameters by hand, didn't You?
sorry for my misunderstanding

> What does M-: (getenv "CVSROOT") display?
"michal@cvs.repository:/var/local/cvsroot"
>
> cvs-checkout reads several arguments interactively, with different functions
> that use different history variables.  Which prompt are you interested in,
> "CVS Root: ", "Module(s): ", or "CVS Checkout Directory: "?

"CVS Root:" one

best regards,
Michal

Re: defadvice question.

[Kevin Rodgers]

Michal wrote:
> Hallo Kevin!
> 
> Kevin Rodgers <kevin.d.rodgers@gmail.com> writes:
> 
>>>> The tricky part is the "...".  You could copy the entire interactive form from
>>>> cvs-checkout source in pcvs.el, but that defeats the whole purpose of using
>>>> advice.  You might be able to work around it like this:
> 
> then You rather meant just to copy these parameters by hand, didn't You?
> sorry for my misunderstanding

No problem, Michal.

>> What does M-: (getenv "CVSROOT") display?
> "michal@cvs.repository:/var/local/cvsroot"
>> cvs-checkout reads several arguments interactively, with different functions
>> that use different history variables.  Which prompt are you interested in,
>> "CVS Root: ", "Module(s): ", or "CVS Checkout Directory: "?
> 
> "CVS Root:" one

That is read with read-string, which uses minibuffer-history, so we got that
part right.

The problem is that I forgot to include this from your original post:
(ad-activate 'cvs-checkout)

I prefer to use the activate flag:
(defadvice cvs-checkout (before minibuffer-history activate)
   ...)

-- 
Kevin Rodgers
Denver, Colorado, USA

Re: defadvice question.

[Michal]

> I prefer to use the activate flag:
> (defadvice cvs-checkout (before minibuffer-history activate)
>   ...)

The thing I like most in emacs is that it is also used by people that do not know
the word "impossible".

Kevin, thank You very much for Your help.
Best regards,
Michal