Ignore empty lines for line events #43031

katleiahramos · 2021-10-15T22:31:47Z

Ignores blank lines for line events

Skip.empty.lines.mov

Links

jira ticket: https://codedotorg.atlassian.net/browse/STAR-1779

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

maddiedierker · 2021-10-18T20:23:59Z

apps/src/p5lab/poetry/PoetryLibrary.js

@@ -299,11 +299,12 @@ export default class PoetryLibrary extends CoreLibrary {
  applyGlobalLineAnimation(renderInfo, frameCount) {
    // Add 2 so there's time before the first line and after the last line
    const framesPerLine = POEM_DURATION / (renderInfo.lines.length + 2);
+    const nonEmptyLines = renderInfo.lines.filter(line => line.text !== ''); // Ignore empty lines for line events


could line.text ever be null/undefined or empty but with whitespace " "? we might want to consider those cases anyways because "empty" could end up meaning any of those things in the future

ajpal · 2021-10-26T02:49:03Z

I think instead of filtering out the blank lines in applyGlobalLineEffect, it would be better to filter them out in executeDrawLoopAndCallbacks where we fire the line event callbacks. Basically, I think the blank lines should still be allowed to take up time in the poem animation (like a stanza pause), but they just shouldn't count for the line event block

katleiahramos · 2021-10-26T18:55:59Z

I think instead of filtering out the blank lines in applyGlobalLineEffect, it would be better to filter them out in executeDrawLoopAndCallbacks where we fire the line event callbacks. Basically, I think the blank lines should still be allowed to take up time in the poem animation (like a stanza pause), but they just shouldn't count for the line event block

@ajpal So I see we're already filtering lines using the property isPoemBodyLine. Does it make sense to mark that as false if the line is "blank" and then just let it get filtered out here?

 const poemLines = renderInfo.lines.filter(
        line => line.isPoemBodyLine
      );

ajpal · 2021-10-26T19:01:20Z

I think instead of filtering out the blank lines in applyGlobalLineEffect, it would be better to filter them out in executeDrawLoopAndCallbacks where we fire the line event callbacks. Basically, I think the blank lines should still be allowed to take up time in the poem animation (like a stanza pause), but they just shouldn't count for the line event block

@ajpal So I see we're already filtering lines using the property isPoemBodyLine. Does it make sense to mark that as false if the line is "blank" and then just let it get filtered out here?
 const poemLines = renderInfo.lines.filter(
        line => line.isPoemBodyLine
      );

Oh, yeah that would totally work! (i think)

katleiahramos · 2021-10-26T19:50:56Z

apps/src/p5lab/poetry/commands/utils.js

+export function containsAtLeastOneAlphaNumberic(string) {
+  return /^.*[a-zA-Z0-9èàùìòÈÀÒÙÌéáúíóÉÁÚÍÓëäüïöËÄÜÏÖêâûîôÊÂÛÎÔç'-]+.*$/.test(
+    string
+  );
+}


This feels like it should go into a global stringUtils type of file. I didn't see one, and so I put it here for now.

Maybe https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/utils.js ?

updated: 6184ff1

katleiahramos · 2021-10-26T19:52:25Z

apps/src/p5lab/poetry/PoetryLibrary.js

@@ -441,7 +441,7 @@ export default class PoetryLibrary extends CoreLibrary {
        x: PLAYSPACE_SIZE / 2,
        y: yCursor,
        size: lineSize,
-        isPoemBodyLine: true
+        isPoemBodyLine: utils.containsAtLeastOneAlphaNumberic(line) // Used to skip blank lines in animations


I almost wonder if we should rename isPoemBodyLine to canHaveEvent or something? I feel like that might be a more accurate representation of what it is/ what it's used for and then we might not need as much info in the comments. But doesn't have to be part of these changes.

katleiahramos requested a review from a team October 15, 2021 22:32

maddiedierker reviewed Oct 18, 2021

View reviewed changes

Check for at least one alphanumeric character

8b457b0

katleiahramos force-pushed the katleiah/handle-blank-lines-poetry branch from 1b3a723 to 8b457b0 Compare October 26, 2021 19:49

katleiahramos commented Oct 26, 2021

View reviewed changes

Move regex check to different utils file

6184ff1

ajpal approved these changes Oct 26, 2021

View reviewed changes

katleiahramos merged commit dd56c9b into staging Oct 26, 2021

katleiahramos deleted the katleiah/handle-blank-lines-poetry branch October 26, 2021 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore empty lines for line events #43031

Ignore empty lines for line events #43031

katleiahramos commented Oct 15, 2021

maddiedierker Oct 18, 2021

ajpal commented Oct 26, 2021

katleiahramos commented Oct 26, 2021

ajpal commented Oct 26, 2021 •

edited

katleiahramos Oct 26, 2021

ajpal Oct 26, 2021

katleiahramos Oct 26, 2021

katleiahramos Oct 26, 2021 •

edited

Ignore empty lines for line events #43031

Ignore empty lines for line events #43031

Conversation

katleiahramos commented Oct 15, 2021

Links

PR Checklist:

maddiedierker Oct 18, 2021

Choose a reason for hiding this comment

ajpal commented Oct 26, 2021

katleiahramos commented Oct 26, 2021

ajpal commented Oct 26, 2021 • edited

katleiahramos Oct 26, 2021

Choose a reason for hiding this comment

ajpal Oct 26, 2021

Choose a reason for hiding this comment

katleiahramos Oct 26, 2021

Choose a reason for hiding this comment

katleiahramos Oct 26, 2021 • edited

Choose a reason for hiding this comment

ajpal commented Oct 26, 2021 •

edited

katleiahramos Oct 26, 2021 •

edited