"Breaking Change in TCustomImageList.AddImage"
"Breaking Change in TCustomImageList.AddImage"
or "How not to fix bugs"
The return value of TCustomImageList.AddImage changed from Seattle to Berlin.
Ever since this method exists (which is somewhere between Delphi 5 and Delphi 7) this function returned the index of the newly added image. This has been consistent with the other Add functions in this class and similar ones in other classes. Since Berlin it returns the size of the image list after adding the new image. Obviously this breaks existing code.
I have to admit that the documentation mentions exactly this new behavior - and it does this even in Delphi 7. Seems nobody cared about this documentation, perhaps because it would have been a less wise decision to implement this function in the documented way.
May be I didn't search hard enough, but I could not find any bug report claiming that the implementation didn't match the documentation, so I am unsure what made someone actually trigger this change.
Given the long time acceptance of the former implementation and the fact that the current implementation is a breaking change, I might suggest that simply changing the documentation would have been the less invasive solution.
Therefore I plead for reverting this change ASAP and change the documentation instead.
https://quality.embarcadero.com/browse/RSP-15870
https://quality.embarcadero.com/browse/RSP-15870
or "How not to fix bugs"
The return value of TCustomImageList.AddImage changed from Seattle to Berlin.
Ever since this method exists (which is somewhere between Delphi 5 and Delphi 7) this function returned the index of the newly added image. This has been consistent with the other Add functions in this class and similar ones in other classes. Since Berlin it returns the size of the image list after adding the new image. Obviously this breaks existing code.
I have to admit that the documentation mentions exactly this new behavior - and it does this even in Delphi 7. Seems nobody cared about this documentation, perhaps because it would have been a less wise decision to implement this function in the documented way.
May be I didn't search hard enough, but I could not find any bug report claiming that the implementation didn't match the documentation, so I am unsure what made someone actually trigger this change.
Given the long time acceptance of the former implementation and the fact that the current implementation is a breaking change, I might suggest that simply changing the documentation would have been the less invasive solution.
Therefore I plead for reverting this change ASAP and change the documentation instead.
https://quality.embarcadero.com/browse/RSP-15870
https://quality.embarcadero.com/browse/RSP-15870
Agreed and voted, thanks!
ReplyDeleteThe fix is correct. If you look at the code of TCustomImageList.AddImage you can see that is put the count of the imagelist into result before adding the images from the input imagelist (while it now does after adding the images which is according to the documentation).
ReplyDeleteIf the input imagelist happened to contain only one image or you passed an index > -1 then yes, by coincidence the result returned had the index of the newly added image. However if there were more than one image in the input (and the index passed is -1) the result is completely useless because the -1 is causing all images from the input to be added to the imagelist.
Stefan Glienke Passing an Index = -1 to add all images is a) not documented at all and b) similar to calling the method AddImages, which is documented and thus preferable. Anyway, the previous implementation gave either the index of the newly added image or (in case of Index = -1) the index of the first newly added image. This is not by coincidence but rather because the internal routine uses a simple Add by itself.
ReplyDeleteI understand that the Berlin implementation matches the documentation now. My objection is that the previous installation was better in terms of consistency and that the change actually breaks current code that has been valid since Delphi 7. I only suggest that adjusting the documentation to the implementation would be the better approach.
In other words, whether the fix is correct or not is not the point here, even if you think the new functionality is correct now (which is still disputable, IMHO).
ReplyDeleteStrictly speaking does the current implementation of AddImage still not adhere completely to the documentation. If we give nil for the first parameter the result is -1, but according to the docs the result should nevertheless be "the new size of the image list", which is always something >= 0.
ReplyDelete