Bug Fix, Temperament Widget: Play and Stop not working properly #2845

daksh4469 · 2021-02-15T17:17:58Z

Issue Reference: #2767
There were some bugs in this widget, all connected to the Play and Stop functionality.

Firstly, if the STOP button was clicked on a certain note (let's say 3), it would not reflect that as the same in the UI. Instead, it would appear to be stopping on the next note (i.e., in this case, 4):

screen-capture5.mp4

Secondly, if the widget was closed while it was still playing, the sound of the same note would still be heard (once again), even after closing:

screen-capture6.mp4

Lastly, if the process was Stopped in the counter-clockwise cycle, i.e., when this.playbackForward = false, the stopping functionality would not be reflected in the UI and thus would result in a regression:

screen-capture7.mp4

This PR solves these bugs as shown below (all solved bugs are shown in the sequence as they are mentioned above) :

screen-capture8.mp4

daksh4469 · 2021-02-15T17:19:58Z

Please review @meganindya @walterbender.
Thanks.

walterbender · 2021-02-15T22:30:10Z

js/widgets/temperament.js

@@ -2005,7 +2005,10 @@ class TemperamentWidget {
        let p = 0;
        this.playbackForward = true;
        this._playing = !this._playing;
-
+        if (!this._playing) {
+            logo.synth.setMasterVolume(0);


I am not sure we want to mess with the Master volume. It may have broader impacts. Why not just exit the loop? (See the mode widget, that does something similar.)

Yeah, just returning out of the loop would work perfectly.
Thanks.

I made this commit. Please review again.
Thanks.

walterbender · 2021-02-15T22:31:55Z

js/widgets/temperament.js

@@ -2199,6 +2199,20 @@ class TemperamentWidget {
                                this.notesCircle.navItems[i - 1].slicePathAttr.fill = "#c8C8C8";
                                this.notesCircle.navItems[i - 1].sliceSelectedAttr.fill =
                                    "#c8C8C8";
+                                if(i==11){


please add spaces around operators

walterbender · 2021-02-15T22:32:07Z

js/widgets/temperament.js

+                                    this.notesCircle.navItems[0].slicePathAttr.fill = "#c8C8C8";
+                                    this.notesCircle.navItems[0].sliceSelectedAttr.fill = "#c8C8C8";
+                                }
+                                else if(i<11){


meganindya

temperament.js:1767 Uncaught TypeError: Assignment to constant variable.
    at TemperamentWidget._save (temperament.js:1767)
    at HTMLDivElement.widgetWindow.addButton.onclick (temperament.js:130)

Also, stop button doesn't reset after playing all the notes up and down is over.

daksh4469 · 2021-02-16T08:37:56Z

temperament.js:1767 Uncaught TypeError: Assignment to constant variable.
    at TemperamentWidget._save (temperament.js:1767)
    at HTMLDivElement.widgetWindow.addButton.onclick (temperament.js:130)

Also, stop button doesn't reset after playing all the notes up and down is over.

I fixed this. Please check and review @meganindya

meganindya

this isn't fixed

temperament.js:1767 Uncaught TypeError: Assignment to constant variable.
    at TemperamentWidget._save (temperament.js:1767)
    at HTMLDivElement.widgetWindow.addButton.onclick (temperament.js:130)

temperament.js:1471 Uncaught TypeError: Cannot read property 'addEventListener' of null
    at temperament.js:1471

daksh4469 · 2021-02-17T07:51:12Z

this isn't fixed

The error Uncaught TypeError: Assignment to constant variable. is occurring due to a global variable OCTAVERATIO which is defined in musicutils.js file. I tried changing its definition from const to let but did not work.
I am trying to figure this out.

Also,can you specify when does this second error occur, because this event listener seems to work fine and it also generates the arbitraryEditSlider on mouseover.

daksh4469 · 2021-02-17T08:06:50Z

Hi @meganindya,
I fixed the first error, i.e., Uncaught TypeError: Assignment to constant variable by declaring the OctaveRatio in the temperament.js file itself, as there was no need to use the global OCTAVERATIO from musicutils.js . Now, the save functionality works fine.
Now, it is generating the same error for the same variable OCTAVERATIO in blocks.js file. And on solving this similarly, it is generating another 2 errors in temperament.js files,which are, Uncaught TypeError: t is undefined temperament.js:140:9 and Uncaught TypeError: this.notesCircle is undefined temperament.js:84:17.

Can you verify if this is happening in your environment setup too or is it just in mine?

vaibhavdaren · 2021-02-17T20:04:04Z

js/widgets/temperament.js

@@ -1760,8 +1763,7 @@ class TemperamentWidget {
            }
        }

-        // Global value
-        OCTAVERATIO = this.powerBase;
+        const OctaveRatio = this.powerBase;


global constant ? simple rename?

Yeah, it worked here perfectly.
Actually, making the variable OCTAVERATIO as let in the musicutils.js file didn't work. Thus, this step.

vaibhavdaren · 2021-02-17T20:04:59Z

js/widgets/temperament.js

+                        if (i !== -1) {
+                            if (this.circleIsVisible == false && docById("wheelDiv4") == null) {
+                                this.notesCircle.navItems[i - 1].fillAttr = "#c8C8C8";
+                                this.notesCircle.navItems[i - 1].sliceHoverAttr.fill = "#c8C8C8";


c8C8C8 should be a constant

But, how will it be beneficial?

One advantage of using a constant symbol instead of a magic number/string is that you can express the semantics of the value more precisely.

vaibhavdaren · 2021-02-17T20:07:06Z

js/widgets/temperament.js

+                                this.circleIsVisible == true &&
+                                docById("wheelDiv4") == null
+                            ) {
+                                j = i - 1;


omit variable

Can you please elaborate?

this variable can be omitted, it's a bad practice to declare temp variables if they are used only once.
i,j => naming should be meaningful for other people who read your code.

vaibhavdaren · 2021-02-17T20:12:35Z

js/widgets/temperament.js

+                                    //on completion of a full circle and on hitting '0' note in clockwise direction
+                                    this.notesCircle.navItems[0].fillAttr = "#c8C8C8";
+                                    this.notesCircle.navItems[0].sliceHoverAttr.fill = "#c8C8C8";
+                                    this.notesCircle.navItems[0].slicePathAttr.fill = "#c8C8C8";
+                                    this.notesCircle.navItems[0].sliceSelectedAttr.fill = "#c8C8C8";


at a particular position(i) you are setting value of nav items to a particular colour.
You can use a function. which changes this.notesCircle.navItems[pos].fillAttr = COLOUR and call it in the code.
It will be a lot cleaner and maintainable in future.

setNotesCircleColour(pos,colour)

Yeah, I will work on this.

vaibhavdaren

can improve on code quality

daksh4469 · 2021-02-18T13:26:55Z

Can you please review it again? @vaibhavdaren

vaibhavdaren · 2021-02-18T16:09:04Z

js/widgets/temperament.js

+                                this.wheel1.navItems[i - 1].fillAttr = TemperamentWidget.LIGHTGREY;
+                                this.wheel1.navItems[i - 1].sliceHoverAttr.fill =
+                                    TemperamentWidget.LIGHTGREY;
+                                this.wheel1.navItems[i - 1].slicePathAttr.fill =
+                                    TemperamentWidget.LIGHTGREY;
+                                this.wheel1.navItems[i - 1].sliceSelectedAttr.fill =
+                                    TemperamentWidget.LIGHTGREY;


missed function call?

No, actually the function updateNotesCircle works on notesCircle, whereas these lines use wheel1 and this only happens once in the whole code. Thus, a different function would have to be made for wheel1 which would not be that useful given that this function would have to be called just once.

This reverts commit aadd943.

daksh4469 · 2021-03-02T14:25:56Z

I did a rebase of this branch after the above-mentioned error by @meganindya was resolved by @ricknjacky in #2862. Now, everything works as expected in the Temperament Widget. Play and stop functionality also works as expected. Also, these commits also cover the changes mentioned in reviews by @vaibhavdaren. I also reverted one commit which involved changes in variable OCTAVERATIO to avoid any inconsistency.
Please have a look at this @walterbender and share your thoughts.

walterbender reviewed Feb 15, 2021

View reviewed changes

meganindya requested changes Feb 16, 2021

View reviewed changes

daksh4469 requested a review from meganindya February 16, 2021 12:26

meganindya requested changes Feb 17, 2021

View reviewed changes

vaibhavdaren reviewed Feb 17, 2021

View reviewed changes

vaibhavdaren suggested changes Feb 17, 2021

View reviewed changes

vaibhavdaren reviewed Feb 18, 2021

View reviewed changes

daksh4469 added 12 commits March 2, 2021 19:13

Temperament: Bug Fix on stopping

2cc1971

Bug Fix on closing the temperament widget

d2b0dcf

Bug Fix in case of counter-clockwise direction

3573427

Fix error in case of stoppin and closing

280d1b9

Removed the step to set the volume to 0

a9d3f02

Fix error on completion of two cycles

4f92613

Remove log statement

2641416

Remove unneccesary code

c48a518

Correct a mistake

31d25c6

Fix Error: Assignment to constant variable

aadd943

Assign colours to static variables

89ed125

Make function updateNotesCircle() to remove repeatition in code

0140cdf

daksh4469 force-pushed the daksh4469-main2 branch from 8d39d4d to 0140cdf Compare March 2, 2021 14:00

Revert "Fix Error: Assignment to constant variable"

a9b0afa

This reverts commit aadd943.

walterbender merged commit c6bb32d into sugarlabs:master Mar 2, 2021

daksh4469 deleted the daksh4469-main2 branch March 2, 2021 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Fix, Temperament Widget: Play and Stop not working properly #2845

Bug Fix, Temperament Widget: Play and Stop not working properly #2845

daksh4469 commented Feb 15, 2021

daksh4469 commented Feb 15, 2021

walterbender Feb 15, 2021

daksh4469 Feb 16, 2021

daksh4469 Feb 16, 2021

walterbender Feb 15, 2021

walterbender Feb 15, 2021

meganindya left a comment

daksh4469 commented Feb 16, 2021

meganindya left a comment

daksh4469 commented Feb 17, 2021 •

edited

Loading

daksh4469 commented Feb 17, 2021 •

edited

Loading

vaibhavdaren Feb 17, 2021

daksh4469 Feb 18, 2021

vaibhavdaren Feb 17, 2021

daksh4469 Feb 18, 2021

vaibhavdaren Feb 18, 2021

daksh4469 Feb 18, 2021

vaibhavdaren Feb 17, 2021

daksh4469 Feb 18, 2021

vaibhavdaren Feb 18, 2021

daksh4469 Feb 18, 2021

vaibhavdaren Feb 17, 2021

daksh4469 Feb 18, 2021

daksh4469 Feb 18, 2021

vaibhavdaren left a comment

daksh4469 commented Feb 18, 2021

vaibhavdaren Feb 18, 2021

daksh4469 Feb 18, 2021

daksh4469 commented Mar 2, 2021

Bug Fix, Temperament Widget: Play and Stop not working properly #2845

Bug Fix, Temperament Widget: Play and Stop not working properly #2845

Conversation

daksh4469 commented Feb 15, 2021

daksh4469 commented Feb 15, 2021

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

meganindya left a comment

Choose a reason for hiding this comment

daksh4469 commented Feb 16, 2021

meganindya left a comment

Choose a reason for hiding this comment

daksh4469 commented Feb 17, 2021 • edited Loading

daksh4469 commented Feb 17, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaibhavdaren left a comment

Choose a reason for hiding this comment

daksh4469 commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daksh4469 commented Mar 2, 2021

daksh4469 commented Feb 17, 2021 •

edited

Loading

daksh4469 commented Feb 17, 2021 •

edited

Loading