Skip to content

Conversation

@astroshim
Copy link
Contributor

What is this PR for?

This PR fixes the problem that does not show the text result in the paragraph sometimes.

What type of PR is it?

Bug Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1078

How should this be tested?

Try run python code constantly like screenshot.

Screenshots (if appropriate)

  • before
    before

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@astroshim
Copy link
Contributor Author

It seems that the CI fail is related to #1084.

@astroshim astroshim closed this Jun 29, 2016
@astroshim astroshim reopened this Jun 29, 2016
@astroshim astroshim closed this Jun 29, 2016
@astroshim
Copy link
Contributor Author

re-trigger CI

@astroshim astroshim reopened this Jun 29, 2016
@astroshim
Copy link
Contributor Author

re-trigger CI

@astroshim astroshim closed this Jun 30, 2016
@astroshim astroshim reopened this Jun 30, 2016
@bzz
Copy link
Member

bzz commented Jul 1, 2016

So you think resultRefreshed condition is not useful check any more?

\cc @felizbear @corneadoug for review

@astroshim
Copy link
Contributor Author

@bzz Thank you for reviewing.
I think resultRefreshed condition causes bug and I don't know why it needs.
@corneadoug @felizbear please review.

@corneadoug
Copy link
Contributor

I'm currently writing a giant review as I'm testing things

@corneadoug
Copy link
Contributor

@bzz @astroshim
Took me quite some time to dig into this.

Removing resultRefreshed would have negative performances effect, its original goal is to not re-render things if there was no change. however its condition isn't that great and a better one would be: !angular.equals(data.paragraph.result, $scope.paragraph.result);

A quick fix is indeed to remove resultRefreshed, but from this condition only else if (newType === 'TEXT' && resultRefreshed) {

The overall issue is that the implementation of the streaming feature in interpreters for the TEXT result type is not consistent.

And since the issue is complex and need of lot of explanations, I will create a separate issue.

We can keep this PR as a quick fix

@astroshim
Copy link
Contributor Author

@corneadoug
I thought it might be impacted but I didn't find a problem so I was waiting these great review!

The implementation of the streaming feature in interpreters for the TEXT result type is already a problem because the paragraph results are cleared before rendering result in the $scope.renderText = function() { function.

Then in this PR, can I just remove resultRefreshed from else if (newType === 'TEXT' && resultRefreshed) {?

@corneadoug
Copy link
Contributor

@astroshim Yes, its the best way for now

@astroshim
Copy link
Contributor Author

@corneadoug Thank you for making clear. I fixed the code as you advised.

@astroshim
Copy link
Contributor Author

re-trigger CI

@astroshim astroshim closed this Jul 3, 2016
@astroshim astroshim reopened this Jul 3, 2016
@corneadoug
Copy link
Contributor

LGTM

@corneadoug
Copy link
Contributor

Merging if there is no more discussions

@bzz
Copy link
Member

bzz commented Jul 4, 2016

Thank you for great feedback @corneadoug and prompt fixes @astroshim

Looks great to me!

@corneadoug
Copy link
Contributor

@astroshim Can you rebase with master?

@astroshim
Copy link
Contributor Author

@corneadoug I rebased. thanks.

@asfgit asfgit closed this in a29fe14 Jul 5, 2016
PhilippGrulich pushed a commit to SWC-SENSE/zeppelin that referenced this pull request Aug 8, 2016
…mes.

### What is this PR for?
This PR fixes the problem that does not show the text result in the paragraph sometimes.

### What type of PR is it?
Bug Fix

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-1078

### How should this be tested?
Try run python code constantly like screenshot.

### Screenshots (if appropriate)
  - before
![before](https://cloud.githubusercontent.com/assets/3348133/16436829/214a2b4e-3ddc-11e6-9af2-2ee1d7e2cf96.gif)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: astroshim <hsshim@nflabs.com>

Closes apache#1104 from astroshim/ZEPPELIN-1078 and squashes the following commits:

e0044db [astroshim] rebase
189c5eb [astroshim] remove resultRefreshed from TEXT type only
ec44166 [astroshim] remove resultRefreshed from TEXT type only
edf2397 [astroshim] Merge branch 'master' into ZEPPELIN-1078
441357c [astroshim] remove resultRefreshed value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants