Fixes the method setExtension of TipImage.java in org.eclipse.tips.tests #525 #723
Conversation
There was a problem hiding this comment.
Thanks for implementing the missing functionality. When you address the minor comment on the code, could you please also improve the commit message such that it primarily explains the behavior change in TipImage and only mentions the fixed and re-enabled tests afterwards?
ua/org.eclipse.tips.core/src/org/eclipse/tips/core/TipImage.java
Outdated
Show resolved
Hide resolved
Michael5601
force-pushed
the
TipImageTests
branch
from
3bd4377 to
f199fc3
Compare
There was a problem hiding this comment.
The change looks good. I just wonder what effect this change should have on the bundle version. The change affects public API, which syntactically stays the same but the setExtension method was semantically correctly (in terms of Javadoc spec and implementation).
In a strict interpretation, I would say this requires a major version bump, but actually I don't think that makes sense for such a "correction"?
HeikoKlare
force-pushed
the
TipImageTests
branch
from
f199fc3 to
64d6254
Compare
…-platform#525 In this commit the method setExtension of TipImage.java is corrected to properly change the extension. Now if setExtension is called, not only the extension is updated but also the base64image because the extension is a part of this image. Mind that for this change the field fBase64Image can't be final because it needs to be updated when the extension changes. setExtension is not used in the constructor anymore to set the fExtension field, rather it is directly set. Also the tests setExtension and setExtension2 are reactivated and updated to test the functionality of setExtension. Contributes to eclipse-platform#525.
HeikoKlare
force-pushed
the
TipImageTests
branch
from
64d6254 to
229dbd5
Compare
There was a problem hiding this comment.
As already mentioned, these changes look good to me.
I see the slightly changed behavior of the setExtension method as a correction/improvement. The previous behavior was kind of broken since the stored file extension and the extension encoded into the image became different, which did not make sense. This can also be seen in the existing tests (which were commented out), as they specified the behavior as it is implemented after this change.
In this commit the method setExtension of TipImage.java is corrected to properly change the extension. Now if setExtension is called, not only the extension is updated but also the base64image because the extension is a part of this image. Mind that for this change the field fBase64Image can't be final because it needs to be updated when the extension changes. setExtension is not used in the constructor anymore to set the fExtension field, rather it is directly set. Also the tests setExtension and setExtension2 are reactivated and updated to test the functionality of setExtension. Contributes to #525.