You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flex.apache.org by "Michael A. Labriola" <la...@digitalprimates.net> on 2012/04/10 16:16:33 UTC
ArrayList itemUpdateHandler Change
This is already in my whiteboard put I want to push it to trunk.
In ArrayList's itemUpdatedHandler, it checks the event.target and attempts to get the index. However, in many cases (such as when you call itemUpdated()) this target is not defined. getItemIndex() returns a -1 in that case, but here it is assigned to a unsigned integer, meaning the property it dispatches is even more non-sensical than normal. Simple change right now is to make this signed so we at least get the -1. Any objections?
protected function itemUpdateHandler(event:PropertyChangeEvent):void
{
internalDispatchEvent(CollectionEventKind.UPDATE, event);
// need to dispatch object event now
if (_dispatchEvents == 0 && hasEventListener(PropertyChangeEvent.PROPERTY_CHANGE))
{
var objEvent:PropertyChangeEvent = PropertyChangeEvent(event.clone());
var index:uint = getItemIndex(event.target);
objEvent.property = index.toString() + "." + event.property;
dispatchEvent(objEvent);
}
}
Proposed:
protected function itemUpdateHandler(event:PropertyChangeEvent):void
{
internalDispatchEvent(CollectionEventKind.UPDATE, event);
// need to dispatch object event now
if (_dispatchEvents == 0 && hasEventListener(PropertyChangeEvent.PROPERTY_CHANGE))
{
var objEvent:PropertyChangeEvent = PropertyChangeEvent(event.clone());
//When itemUpdated is called, there is no event target. This means getItemIndex returns a -1
//Since this was originally cast as a uint, this caused many strange results. Changing it to
//an int to be consistent throughout other uses of this event
var index:int = getItemIndex(event.target);
objEvent.property = index.toString() + "." + event.property;
dispatchEvent(objEvent);
}
}
digital primates (r)
Michael Labriola
mlabriola@digitalprimates.net<ma...@digitalprimates.net>
tel: +1.773.693.7800 x206
fax: +1.773.409.9251
Re: ArrayList itemUpdateHandler Change
Posted by Tink <fl...@tink.ws>.
On 10 Apr 2012, at 15:16, Michael A. Labriola wrote:
> This is already in my whiteboard put I want to push it to trunk.
>
> In ArrayList's itemUpdatedHandler, it checks the event.target and
> attempts to get the index. However, in many cases (such as when you
> call itemUpdated()) this target is not defined. getItemIndex()
> returns a -1 in that case, but here it is assigned to a unsigned
> integer, meaning the property it dispatches is even more non-
> sensical than normal. Simple change right now is to make this signed
> so we at least get the -1. Any objections?
>
> protected function
> itemUpdateHandler(event:PropertyChangeEvent):void
> {
> internalDispatchEvent(CollectionEventKind.UPDATE, event);
> // need to dispatch object event now
> if (_dispatchEvents == 0 &&
> hasEventListener(PropertyChangeEvent.PROPERTY_CHANGE))
> {
> var objEvent:PropertyChangeEvent =
> PropertyChangeEvent(event.clone());
> var index:uint = getItemIndex(event.target);
> objEvent.property = index.toString() + "." +
> event.property;
> dispatchEvent(objEvent);
> }
> }
>
> Proposed:
>
> protected function
> itemUpdateHandler(event:PropertyChangeEvent):void
> {
> internalDispatchEvent(CollectionEventKind.UPDATE, event);
> // need to dispatch object event now
> if (_dispatchEvents == 0 &&
> hasEventListener(PropertyChangeEvent.PROPERTY_CHANGE))
> {
> var objEvent:PropertyChangeEvent =
> PropertyChangeEvent(event.clone());
> //When itemUpdated is called, there is no event
> target. This means getItemIndex returns a -1
> //Since this was originally cast as a uint, this
> caused many strange results. Changing it to
> //an int to be consistent throughout other uses of
> this event
> var index:int = getItemIndex(event.target);
> objEvent.property = index.toString() + "." +
> event.property;
> dispatchEvent(objEvent);
> }
> }
Looks like common sense to me.
Tink
Re: ArrayList itemUpdateHandler Change
Posted by Jonathan Campos <jo...@gmail.com>.
On Tue, Apr 10, 2012 at 9:16 AM, Michael A. Labriola <
labriola@digitalprimates.net> wrote:
> var objEvent:PropertyChangeEvent = PropertyChangeEvent(event.clone());
> //When itemUpdated is called, there is no event target.
> This means getItemIndex returns a -1
> //Since this was originally cast as a uint, this caused
> many strange results. Changing it to
> //an int to be consistent throughout other uses of this
> event
> var index:int = getItemIndex(event.target);
> objEvent.property = index.toString() + "." + event.property;
> dispatchEvent(objEvent);
>
I would agree with this change as uints also are very error prone on a
variety of air targets and ints are uniform across platforms.
--
Jonathan Campos
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>It looks like the index is converted to a string. Using -1 instead of 0 seems just as bad. I would think the fix would be to find a way to get the right index.
Alex,
It's not -1 instead of 0, its -1 instead of 4294967295. Since that seems like a valid index there is other code which can handle the -1 which now tries to treat that as a positive index into an ArrayList inside of AdvancedDataGrid, etc. However, I agree with you. Let me put together a better fix for both ArrayList and VectorList and let me push that to my whiteboard.
Mike
--
Alex Harui
Flex SDK Team
Adobe Systems, Inc.
http://blogs.adobe.com/aharui
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>It looks like the index is converted to a string. Using -1 instead of 0 seems just as bad. I would think the fix would be to find a way to get the right index.
Okay, ArrayList and VectorList along with the unit tests for each are now pushed back up to my whiteboard. Please review. Factored out some code into a separate method. It now correctly assigns this index in the case of an itemUpdated or of a child object dispatch.
Mike
Re: ArrayList itemUpdateHandler Change
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> I will put it where I think it should go for now. I am not planning on modifying the manifests, so someone else can make that decision.
I'll stick into the build_framework script for you once you've checked it into patches.
Thanks,
Justin
Re: ArrayList itemUpdateHandler Change
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> I am still curious how this works with the framework as it is now, but working on correcting these.
It generally has well formed XML and escapes characters like ",", "&",">" etc. Currently only public and protected constants, properties and methods comments are looked at, I've run over all of the private and things marked @prices and fixed a fix issues the other day.
Thanks,
Justin
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>Probably best to submit to the patches branch (for now) until we have an initial release.
Okay, in it.
>These is also the question of what namespace it should go in.
I will put it where I think it should go for now. I am not planning on modifying the manifests, so someone else can make that decision. I honestly don't care, I just want to get it into the project and move back into compiler land.
>ASDocs currently fails to compile on the code (basically comments must be valid XML so "<" and ">" should be escaped) - not a huge issue IMO. I was going to provide a patch but not had the time.
I am still curious how this works with the framework as it is now, but working on correcting these.
Mike
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>ASDocs currently fails to compile on the code (basically comments must be valid XML so "<" and ">" should be escaped) - not a huge issue IMO. I was going to provide a patch but not had the time.
Interesting as I think 99% of those comments are just copies from ArrayList
Re: ArrayList itemUpdateHandler Change
Posted by Justin Mclean <ju...@classsoftware.com>.
Hi,
> Now that this issue is fixed, any problems with me committing this and the VectorList and Collection to trunk.
Probably best to submit to the patches branch (for now) until we have an initial release.
These is also the question of what namespace it should go in. You might might to look thought the long discussions about this previously. The general consensus (although certainly not final) was than it should live in a separate namespace and that org.apache.flex was a good idea. In this case I'd have no objections to the existing mx namespace - but perhaps add it to the apache project/manifest rather than the framework project?
ASDocs currently fails to compile on the code (basically comments must be valid XML so "<" and ">" should be escaped) - not a huge issue IMO. I was going to provide a patch but not had the time.
Thanks,
Justin
Re: ArrayList itemUpdateHandler Change
Posted by Alex Harui <ah...@adobe.com>.
On 4/11/12 7:26 AM, "Michael A. Labriola" <la...@digitalprimates.net>
wrote:
>
> Now that this issue is fixed, any problems with me committing this and the
> VectorList and Collection to trunk.
Trunk is "closed" until we cut a "parity" release. I think Justin has a
branch you can use for now.
--
Alex Harui
Flex SDK Team
Adobe Systems, Inc.
http://blogs.adobe.com/aharui
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>I would think the fix would be to find a way to get the right index.
Now that this issue is fixed, any problems with me committing this and the VectorList and Collection to trunk. We still need to determine how we want to add serialization for Vector(if we do) for right now I have removed the interface declaration from both VectorCollection and VectorList.
Mike
Re: ArrayList itemUpdateHandler Change
Posted by Alex Harui <ah...@adobe.com>.
On 4/10/12 9:11 AM, "Michael A. Labriola" <la...@digitalprimates.net>
wrote:
>> I do now. Is there an exiting Flex bug for it?
>
> Can't find the original bug I wanted, however, on a side note, this one
> applies to one of the inconsistencies that I think needs to be fixed:
>
> https://bugs.adobe.com/jira/browse/SDK-31861
>
> Want me to create a jira issue somewhere for this one?
I don't think there has to be a JIRA issue for every change. I was just
curious as to how many people might have hit this problem and was hoping it
would fix some existing bugs.
I'm still troubled by the proposed fix, but maybe I'm not understanding it.
It looks like the index is converted to a string. Using -1 instead of 0
seems just as bad. I would think the fix would be to find a way to get the
right index.
--
Alex Harui
Flex SDK Team
Adobe Systems, Inc.
http://blogs.adobe.com/aharui
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>I do now. Is there an exiting Flex bug for it?
Can't find the original bug I wanted, however, on a side note, this one applies to one of the inconsistencies that I think needs to be fixed:
https://bugs.adobe.com/jira/browse/SDK-31861
Want me to create a jira issue somewhere for this one?
Mike
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>I do now. Is there an exiting Flex bug for it?
There was a bug about the AdvancedDataGrid and this was the root cause. It's been a couple of years but I can try to find it again.
Mike
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>That is exactly my point. We can "fix" a whole slew of big, bad, annoying things. but we will end up creating an SDK version that will totally shatter many existing applications which most likely will have implemented >careful workarounds and quirk-specific code for quirks that we will be "resolving"... Don't get me wrong, this is a great idea and well worth the time in my opinion, but how will we prepare (or better, shield) the >community from the effects on the common developer's code?
Well, first thing I think is that we make sure everything still internally works in the framework. At some point though we will likely need to document this and bit the bullet.
As someone who has been intimate with the framework for years I can guarantee you that the order of events in something like combobox has basically changed every other versions since flex 2, so we have precedent :)
RE: ArrayList itemUpdateHandler Change
Posted by David Coleman <da...@hotmail.com>.
That is exactly my point. We can "fix" a whole slew of big, bad, annoying things. but we will end up creating an SDK version that will totally shatter many existing applications which most likely will have implemented careful workarounds and quirk-specific code for quirks that we will be "resolving"... Don't get me wrong, this is a great idea and well worth the time in my opinion, but how will we prepare (or better, shield) the community from the effects on the common developer's code?
> From: labriola@digitalprimates.net
> To: flex-dev@incubator.apache.org
> Subject: RE: ArrayList itemUpdateHandler Change
> Date: Tue, 10 Apr 2012 16:18:43 +0000
>
> >I agree with this, there are a number of inconsistencies in the handling of indexes throughout the framework. I think that it is a worthwhile effort to address them in this way. As Mike states, the number of side >effects could be numerous. But with that being said, I think that changing this from unsigned to signed may be the first step in identifying some of these side effects and possibly fix one or two incidental issues. As a >long time flex developer, I would add a word of caution that this seems like a slippery slope. Once we go down this path we may find a huge number of subsequent changes flying around. I think we will definitely >resolve some long pending issues, but we should be very careful and aware that this could be a rather deep rabbit hole.
>
> I agree. I have identified about 20 inconsistencies so far in the way property change is dispatched or used in certain circumstances and I think the fixes will be great. Again though, I am not proposing any of these until we have tests. If you look at the unit tests I wrote for ArrayList, you will see things like "This demonstrates the way it works today, but are we sure we want this behavior," because I do think there are actually a number of issues that should be addressed.
>
> For example, using setItemAt, the CollectionChange event has a PropertyChangeEvent for its items, and those items have oldValue, newValue and property, which is actually the index. However, in the case of an add or remove, the items array actually contains the real item that was added or removed, not a PropertyChangeEvent. In this case of an update, the property change comes first and the collection change second. In the case of an add or remove, it is the opposite.
>
> Mike
>
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>I agree with this, there are a number of inconsistencies in the handling of indexes throughout the framework. I think that it is a worthwhile effort to address them in this way. As Mike states, the number of side >effects could be numerous. But with that being said, I think that changing this from unsigned to signed may be the first step in identifying some of these side effects and possibly fix one or two incidental issues. As a >long time flex developer, I would add a word of caution that this seems like a slippery slope. Once we go down this path we may find a huge number of subsequent changes flying around. I think we will definitely >resolve some long pending issues, but we should be very careful and aware that this could be a rather deep rabbit hole.
I agree. I have identified about 20 inconsistencies so far in the way property change is dispatched or used in certain circumstances and I think the fixes will be great. Again though, I am not proposing any of these until we have tests. If you look at the unit tests I wrote for ArrayList, you will see things like "This demonstrates the way it works today, but are we sure we want this behavior," because I do think there are actually a number of issues that should be addressed.
For example, using setItemAt, the CollectionChange event has a PropertyChangeEvent for its items, and those items have oldValue, newValue and property, which is actually the index. However, in the case of an add or remove, the items array actually contains the real item that was added or removed, not a PropertyChangeEvent. In this case of an update, the property change comes first and the collection change second. In the case of an add or remove, it is the opposite.
Mike
RE: ArrayList itemUpdateHandler Change
Posted by David Coleman <da...@hotmail.com>.
I agree with this, there are a number of inconsistencies in the handling of indexes throughout the framework. I think that it is a worthwhile effort to address them in this way. As Mike states, the number of side effects could be numerous. But with that being said, I think that changing this from unsigned to signed may be the first step in identifying some of these side effects and possibly fix one or two incidental issues. As a long time flex developer, I would add a word of caution that this seems like a slippery slope. Once we go down this path we may find a huge number of subsequent changes flying around. I think we will definitely resolve some long pending issues, but we should be very careful and aware that this could be a rather deep rabbit hole.
> From: labriola@digitalprimates.net
> To: flex-dev@incubator.apache.org
> Subject: RE: ArrayList itemUpdateHandler Change
> Date: Tue, 10 Apr 2012 15:47:12 +0000
>
> >Frankly, all of the propertyChange stuff and collectionChange stuff is a mess and inconsistent. I want to fix it all, but I am trying to make the narrowest number of changes until we have tests. The other method:
>
> Adding to this for a moment. One of the issues here is that when a child object is an event dispatcher and dispatches the event, the ArrayList passes along that event. That is use case #1 for itemUpdatedHandler... in this use case the target can be used for determining which item it was in the Array.
>
> Use case #2 is that this is called when a property is directly modified through things like itemUpdated() or setItemAt(), etc. In those cases, there is actually a source propery set instead.
>
> However, in both cases itemUpdateHandler attempts to use the target to find the index in the array and hence a property change event. Initially I thought we could fix this by just having two handlers, and we can... and should. The amusing part is that this particular property of the property change event is rarely examined in the framework, it is the mere existence of the event that causes a refresh. However, this error does actually have some performance impact issue (as well as a few times where you can get out of bounds errors) in the AdvancedDataGrid.
>
> So, I would like to truly 100% fix it the right way. Things would be better, more efficient and we could actually fix some other bugs. I just cant tell all of the side effects so I want to keep it contained until we have tests.
>
> Make more sense?
>
> Mike
>
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>Frankly, all of the propertyChange stuff and collectionChange stuff is a mess and inconsistent. I want to fix it all, but I am trying to make the narrowest number of changes until we have tests. The other method:
Adding to this for a moment. One of the issues here is that when a child object is an event dispatcher and dispatches the event, the ArrayList passes along that event. That is use case #1 for itemUpdatedHandler... in this use case the target can be used for determining which item it was in the Array.
Use case #2 is that this is called when a property is directly modified through things like itemUpdated() or setItemAt(), etc. In those cases, there is actually a source propery set instead.
However, in both cases itemUpdateHandler attempts to use the target to find the index in the array and hence a property change event. Initially I thought we could fix this by just having two handlers, and we can... and should. The amusing part is that this particular property of the property change event is rarely examined in the framework, it is the mere existence of the event that causes a refresh. However, this error does actually have some performance impact issue (as well as a few times where you can get out of bounds errors) in the AdvancedDataGrid.
So, I would like to truly 100% fix it the right way. Things would be better, more efficient and we could actually fix some other bugs. I just cant tell all of the side effects so I want to keep it contained until we have tests.
Make more sense?
Mike
Re: ArrayList itemUpdateHandler Change
Posted by Alex Harui <ah...@adobe.com>.
On 4/10/12 8:38 AM, "Michael A. Labriola" <la...@digitalprimates.net>
wrote:
> However, regardless of all of this, we are still calling a method which states
> that -1 is a valid return value, but casting it to an uinsigned integer. That
> is a problem in and of itself.
>
> See the issue?
I do now. Is there an exiting Flex bug for it?
--
Alex Harui
Flex SDK Team
Adobe Systems, Inc.
http://blogs.adobe.com/aharui
RE: ArrayList itemUpdateHandler Change
Posted by "Michael A. Labriola" <la...@digitalprimates.net>.
>This did not make sense to me. ItemUpdated is called on objects that should already exist in a dataprovider, no? Can I get more details on the scenario?
When you call itemUpdated, you pass it an object:
public function itemUpdated(item:Object, property:Object = null,
oldValue:Object = null,
newValue:Object = null):void
{
var event:PropertyChangeEvent =
new PropertyChangeEvent(PropertyChangeEvent.PROPERTY_CHANGE);
event.kind = PropertyChangeEventKind.UPDATE;
event.source = item;
event.property = property;
event.oldValue = oldValue;
event.newValue = newValue;
itemUpdateHandler(event);
}
Notice that the event is passed to the itemUpdateHandler, not dispatched. So, there is no target in the event, which is what is being looked for in the other method. Frankly, all of the propertyChange stuff and collectionChange stuff is a mess and inconsistent. I want to fix it all, but I am trying to make the narrowest number of changes until we have tests. The other method: itemUpdateHandler() is sometimes called this way, and sometimes called as the result of an actual event dispatch.
However, regardless of all of this, we are still calling a method which states that -1 is a valid return value, but casting it to an uinsigned integer. That is a problem in and of itself.
See the issue?
Mike
Re: ArrayList itemUpdateHandler Change
Posted by Alex Harui <ah...@adobe.com>.
On 4/10/12 7:16 AM, "Michael A. Labriola" <la...@digitalprimates.net>
wrote:
> This is already in my whiteboard put I want to push it to trunk.
Trunk still isn't "open" until we get a parity release.
>
> In ArrayList's itemUpdatedHandler, it checks the event.target and attempts to
> get the index. However, in many cases (such as when you call itemUpdated())
> this target is not defined.
This did not make sense to me. ItemUpdated is called on objects that should
already exist in a dataprovider, no? Can I get more details on the
scenario?
--
Alex Harui
Flex SDK Team
Adobe Systems, Inc.
http://blogs.adobe.com/aharui