PG1204 commented on PR #5146:
URL: https://github.com/apache/texera/pull/5146#issuecomment-4594937812
> Requesting changes — I think two of the points raised are worth addressing
before merge rather than leaving as follow-ups:
>
> 1. **Decide the operator's border color in one place.** Right now two
separate parts of the code set it, with the same look-up-and-repaint logic
duplicated in both. Consolidating it into a single decision (check validity
first, then saved status, then the default) removes the duplication and also
fixes the edge case where an operator that is both invalid and has a saved
"completed" status can show the wrong color depending on which code path
happens to run last.
> 2. **Make the two new tests check the operator's final border color**
rather than just whether a paint function was called. As written, one of them
would pass even if the fix were removed, so the regression tests don't yet
guard against the regression.
>
> Details are in the inline comments. The behavior fix itself is correct and
close — these are about making it robust and properly tested.
@Xiao-zhen-Liu
Both points addressed in the latest push.
Extracted applyOperatorBorder(operatorID) as the single decision point,
validity -> cached execution status -> default valid. Both
handleOperatorValidation and the operator-add subscription delegate to it. The
invalid + cached Completed case is now resolved structurally (the helper picks
red unconditionally for invalid), so the outcome no longer depends on which
stream fires last.
Rewrote the three existing tests to assert the actual rect.body/stroke on
the paper ("green" / "#CFCFCF" / "red"). Added a 4th test specifically for
invalid + cached Completed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
