[prev in list] [next in list] [prev in thread] [next in thread]
List: gallery-devel
Subject: [Gallery-devel] Re: Why editing blocks doesn't work with my theme...
From: Bharat Mediratta <bharat () menalto ! com>
Date: 2006-05-04 5:58:55
Message-ID: 4459981F.9040206 () menalto ! com
[Download RAW message or body]
+gallery-devel
I'm not sure what to think about this so I'm including gallery-devel to
see if anybody else cares. Top-replying so that your entire message is
legible below.
I like the concept of having prototype in our codebase, but I fear that
if we start having to tweak the Javascript so that it works with
prototype then we're going to continually be breaking it.
About the only way that I can see for us to avoid having this problem
perpetually is to write a unit test which scans our .tpl files for that
construct and fails the test.
The way that you've done the change right now is somewhat intrusive in
that you're wrapping the existing code. How about if you did it like this:
for (var paramName in this.parameters) {
if (!this.parameters.propertyIsEnumerable(paramName)) {
continue;
}
...
}
This has two advantages:
1. It makes for a smaller change. Right now it's hard to read
the diff because the whitespace changes make it look like it's
bigger than it really is. "svn diff -x -uw" might help with
that though.
2. You can write a .tpl scanner that makes sure that anywhere we
have the "for (var X in Y)" construct it's immediately followed
by the "if Y.propertyIsEnumerable(X) { continue; }" idiom, and
if not we fail the test.
If we do that, then we can be sure that we don't regress. I'd be happy
to approve that.
Can you separate out any changes related to varType == 'text' into a
separate diff? As far as I can tell it's just a 2 line change, but the
other diffs are overshadowing it. I'd be happy to review that
separately. What kinds of error inputs do you think would cause it
problems?
-Bharat
Chris Smith wrote:
> So, attached is a diff of BlockSelectWidget.js with my changes to do
> with as you wish. It has the onerous propertyIsEnumerable hack (in
> about 10 places).
>
> And as a surprise bonus, I've also enhanced it to support "text" input
> types for block properties (before it only supported "checkboxes" and
> "choices"). My Animoblock block requires this. It's likely this has
> insufficient error handling (you can't go wrong with checkboxes and
> choices, but you sure can with text inputs). Currently bad input will
> just fail silently I guess.
>
> Anyway, that's that. I'd love to get an official resolution to the
> prototype issue (figured I'd wait to update my theme until it's
> resolved). Also, lemme kow aht you think about the text input
> processing of BlockSelectWidget, as currently my block depends up on
> it.
>
> Oh, and my svn checkout is all good. I too got those timeout errors
> that Alan reported, so it took many updates to checkout the whole
> thing. It's not clear to me that Alan ever got a resolution to that.
>
> Cheers,
> chris
>
> On 4/28/06, Chris Smith <chris@jacko.com> wrote:
>
>> Heya, so I had a change to look at this problem some more tonight. It
>> turns out that this is a well known issue. prototype.js breaks arrays
>> (unless you use them the prototype way). This blog describe the exact
>> problem I was having:
>>
>> http://blog.metawrap.com/blog/CommentView,guid,42b961d5-b539-4d9a-b1e0-108e546ae3e6.aspx
>>
>>
>> So, the good news is that there is a workaround. Using the suggestion
>> made by the last commenter on that page. I hacked BlockSelectWidget.js
>> to make an extra check when iterating over associative arrays. As an
>> example you go from this:
>>
>> for (var i in callbacks) {
>> callbacks[i]();
>> }
>>
>> to this:
>>
>> for (var i in callbacks) {
>> if (callbacks.propertyIsEnumerable(i)) {
>> callbacks[i]();
>> }
>> }
>>
>> As you can see it's a pretty ugly thing (and there are >10 places
>> where it had to be done), and only necessary because my theme happens
>> to include prototype.js.
>>
>> Sooo, what do you think? I see the two choices:
>> - put in this (or maybe there's another more elegant hack I don't
>> know about)
>> - ban the use of protype.js for themes.
>> Or, I guess, port BlockSelectWidget (and probabl other libs) *to*
>> prototype :)
>>
>> On a related note I'm kinda disconnected from the G2 repository at the
>> moment.
>> - CVS: I'm seeing "cvs [update aborted]: read lock failed - giving
>> up". Is the related o the svn migration?
>> - SVN: subversion isn't installed on howler, and it seems apt-get
>> won't install it. And it seems (I just learned from jules) our apt-get
>> is living on the "testing" tree, instead of "stable". Jules is trying
>> to figure out a way to get us to stable without making the world
>> explode.
>>
>> Cheers,
>> chris
>>
>>
>> On 4/15/06, Chris Smith <chris@jacko.com> wrote:
>> > Heya, so I kinda figured out why you can add/remove/edit blocks when
>> > editing an album that uses my debaser theme. It turns out that the
>> > BlockSelectWidget.js library is incompatible with prototype.js (which
>> > my theme uses).
>> >
>> > I made a little page to demonstrate the issue.
>> >
>> > http://www.jacko.com/dev/protonogo/
>> >
>> > But was too tired to investigate fixing it. The BlockSelectWidget.js
>> > does "for(var item in itemArray) {}" and things crash. On the surface
>> > it looks like prototype.js breaks a standard way of iterating over
>> > arrays. But this shouldn't be it since that would be such a bad thing
>> > someone would have noticed (and I'd see people complaining about it).
>> > Oh, I guess I found one person complaining:
>> >
>> > http://trimpath.com/forum/viewtopic.php?pid=937
>> >
>> > Anyway, I'd poke around more but I'm heading to SC and will be offline
>> > for a while. Thought you'd be curious or at least able to delegate to
>> > someone who was curious.
>> >
>> > Cheers,
>> > chris
>> >
>>
["BlockSelectWidget.js.diff" (application/octet-stream)]
Index: BlockSelectWidget.js
===================================================================
--- BlockSelectWidget.js (revision 13565)
+++ BlockSelectWidget.js (working copy)
@@ -93,12 +93,14 @@
this.toString = function() {
var result = "[" + this.blockId;
for (var paramName in this.parameters) {
- var paramValue = this.parameters[paramName];
- var defaultValue =
- bsw_WIDGET_BLOCKS[key][blockId]['parameters'][paramName]['defaultValue'];
- if (paramValue != defaultValue) {
- result += " " + paramName + "=" + paramValue;
- }
+ if (this.parameters.propertyIsEnumerable(paramName)) {
+ var paramValue = this.parameters[paramName];
+ var defaultValue =
+ \
bsw_WIDGET_BLOCKS[key][blockId]['parameters'][paramName]['defaultValue']; + \
if (paramValue != defaultValue) { + result += " " + paramName + \
"=" + paramValue; + }
+ }
}
result += "]";
@@ -109,8 +111,10 @@
/* Set the defaults */
if (bsw_WIDGET_BLOCKS[key][blockId]) {
for (var value in bsw_WIDGET_BLOCKS[key][blockId]['parameters']) {
- this.setParameterValue(
- value, bsw_WIDGET_BLOCKS[key][blockId]['parameters'][value]['defaultValue']);
+ if (bsw_WIDGET_BLOCKS[key][blockId]['parameters'].propertyIsEnumerable(value)) \
{ + this.setParameterValue(
+ value, \
bsw_WIDGET_BLOCKS[key][blockId]['parameters'][value]['defaultValue']); + }
}
} else {
/*
@@ -121,7 +125,9 @@
/* Override whatever we're specifically changing */
for (var value in values) {
- this.setParameterValue(value, values[value]);
+ if (values.propertyIsEnumerable(value)) {
+ this.setParameterValue(value, values[value]);
+ }
}
}
@@ -229,7 +235,6 @@
function bsw_selectToChange(key) {
bsw_disableButton(key, "AddButton");
bsw_enableButton(key, "RemoveButton");
-
var usedEl = document.getElementById("blocksUsedList_" + key);
if (usedEl.selectedIndex > 0) {
bsw_enableButton(key, "MoveUpButton");
@@ -376,7 +381,9 @@
/* For some reason, "block.parameters.length == 0" doesn't work here. wtf? */
var i = 0;
for (var paramName in block.parameters) {
- i++;
+ if (block.parameters.propertyIsEnumerable(paramName)) {
+ i++;
+ }
}
if (i == 0) {
return;
@@ -405,21 +412,23 @@
/* Now add the parameter value rows */
callbacks = new Array();
for (var paramName in block.parameters) {
- var paramRowEl = document.createElement("tr");
+ if (block.parameters.propertyIsEnumerable(paramName)) {
+ var paramRowEl = document.createElement("tr");
- var paramNameEl = document.createElement("td");
- paramNameEl.appendChild(document.createTextNode(block.parameters[paramName]['description']));
- paramRowEl.appendChild(paramNameEl);
+ var paramNameEl = document.createElement("td");
+ paramNameEl.appendChild(document.createTextNode(block.parameters[paramName]['description']));
+ paramRowEl.appendChild(paramNameEl);
- var paramValueEl = document.createElement("td");
- var result = bsw_getValueElement(paramName, blockPref, block);
- paramValueEl.appendChild(result[0]);
- paramRowEl.appendChild(paramValueEl);
+ var paramValueEl = document.createElement("td");
+ var result = bsw_getValueElement(paramName, blockPref, block);
+ paramValueEl.appendChild(result[0]);
+ paramRowEl.appendChild(paramValueEl);
- optionTbodyEl.appendChild(paramRowEl);
- if (result[1]) {
- callbacks.push(result[1]);
- }
+ optionTbodyEl.appendChild(paramRowEl);
+ if (result[1]) {
+ callbacks.push(result[1]);
+ }
+ }
}
optionTableEl.appendChild(optionTbodyEl);
blockOptionsEl.appendChild(optionTableEl);
@@ -429,7 +438,9 @@
* drops the state. So run callbacks at the end to set the state.
*/
for (var i in callbacks) {
- callbacks[i]();
+ if (callbacks.propertyIsEnumerable(i)) {
+ callbacks[i]();
+ }
}
bsw_updateVarOverrides(key, blockPref);
@@ -452,7 +463,13 @@
var returnElement;
var id = block.key + "_prefValue_" + paramName;
- if (elementType == 'boolean') {
+ if (elementType == 'text') {
+ returnElement = document.createElement("input");
+ returnElement.type = 'text';
+ returnElement.value = prefValue;
+ returnElement.onchange = function() { bsw_updatePrefValue(block.key, \
elementType, this); }; +
+ } else if (elementType == 'boolean') {
returnElement = document.createElement("input");
returnElement.type = 'checkbox';
returnElement.checked = undefined;
@@ -472,17 +489,19 @@
var option;
i = 0;
for (var choice in block.parameters[paramName]['extra']) {
- option = document.createElement("option");
- option.appendChild(document.createTextNode(choice));
- option.value = choice;
- option.innerHTML = block.parameters[paramName]['extra'][choice];
- returnElement.appendChild(option);
+ if (block.parameters[paramName]['extra'].propertyIsEnumerable(choice)) { \
+ option = document.createElement("option");
+ option.appendChild(document.createTextNode(choice));
+ option.value = choice;
+ option.innerHTML = block.parameters[paramName]['extra'][choice];
+ returnElement.appendChild(option);
- if (choice == prefValue) {
- callback = new Function('document.getElementById("' + id + '").selectedIndex = ' +
- i + ';');
- }
- i++;
+ if (choice == prefValue) {
+ callback = new Function('document.getElementById("' + id + \
'").selectedIndex = ' + + i + ';');
+ }
+ i++;
+ }
}
returnElement.selectedIndex = -1;
@@ -511,7 +530,9 @@
paramName = paramName.substr(paramName.indexOf('_') + 1);
var paramValue;
- if (varType == 'boolean') {
+ if (varType == 'text') {
+ paramValue = element.value;
+ } else if (varType == 'boolean') {
paramValue = element.checked;
} else {
paramValue = element.options[element.selectedIndex].value;
@@ -529,12 +550,16 @@
var block = bsw_WIDGET_BLOCKS[key][blockPref.blockId];
for (var paramName in block.parameters) {
- if (block.parameters[paramName]['type'] == 'boolean') {
- var currentValue = document.getElementById(key + "_prefValue_" + \
paramName).checked;
- for (var overrideName in block.parameters[paramName]['varOverrides']) {
- document.getElementById(key + "_prefValue_" + overrideName).disabled = \
currentValue;
- }
- }
+ if (block.parameters.propertyIsEnumerable(paramName)) {
+ if (block.parameters[paramName]['type'] == 'boolean') {
+ var currentValue = document.getElementById(key + "_prefValue_" + \
paramName).checked; + for (var overrideName in \
block.parameters[paramName]['varOverrides']) { + if \
(block.parameters[paramName]['varOverrides'].propertyIsEnumerable(overrideName)) { + \
document.getElementById(key + "_prefValue_" + overrideName).disabled = currentValue; \
+ } + }
+ }
+ }
}
}
-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic