3

I'm working on a class that holds an Image and paints it centered on 0, 0; for this, it retrieves the height and width of the image and bases its display offset on those values. But, in making it an ImageObserver in case the image is not yet fully loaded, I'm leaking this in the constructor:

public class Sprite extends SimplePaintable implements ImageObserver {

    private final Image sprite;
    private int xOffset;
    private boolean xOffsetSet;
    private int yOffset;
    private boolean yOffsetSet;

    public Sprite(Image sprite) {
        this.sprite = sprite;
        //warning: leaking this in constructor
        int width = sprite.getWidth(this);
        xOffset = width/2;
        xOffsetSet = width != -1;
        int height = sprite.getHeight(this);
        yOffset = height/2;
        yOffsetSet = height != -1;
    }

    @Override
    public boolean imageUpdate(Image img, int infoflags, int x, int y, int width, int height) {
        assert img == sprite;
        if ((infoflags & WIDTH) != 0) {
            xOffset = width / 2;
            xOffsetSet = true;
        } 
        if ((infoflags & HEIGHT) != 0) {
            yOffset = height / 2;
            yOffsetSet = true;
        }
        return !(xOffsetSet && yOffsetSet);
    }
    ...

At first I thought this was fine, as only the offset variables were uninitialized and their default values were fine for (not) displaying an unloaded image, but then I realized that if the image loaded immediately as getWidth(this) was called, it could theoretically call imageUpdate before the constructor finished, causing the offsets to be set properly in imageUpdate, and then be unset by the constructor. Is this a concern, or are images only loaded synchronously on the EDT? If this is a concern, would making imageUpdate a synchronized method prevent it from running until the constructor finished?

8
  • No. It is not safe to pass this from within a constructor. The issue (as you surmised) is that your instance hasn't been initialized yet. Dec 25, 2014 at 21:16
  • If the Image is already loaded, observer can be null.
    – trashgod
    Dec 25, 2014 at 21:17
  • @ElliottFrisch It is not generally safe to do so; however, if Image.getWidth is guaranteed not to call imageUpdate until after the constructor is finished, then it is safe in this specific case.
    – Vitruvie
    Dec 25, 2014 at 21:19
  • @Saposhiente If you already know the answers, why did you ask the question? Dec 25, 2014 at 21:20
  • 1
    @Saposhiente It isn't. The Javadoc says Determines the width of the image. If the width is not yet known, this method returns -1 and the specified ImageObserver object is notified later. Dec 25, 2014 at 21:21

1 Answer 1

1

Leaking this in the constructor is only a warning, because it could lead to an issue. If variables of super are initialized, but this modifies them afterwards, yet super leaks itself, then someone is working with not yet fully initialized variables.

If you are sure that there will be no one accessing any variables while constructing, and there is no other way to do it (because i.E. external libraries are keeping you from doing it the right way), then it is ok to ignore that warning.

In your case it is theoretically possible, that during the call of sprite.getWidth(this) the image calls the observer to update the progress immediately, so imageUpdate is called before the constructor has finished. In this case the offset variables will be overwritten by the constructor initialization afterwards. And no, synchronized will not prevent the issue, as no one else is holding the lock during that moment.

There are several ways to work around this:

  • Use a BufferedImage, which does not require an observer for getWidth/getHeight.
    Downside: the image has to be fully loaded, which in some cases might introduce a small delay (if i.E. loaded over network).

  • User proper locking:

    private final Object offsetLock = new Object();
    
    public Sprite(Image sprite) {
        this.sprite = sprite;
    
        synchronized(offsetLock) {
            int width = sprite.getWidth(this);
            xOffset = width/2;
            xOffsetSet = width != -1;
            int height = sprite.getHeight(this);
            yOffset = height/2;
            yOffsetSet = height != -1;
        }
    }
    
    @Override
    public boolean imageUpdate(Image img, int infoflags, int x, int y, int width, int height) {
        assert img == sprite;
        if ((infoflags & WIDTH) != 0) {
            synchronized(offsetLock) {
                xOffset = width / 2;
                xOffsetSet = true;
            }
        } 
        if ((infoflags & HEIGHT) != 0) {
            synchronized(offsetLock) {
                yOffset = height / 2;
                yOffsetSet = true;
            }
        }
        return xOffsetSet && yOffsetSet;
    }
    

    Downside: this will call synchronized, which might cause a slight delay. You usually won't notice if this is called only a handful of times, but in a time-critical loop this might be noticeable.

  • Use an external function to update the data after construction has finished:

    public Sprite(Image sprite) {
        this.sprite = sprite;
    }
    
    protected updateOffsets() {
        updateWidth(sprite.getWidth(this));
        updateHeight(sprite.getHeight(this));
    }
    
    protected updateWidth(final int newWidth) {
        if (newWidth != -1) {
            xOffset = newWidth/2;
            xOffsetSet = true;
        }
    }
    
    protected updateHeight(final int newHeight) {
        if (newHeight!= -1) {
            yOffset = newHeight/2;
            yOffsetSet = true;
        }
    }
    
    @Override
    public boolean imageUpdate(Image img, int infoflags, int x, int y, int width, int height) {
        assert img == sprite;
        if ((infoflags & WIDTH) != 0) {
            updateWidth(width);
        } 
        if ((infoflags & HEIGHT) != 0) {
            updateHeight(height);
        }
        return xOffsetSet && yOffsetSet;
    }
    

    Downside: someone has to call the updateOffsets() method, and before that the object is not fully initialized, which might lead to bugs or requires you to write a builder method, further complicating things.

2
  • Decided to split the ImageObserver into a helper class to hide the imageUpdate method, check whether the Image received was a BufferedImage, and if not, to synchronize on the helper ImageObserver.
    – Vitruvie
    Dec 29, 2014 at 3:04
  • Also, I realized that the return value of imageUpdate was opposite of correct and edited it--you'll want to edit yours to match to prevent confusion.
    – Vitruvie
    Mar 24, 2015 at 1:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.