macros: availability of context variables

[FND]

As discussed  here, evaluated macro parameters should have access to the current place and tiddler object.

The attached patch is based upon Eric's  CoreTweaks.

[FND]

corrected version

[JeremyRuston]

Concerned that using global variables isn't a good solution to this problem. I've done some experiments with wrapping plugins up as an anonymous function that is called with tiddler, place, etc as parameters, which seems a much cleaner approach, and less likely to run into re-entrancy problems.

[EricShulman]

Replying to JeremyRuston:

Concerned that using global variables isn't a good solution to this problem. I've done some experiments with wrapping plugins up as an anonymous function that is called with tiddler, place, etc as parameters, which seems a much cleaner approach, and less likely to run into re-entrancy problems.

The purpose of this code isn't to provide the 'tiddler' and 'place' values to plugins... it is to make it available within the core's parseParams() function, so that "evalutaed macro parameters" (i.e., using {{...}}) can make reference to these values.

Note also that the current release of TW already defines a global 'window.tiddler' variable, but its value is 'junk' left over from loadPlugins(). The problem is that people are instinctively attempting to use macro parameters such as {{tiddler.title}} that reference this global variable, but does not return the expected value (i.e., the current tiddler's title).

The suggested CoreTweak? simply updates this existing global variable so that its value is actually useful...

[EricShulman]

Based on discussion in the "Developer's Conference Call" on Nov 10, 2008, this change should be accepted for the next release.

Note that in the code below, the tiddler that is used is the *containing* tiddler i.e., the tiddler in which the macro is being *rendered*, which may be different from the tiddler in which the macro *source* is stored (which is the tiddler value that is passed-in as a param). If the macro is not being rendered in a tiddler (e.g., content in the mainMenu div), then the tiddler context falls back to using the tiddler in which the macro is stored.

function invokeMacro(place,macro,params,wikifier,tiddler)
{
	// ADD THE NEXT THREE LINES:
	var here=story.findContainingTiddler(place);
	window.tiddler=here?store.getTiddler(here.getAttribute("tiddler")):tiddler;
	window.place=place;

	try {
		var m = config.macros[macro];
		if(m && m.handler)
			m.handler(place,macro,params.readMacroParams(),wikifier,params,tiddler);
		else
			createTiddlyError(place,config.messages.macroError.format([macro]),config.messages.macroErrorDetails.format([macro,config.messages.missingMacro]));
	} catch(ex) {
		createTiddlyError(place,config.messages.macroError.format([macro]),config.messages.macroErrorDetails.format([macro,ex.toString()]));
	}
}

[EricShulman]

Why was this not done for 2.4.2 as had been decided during the last Developer Conference call? This particular issue continues to be a significant thorn in the side of TW authors and was reported over a year ago, along with a fix that has now been extensively field-tested via TiddlyTools CoreTweaks?.

The problem is that TW authors often try to use 'tiddler.title' in computed macro parameters (e.g. <<someMacro ... {{tiddler.title}} ...>>), but get the wrong value because the 'tiddler' global *is* already defined, but does not contain information about the *current* tiddler as is generally expected, but rather contains a value left behind by the plugin loading process and, in fact, refers to the last systemConfig-tagged tiddler that was processed during document startup.

Unfortunately, as a result of this 'junk' value, their macros don't report an error due to a missing variable, but instead break in unexpected ways and can potentially result in destructive changes to tiddler content (by overwriting the wrong tiddler, based on the incorrect value of tiddler.title, for example).

Please don't defer this fix again... it's really long overdue! Thanks.

[SaqImtiaz]

Point of clarification: I'd explicitly mentioned at the beginning of the last Developer Conference call that any tickets that we discussed and agreed to implement may not necessarily make it into the next or even the subsequent release of TiddlyWiki. So while a decision was made to implement this ticket, no commitment was made as to when this would happen. (As per my personal notes from the developer call a decision was made to implement this pending discussion of the implementation. Apparently this detail was not reflected in the minutes uploaded to tiddlywiki.org)

All core changes require consideration and time from the lead developers and as such they do their best to cover as much ground as possible for each release.

I too agree that this is a useful ticket and it would be good to have this implemented sooner rather than later.

[EricShulman]

We reviewed 25 tickets during that call. Most were decided as "won't fix", "stay a plugin", or "needs discussion/investigate alternatives".... (which I found disappointing)... however, a few tickets, *including this one* were designated as "accept"... and I clearly recall (sorry, no written notes) being very relieved that this specific ticket (#444) was *NOT* one that anyone raised concerns about and that this fix would finally be added to the core after being an open ticket for more than a year.

In fact, much discussion about the fix had already occurred within this ticket itself and the code in question has been extensively field-tested over the entire time that this ticket has been open, so it seemed clear to me that this was a solid decision to finally implement this fix as submitted, and not a "we'll think about it some more" situation (like most of the other responses during that call.)

In any case, the scheduled build #, severity and priority for this ticket was simply changed 10 days ago without any explanation whatsoever. If there were any outstanding concerns about this implementation, then they should have been added to the ticket at the same time as its severity, priority and status were being changed.

Because I am not physically located at Osmosoft, I have to rely upon the communication channels we have set up in order to know what changes to expect in the next release, so that I can plan accordingly and provide the TW community with the support they need. Given the complete lack of explanation for the change in status of this ticket, simply asking "WHY was this not implemented?" would seem to be the most mild of ways that I could raise this issue.

So again, I ask: WHY was this not implemented in this release (2.4.2)?

(and quite frankly, "we just didn't have time to get to it" isn't a good enough explanation, given the number of *trivial* changes to the code that *have* been submitted for the current build.)

[SaqImtiaz]

I'll restrict my comments to decisions made on the Developer Conference call since that is my area of responsibility, and will leave other issues for the core developers to address.

I both specifically noted decisions reached for each ticket during the call and then on the following day went through the recording of the call to double check that I had everything down accurately. My notes clearly state that the decision reached was that the feature requested by this ticket should be implemented but that the implementation should be discussed further. No objections to this decision were raised then or after the fact.

I put a lot of effort into organizing the calls as best as I can, taking notes to ensure that decisions are captured accurately and running the calls as impartially as I can. There is no room for doubt whatsoever that I captured this decision correctly. If necessary I can upload the call recording so you can verify this for yourself.

[EricShulman]

changed ticket type from enhancement to defect to emphasize that the current TW core code produces incorrect results (as opposed to simply not supporting the syntax at all) and can, for some use-cases, potentially result in loss of data if the wrong tiddler is overwritten due to this bug.

[FND]

Fixed in changeset:8367