-
-
Notifications
You must be signed in to change notification settings - Fork 139
Update when figure changes regardless of revision #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this condition should be ( (!revisionDefined && !figureChanged) || (revisionDefined && !revisionChanged) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even clearer:
if(revisionDefined) { if (!revisionChanged) return; } else { if (!figureChanged) return; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh you want clear?
let revisionNotDefinedAndFigureNotChanged = !revisionDefined && !figureChanged; let revisionDefinedAndRevisionNotChanged = revisionDefined && !figureChanged; if (revisionNotDefinedAndFigureNotChanged || revisionDefinedAndRevisionNotChanged) { return; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or may as well just keep going here
let revisionNotDefinedAndFigureNotChangedOrRevisionDefinedAndRevisionNotChanged = revisionNotDefinedAndFigureNotChanged || revisionDefinedAndRevisionNotChanged; if (revisionNotDefinedAndFigureNotChangedOrRevisionDefinedAndRevisionNotChanged) { return; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#126 (comment) is great :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I was still holding on to an earlier viewpoint.
I'm 💃 on the current code actually. Sorry for allll the back and forth and thanks for continuing to push this thing in the right direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just hard to read because it's defining the do-nothing case. It might be more readable to say if(! (<define the positive case>) )...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mine can be simplified to
(!figureChanged || revisionDefined || revisionChanged))
Forgot all my propositional calculus, tools to the rescue :)
http://logictools.org/
(-figureChanged & (-revisionDefined | (revisionDefined & -revisionChanged)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tools showed that @nicolaskruchten's and mine are not equivalent. Upon further inspection, ((!revisionDefined && !figureChanged) || (revisionDefined && !revisionChanged)) fails the same test as #126 (comment). So I'm gonna use my original. The simplified version above was my error... Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, github didn't show the comments above until I refreshed, will merge...
56ba088 to
62c2b10
Compare
62c2b10 to
0108bbe
Compare
Uh oh!
There was an error while loading. Please reload this page.
Addresses #59
Tested with RCE