Fix run failure when tabs are unviewed #4547

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@DominusDodo

DominusDodo commented Jun 24, 2016

Fixes jdf/processing.py#208

This affects Python mode, but the source of the bug is here. When some tabs have not been viewed after the sketch has been opened, the 'document' of the respective SketchCode is null. This is fine, just don't throw an error. This fix shouldn't have any negative effects.

Fix run failure when tabs are unviewed
This affects Python mode, but the source of the bug is here. When some
tabs have not been viewed after the sketch has been opened, the
'document' of the respective SketchCode is null. This is fine, just
don't throw an error. This fix shouldn't have any negative effects.
@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Jun 27, 2016

Contributor

I don't think this is a good patch, since it papers over an error without addressing the cause.

Contributor

jdf commented Jun 27, 2016

I don't think this is a good patch, since it papers over an error without addressing the cause.

@DominusDodo

This comment has been minimized.

Show comment
Hide comment
@DominusDodo

DominusDodo Jun 28, 2016

To clarify, the NullPointerException is raised because in this section

  private void loadSavedCode() {
    SketchCode[] code = sketch.getCode();
    for (int i=0; i<code.length; i++) {
      if (!code[i].getProgram().equals(code[i].getSavedProgram())) {
        code[i].setProgram(code[i].getSavedProgram());
        /* Wild Hack: set document to null so the text editor will refresh
           the program contents when the document tab is being clicked */
        code[i].setDocument(null);
      }
    }
    // this will update the current code
    setCode(sketch.getCurrentCode());
  }

the document is set to null in each SketchCode instance. This means that if the tab has not been viewed, it will have a document of value null. However, the value for the program string in the code[i] will still be set to the loaded value (from code[i].getSavedProgram()). I can't find any other possible reasons for document to be null but code[i] to have a value without prior errors, so catching and suppressing the NullPointerException shouldn't conceal any other errors.

To clarify, the NullPointerException is raised because in this section

  private void loadSavedCode() {
    SketchCode[] code = sketch.getCode();
    for (int i=0; i<code.length; i++) {
      if (!code[i].getProgram().equals(code[i].getSavedProgram())) {
        code[i].setProgram(code[i].getSavedProgram());
        /* Wild Hack: set document to null so the text editor will refresh
           the program contents when the document tab is being clicked */
        code[i].setDocument(null);
      }
    }
    // this will update the current code
    setCode(sketch.getCurrentCode());
  }

the document is set to null in each SketchCode instance. This means that if the tab has not been viewed, it will have a document of value null. However, the value for the program string in the code[i] will still be set to the loaded value (from code[i].getSavedProgram()). I can't find any other possible reasons for document to be null but code[i] to have a value without prior errors, so catching and suppressing the NullPointerException shouldn't conceal any other errors.

@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Jul 4, 2016

Contributor

Rather than catching the NullPointerException, the code quoted above should not set the document of each tab to null. Whatever the issue was that the hack addressed should be fixed.

Contributor

jdf commented Jul 4, 2016

Rather than catching the NullPointerException, the code quoted above should not set the document of each tab to null. Whatever the issue was that the hack addressed should be fixed.

@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Jul 4, 2016

Contributor

To clarify, the code that sets each tab to null is a bug. Catching NPE will silently obscure an infinite class of other bugs (current and future). It's not a good idea.

Contributor

jdf commented Jul 4, 2016

To clarify, the code that sets each tab to null is a bug. Catching NPE will silently obscure an infinite class of other bugs (current and future). It's not a good idea.

@DominusDodo

This comment has been minimized.

Show comment
Hide comment
@DominusDodo

DominusDodo Jul 5, 2016

So perhaps we should look to instead create a boolean flag for whether the
tab has been loaded or not?

On Tue, Jul 5, 2016 at 5:58 AM, Jonathan Feinberg notifications@github.com
wrote:

To clarify, the code that sets each tab to null is a bug. Catching NPE
will silently obscure an infinite class of other bugs (current and future).
It's not a good idea.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4547 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ANDjnM27XMmOXZekMjlTw3v__Rh5XPLlks5qSUmogaJpZM4I-I0h
.

So perhaps we should look to instead create a boolean flag for whether the
tab has been loaded or not?

On Tue, Jul 5, 2016 at 5:58 AM, Jonathan Feinberg notifications@github.com
wrote:

To clarify, the code that sets each tab to null is a bug. Catching NPE
will silently obscure an infinite class of other bugs (current and future).
It's not a good idea.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4547 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ANDjnM27XMmOXZekMjlTw3v__Rh5XPLlks5qSUmogaJpZM4I-I0h
.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jul 29, 2016

Member

@JakubValtar more quirkiness with Document/SketchCode/etc...

Member

benfry commented Jul 29, 2016

@JakubValtar more quirkiness with Document/SketchCode/etc...

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Jul 29, 2016

Contributor

@benfry Same problem as #4555, I think I'm going to roll back my change which caused both of these and figure out something better.

Contributor

JakubValtar commented Jul 29, 2016

@benfry Same problem as #4555, I think I'm going to roll back my change which caused both of these and figure out something better.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jul 29, 2016

Member

Closing for the reasons cited by @jdf; looking for a more comprehensive fix to #4555

Member

benfry commented Jul 29, 2016

Closing for the reasons cited by @jdf; looking for a more comprehensive fix to #4555

@benfry benfry closed this Jul 29, 2016

@tyfkda tyfkda referenced this pull request Jul 30, 2016

Closed

Fix Null Pointer Exception #4563

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment