Addition of lyrics through phase maker #4174

Commanderk3 · 2024-12-22T20:53:00Z

I was able to figure out how we can add print blocks to the block stack. But addition of a "true" print block in the phase maker widget and detecting it in the matrix is challenging. I spent almost 2 days on this problem but no results were produced.
Hence I am making it a draft PR for the interested one. They can continue my work and also I am ready to collab.

Things we can focus on :-

Modifying print block code. Orginally the phase maker only identifies notes and instruments like drum. No such implementation
for other blocks.
Removal of keyboard shorcuts while typing in the cell.
Here I have used an array to store lyrics. Is there any better option ?

@walterbender @pikurasa Sir please check this PR.

lyric.mp4

walterbender · 2024-12-22T22:06:04Z

Generally seems to work well. Not sure why the blocks are created when you add lyrics. But export is correct.
One small detail: it would be good to make the lyric block the same width as the other blocks in their columns.

Commanderk3 · 2024-12-23T06:25:59Z

Generally seems to work well. Not sure why the blocks are created when you add lyrics. But export is correct.
One small detail: it would be good to make the lyric block the same width as the other blocks in their columns.

Blocks are probably added because of keyboard shortcuts, just have to disable it like typing in the search box.

Originally Mr. Devin asked to initiate the lyrics row when print block is added to phrase maker widget. But in this PR their is no such implementation. Lyrics row will generate any way. I have to understand more deeply how it works.

Although I have added a Boolean lyricsON, if you think that initiating lyrics row in Matrix by adding a print block is not necessary then we can just use the Boolean to add print blocks in the generated action block.

haroon10725 · 2024-12-23T12:14:06Z

@Commanderk3 I would like to continue it.

walterbender · 2024-12-23T13:06:34Z

I realized after I typed my comment that the extra note blocks must be generated (as you had mentioned) by keyboard short cuts. We should probably disable keyboard shortcuts whenever any of the widgets are open.

I agree that the Lyrics row should not be a default. It should be driven by the presense of a print block. The glue for doing that is a bit messy, but you can follow the example for any of the graphics blocks.

Commanderk3 · 2024-12-23T15:25:16Z

@Commanderk3 I would like to continue it.

Sure, go ahead.

Commanderk3 · 2024-12-23T15:28:11Z

I realized after I typed my comment that the extra note blocks must be generated (as you had mentioned) by keyboard short cuts. We should probably disable keyboard shortcuts whenever any of the widgets are open.

I agree that the Lyrics row should not be a default. It should be driven by the presense of a print block. The glue for doing that is a bit messy, but you can follow the example for any of the graphics blocks.

Yes, I will look into it. The generation of cells excluding the first two is a bit tricky part. I will take my time on this one.

Commanderk3 · 2024-12-26T20:20:12Z

i wont say the commit "update 3" is perfect. A bit hardcoded. Just sharing my idea, still have some issues.
Cell width is now matching with others.

haroon10725 · 2024-12-27T01:44:22Z

@Commanderk3 I am working on those issues.

Commanderk3 · 2024-12-27T06:28:17Z

@walterbender

Okay I am satisfied with the recent commit. Everything is working fine.
What I did ?

Instead of pushing "print" into rowLabels it will now mark lyricsON true. Irrespective of print block position the lyrics row will be generated at the end.
And because it is not in rowLabels, no need to modify existing implemented functions like "Add notes" and "make clickable".

Also removed keyboard shortcuts while typing.

I have to make the formatting correct. I will update it after sometime and also will attach a video.

haroon10725 · 2024-12-27T07:00:57Z

@Commanderk3 Shouldn’t we add to rowLabels and rowArgs. Your implementation is not adding them in rowLabels and rowArgs array.

Commanderk3 · 2024-12-27T07:14:01Z

No problem. It works. Adding it to rowLabels means modifying existing functions. This approach is much simpler. And the lyrics row is anyway going to be at last.

If @walterbender and @pikurasa agree I shall make it a PR.

haroon10725 · 2024-12-27T07:54:02Z

But it can be confusing because in phrase maker we have print block, but it doesn't shows in rowArgs and rowLabels.

@walterbender What do you think about it?

Commanderk3 · 2024-12-27T09:23:59Z

But it can be confusing because in phrase maker we have print block, but it doesn't shows in rowArgs and rowLabels.

@walterbender What do you think about it?

Hmm...maybe. I actually prefer this one because of its simplicity. And everything works fine.

haroon10725 · 2024-12-27T11:24:31Z

I am working on this issue, and will come with a solution soon.

walterbender · 2024-12-27T12:41:33Z

js/blocks/ExtrasBlocks.js

@@ -534,6 +534,8 @@ function setupExtrasBlocks(activity) {
                        }
                    }
                }
+            } else if (logo.inMatrix) {


Why not move this else if above the previous one so you don't need the && !logo.inMatrix ?

Yes, i will do it. 👍

walterbender · 2024-12-27T12:42:13Z

js/widgets/phrasemaker.js

@@ -210,7 +210,7 @@ class PhraseMaker {

        // This array keeps track of the position of the rows after sorting.
        /**
-         * Map of row positions after sorting.
+         * Map of row positions after sorting. 


please remove trailing space

walterbender · 2024-12-27T12:44:29Z

js/widgets/phrasemaker.js

                            lastConnection = null;
                        } else {
-                            lastConnection = thisBlock + 3;
+                            lastConnection = thisBlock + 3; 


please remove trailing space

walterbender · 2024-12-27T12:45:22Z

js/widgets/phrasemaker.js

        ptmTableRow.insertCell().append(tempTable);

+        console.log(tempTable2.outerHTML);


Please remove console log in final PR (and extra new lines)

walterbender · 2024-12-27T12:45:38Z

js/widgets/phrasemaker.js

        this._tupletValueRow = tempTable.insertRow();
-        this._noteValueRow = tempTable.insertRow();
+        this._noteValueRow = tempTable.insertRow(); 


please remove trailing space

walterbender · 2024-12-27T12:46:29Z

js/widgets/phrasemaker.js

+         */
+        this._lyrics = [];
+
+        this._lyricsON = false;


Please don't use _ for fields that are accessed externally

walterbender · 2024-12-27T12:50:54Z

I am somewhat on the fence. Sidestepping the rowBlocks mechanism is simpler but I wonder if it doesn't add a different kind of complexity in that now there are two different mechanism for managing rows. That said, I think we can live with this implementation and perhaps make the change at some point in a different PR.

But we should add a comment in the code explaining that the lyrics are handled differently. Also a comment about disabling keyboard shortcuts and why.

Commanderk3 · 2024-12-27T13:18:03Z

Roger that. Formatting I will fix now and other changes that you said.

Commanderk3 · 2024-12-27T14:05:34Z

@walterbender I have fixed the issues you mentioned.

walterbender · 2024-12-27T17:33:12Z

js/blocks/WidgetBlocks.js

@@ -1430,6 +1430,7 @@ function setupWidgetBlocks(activity) {
            logo.phraseMaker.rowArgs = [];
            logo.phraseMaker.graphicsBlocks = [];
            logo.phraseMaker.clearBlocks();
+            logo.phraseMaker._lyricsON = false;


This needs to change to logo.phraseMaker.lyricsOn = false;

(Did you test after your updates?)

walterbender · 2024-12-27T17:33:38Z

js/blocks/ExtrasBlocks.js

@@ -508,6 +508,8 @@ function setupExtrasBlocks(activity) {
                    logo.oscilloscopeTurtles.indexOf(activity.turtles.turtleList[turtleIndex]) < 0
                )
                    logo.oscilloscopeTurtles.push(activity.turtles.turtleList[turtleIndex]);
+            } else if (logo.inMatrix) {
+                logo.phraseMaker._lyricsON = true;


This needs to change (remove the _)

walterbender · 2024-12-27T17:34:00Z

js/widgets/phrasemaker.js

@@ -911,8 +926,78 @@ class PhraseMaker {
            this._rows[j] = ptmRow;

            j += 1;
-        }
+        }       


please remove trailing spaces

walterbender · 2024-12-27T17:34:22Z

js/widgets/phrasemaker.js

+
+                const lyricsInput = document.createElement("input");
+                lyricsInput.type = "text";
+


please remove the extra newline

walterbender · 2024-12-27T17:35:02Z

js/widgets/phrasemaker.js

@@ -5036,7 +5122,7 @@ class PhraseMaker {
                    if (obj === null) {
                        // add a hertz block
                        // The last connection in last pitch block is null.
-                        if (note[0].length === 1 || j === note[0].length - 1) {
+                        if (!this._lyricsON && (note[0].length === 1 || j === note[0].length - 1)) {


walterbender · 2024-12-27T17:35:09Z

js/widgets/phrasemaker.js

@@ -5061,7 +5147,7 @@ class PhraseMaker {
                    } else if (drumName != null) {
                        // add a playdrum block
                        // The last connection in last pitch block is null.
-                        if (note[0].length === 1 || j === note[0].length - 1) {
+                        if (!this._lyricsON && (note[0].length === 1 || j === note[0].length - 1)) {


walterbender · 2024-12-27T17:35:15Z

js/widgets/phrasemaker.js

@@ -5086,7 +5172,7 @@ class PhraseMaker {
                    } else if (note[0][j].slice(0, 4) === "http") {
                        // add a playdrum block with URL
                        // The last connection in last pitch block is null.
-                        if (note[0].length === 1 || j === note[0].length - 1) {
+                        if (!this._lyricsON && (note[0].length === 1 || j === note[0].length - 1)) {


walterbender · 2024-12-27T17:35:28Z

js/widgets/phrasemaker.js

@@ -5111,7 +5197,7 @@ class PhraseMaker {
                    } else if (obj.length > 2) {
                        // add a 2-arg graphics block
                        // The last connection in last pitch block is null.
-                        if (note[0].length === 1 || j === note[0].length - 1) {
+                        if (!this._lyricsON && (note[0].length === 1 || j === note[0].length - 1)) {


walterbender · 2024-12-27T17:35:35Z

js/widgets/phrasemaker.js

@@ -5143,7 +5229,7 @@ class PhraseMaker {
                    } else if (obj.length > 1) {
                        // add a 1-arg graphics block
                        // The last connection in last pitch block is null.
-                        if (note[0].length === 1 || j === note[0].length - 1) {
+                        if (!this._lyricsON && (note[0].length === 1 || j === note[0].length - 1)) {


walterbender · 2024-12-27T17:35:43Z

js/widgets/phrasemaker.js

@@ -5168,7 +5254,7 @@ class PhraseMaker {
                    } else {
                        // add a pitch block
                        // The last connection in last pitch block is null.
-                        if (note[0].length === 1 || j === note[0].length - 1) {
+                        if (!this._lyricsON && (note[0].length === 1 || j === note[0].length - 1)) {


walterbender · 2024-12-27T17:35:49Z

js/widgets/phrasemaker.js

+                        }                    
+                    }
+                }
+                if (this._lyricsON) {


walterbender · 2024-12-27T17:36:50Z

Please test before pushing your changes to the PR.

Commanderk3 · 2024-12-27T18:15:47Z

Please test before pushing your changes to the PR.

My apologies sir. I did test, somehow it worked. I will take some time, right now I am traveling.

Commanderk3 added 3 commits December 23, 2024 01:33

No clear space

b6ad95f

Lyrics row in Phrase Maker

bcdde4b

Addition of lyric row in phrase maker widget

1c7d392

Commanderk3 marked this pull request as ready for review December 23, 2024 07:53

Commanderk3 marked this pull request as draft December 23, 2024 07:57

Commanderk3 added 2 commits December 25, 2024 21:13

update 2

dff0a96

update 3

d4faf70

final

d7f73a2

walterbender reviewed Dec 27, 2024

View reviewed changes

Commanderk3 added 3 commits December 27, 2024 18:57

formatting fix

3e34753

fix 2

1ef93a1

final fix

b0518c1

Commanderk3 marked this pull request as ready for review December 27, 2024 14:05

walterbender reviewed Dec 27, 2024

View reviewed changes

js/widgets/phrasemaker.js

}

}

}

if (this._lyricsON) {

Copy link

Member

walterbender Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_

		ptmTableRow.insertCell().append(tempTable);

		console.log(tempTable2.outerHTML);


		const lyricsInput = document.createElement("input");
		lyricsInput.type = "text";

Addition of lyrics through phase maker #4174

Are you sure you want to change the base?

Addition of lyrics through phase maker #4174

Conversation

Commanderk3 commented Dec 22, 2024 • edited Loading

walterbender commented Dec 22, 2024

Commanderk3 commented Dec 23, 2024

haroon10725 commented Dec 23, 2024

walterbender commented Dec 23, 2024

Commanderk3 commented Dec 23, 2024

Commanderk3 commented Dec 23, 2024

Commanderk3 commented Dec 26, 2024 • edited Loading

haroon10725 commented Dec 27, 2024

Commanderk3 commented Dec 27, 2024

haroon10725 commented Dec 27, 2024

Commanderk3 commented Dec 27, 2024 • edited Loading

haroon10725 commented Dec 27, 2024 • edited Loading

Commanderk3 commented Dec 27, 2024 • edited Loading

haroon10725 commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterbender Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

walterbender commented Dec 27, 2024

Commanderk3 commented Dec 27, 2024

Commanderk3 commented Dec 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterbender commented Dec 27, 2024

Commanderk3 commented Dec 27, 2024

Commanderk3 commented Dec 22, 2024 •

edited

Loading

Commanderk3 commented Dec 26, 2024 •

edited

Loading

Commanderk3 commented Dec 27, 2024 •

edited

Loading

haroon10725 commented Dec 27, 2024 •

edited

Loading

Commanderk3 commented Dec 27, 2024 •

edited

Loading

walterbender Dec 27, 2024 •

edited

Loading