Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Temperament BugFix and ES6+ port #2772

Merged
merged 3 commits into from
Jan 27, 2021
Merged

Temperament BugFix and ES6+ port #2772

merged 3 commits into from
Jan 27, 2021

Conversation

ricknjacky
Copy link
Member

Issue reference:- #2767, #2630, #2609

Bug:-

When stop button is pressed, temperament widget continues to play the previous value of the pitch in conjunction to the pitch values from the start. Notice the erratic behavior in the video below.

Temprament-1.mp4

Obviation of bug post fixes:-

Temprament2.mp4

Post port, the behavior of rest of functionalities of the temperament widget:-

Temprament3.mp4

As is, this port didn't produce any errors for me on my system locally, however if any error/erratic behavior is produced due to this port, do let me know. Thanks

@ricknjacky
Copy link
Member Author

@meganindya please review this.

meganindya
meganindya requested changes Jan 25, 2021
View reviewed changes
Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

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

The bug is fixed but I can spot a few other crashes using some of the UI buttons in the widget. Please check.

temperament.js:1946 Uncaught TypeError: Cannot set property 'fillAttr' of undefined
    at __playLoop (temperament.js:1946)
    at temperament.js:2056
temperament.js:862 Uncaught ReferenceError: ratios is not defined
    at TemperamentWidget.performEqualEdit (temperament.js:862)
    at HTMLDivElement.<anonymous> (temperament.js:761)

@ricknjacky
Copy link
Member Author

The bug is fixed but I can spot a few other crashes using some of the UI buttons in the widget. Please check.

Thankyou for the feedback. I have rectified my errors:-
Post FIX behavior of UI buttons:-

Temprament-UI-Fixes.mp4

@ricknjacky ricknjacky requested a review from meganindya January 25, 2021 20:03
@ricknjacky
Copy link
Member Author

@meganindya In ad5bb2b I have added some of the JSDoc content. If this is fine by you, I'll add JSDoc styling comments for the entire file.
Looking forward to hear from you, Thanks.

@meganindya
Copy link
Member

Yeah pls do. I've fixed the linting errors and formatted the code. If possible, add a global locations comment like @ksraj123 has been doing (could be narrower with the whitespaces though) and remove dirty code if any.

@ricknjacky
Copy link
Member Author

Yeah pls do. I've fixed the linting errors and formatted the code. If possible, add a global locations comment like @ksraj123 has been doing (could be narrower with the whitespaces though) and remove dirty code if any.

Did the necessary changes. Any corrections?

@ricknjacky ricknjacky mentioned this pull request Jan 26, 2021
Closed
ricknjacky and others added 3 commits January 27, 2021 01:32
@ricknjacky @meganindya
Prototype function to class
01c9699 
Bugfix-1

Bugfix ratios

Bugfix for Ui elements
@ricknjacky @meganindya
Lint and add JSDoc
7c91e70 
Widgetblocks.js modification for logo

Linting finishes for temprament

JSDoc styling comments

Typo in JSDoc
@meganindya
Move init to top and format code
6a86702
meganindya
meganindya requested changes Jan 26, 2021
View reviewed changes
Copy link
Member

@meganindya meganindya left a comment

Choose a reason for hiding this comment

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

I clustered your commits and formatted the code. However, the widget shows anomalous behavior. Last time it was alright. Please check.

@ricknjacky
Copy link
Member Author

I clustered your commits and formatted the code.

Thanks

However, the widget shows anomalous behavior. Last time it was alright. Please check.

Tested again:-

Temp-Test.mp4

This time even with formatting changes that you have done, I did not encounter any error, can you please describe the anomalous behavior?

@meganindya
Copy link
Member

Can't reproduce it again.

@meganindya meganindya merged commit fc0ff8b into sugarlabs:master Jan 27, 2021
@ricknjacky ricknjacky deleted the Temprament-Fix branch January 27, 2021 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meganindya meganindya meganindya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ricknjacky @meganindya