createTiddlyLink bug caused by createExternalLink

[JayFresh - http://jayfresh.wordpress.com]

In the core createTiddlyLink() function, it should add the provided
"className" parameter to the class of the link, but in the case of a
static link (as signalled by setting the "isStatic" flag) it does not.

This is a consequence of the link being created using
createExternalLink (rather than createTiddlyButton), which does not
accept a className for the link.

My first reaction is to extend createExternalLink so it takes a
consistent parameter set with createTiddlyButton i.e.:

place,url -> parent,text,tooltip,action,className,id,accessKey,attribs

Thoughts?


J.

[Eric Shulman]

> My first reaction is to extend createExternalLink so it takes a
> consistent parameter set with createTiddlyButton i.e.:
>
> place,url -> parent,text,tooltip,action,className,id,accessKey,attribs
>
> Thoughts?

Changing the API of an existing core function is a very bad idea! It
breaks all existing plugin code that uses that function. Because
people normally just migrate their existing tiddlers (including
plugins) when upgrading, a core change that invalidates existing
plugins immediately breaks their current document content.

If it is necessary to add arguments, they should always be added to
the *end* of the argument list, and the internal handling should check
for "undefined" values so that existing code that does not pass those
arguments won't break.

-e

[JayFresh - http://jayfresh.wordpress.com]

Thanks Eric for catching that. I originally intended to extend the
list of params so it should really have been:

place,url -> parent,url,tooltip,action,className,id,accessKey,attribs

I'm interested to know whether this much larger list is overkill or
not. After all, I've only raised this buggy behaviour because of an
inconsistency with the treatment of className.


J.

[chris...@gmail.com]

On May 20, 8:39 pm, Eric Shulman <elsdes...@gmail.com> wrote:

> If it is necessary to add arguments, they should always be added to
> the *end* of the argument list, and the internal handling should check
> for "undefined" values so that existing code that does not pass those
> arguments won't break.

I would really really really like to see the core methods adjusted,
either stepwise or en masse such that:

* The old method names and signatures remain in place, unchanged
externally but wrap
* New methods that accept dictionaries of named parameters.

For example something like:

function
doHttp(type,url,data,contentType,username,password,callback,params,headers,allowCache)
{
// either
var method=type
return twDoHttp(method, url, callback, {data: data, type:
contentType, <etc>});
// or
return twDoHttp({method: type, url: url, data:, data,
contentType:contentType <etc>})
}

The first version has some ordered parameters for things which are
required for the method to function.

I believe this preserves backwards compatibility for existing plugins
while allowing new plugins to alter, without pain, the signature of
core methods.

Is this crazy?

[JayFresh - http://jayfresh.wordpress.com]

> I believe this preserves backwards compatibility for existing plugins
> while allowing new plugins to alter, without pain, the signature of
> core methods.
>
> Is this crazy?

I suppose you have to weigh this off against the extra code involved
in the core, plus how often people really want to override a core
function... plus how hard it is to hijack as and when you need to:

var oldFunc = someCoreFunc;
someCoreFunc = function(dodgyParams) {
/* my override code here... */
oldFunc.call(this,oldParams);
};


J.

[jayfresh]

I've created a ticket on trac and supplied a patch that fixes the
createTiddlyLink bug:
http://trac.tiddlywiki.org/ticket/673



On May 22, 11:23 am, "JayFresh - http://jayfresh.wordpress.com"