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.

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

Turtles MVC component #2309

Merged
merged 8 commits into from
Jun 4, 2020
Merged

Conversation

meganindya
Copy link
Member

@meganindya meganindya commented Jun 2, 2020

Setup

  • Separate Turtles class in a separate file and load it in activity.js
  • Create TurtlesModel and TurtlesView classes for model and view respectively
  • Add feature to import members of TurtlesModel and TurtlesView into Turtles (controller)
  • Import all members of TurtlesModel and TurtlesView after instantiating them, and delete the separate model and view objects to free redundant memory

Separation

  • Transport model specific members to TurtlesModel
  • Transport view specific members to TurtlesView
  • Create getters to access members
  • Replace direct access by Turtles component members with getters
  • Rename tokens to clearly identify private members

@meganindya meganindya marked this pull request as ready for review June 2, 2020 16:37
@meganindya
Copy link
Member Author

This is mostly done except for the add function whose code needs to be broken down between M V and C.

@meganindya meganindya marked this pull request as draft June 3, 2020 08:50
@pikurasa
Copy link
Collaborator

pikurasa commented Jun 3, 2020

One test I did on your branch was to export lilypond.

I started from Walter's Rainbow Connection in Examples folder.

It returned this error, visible in console:

logo.js:4849 Uncaught ReferenceError: markup is not defined
    at Logo.updateNotation (logo.js:4849)
    at __playnote (note.js:1257)
    at Function._processNote (note.js:1980)
    at __listener (note.js:192)
    at a.b._dispatchEvent (easeljs.min.js:12)
    at a.b._dispatchEvent (easeljs.min.js:12)
    at a.b.dispatchEvent (easeljs.min.js:12)
    at Logo._runFromBlockNow (logo.js:1944)
    at logo.delayTimeout.(anonymous function).setTimeout (file:///home/devin/Computer-Code/musicblocks-all/walter-bender-musicblocks/js/logo.js:1544:26)

@meganindya
Copy link
Member Author

@pikurasa I looked into the code and faced another "there you are" moment. I'm sure there'll be more of these, now that I've ported the old syntax to ES6 classes: weird ways how old JavaScript works. Thanks for spotting this. Hopefully, the application's general behaviour will be more stable once I start working on logo.js.

@walterbender Since this is a minor one I'm pushing the commit in this PR. Yet another variable declaration bug.

@meganindya meganindya marked this pull request as ready for review June 3, 2020 14:36
@meganindya
Copy link
Member Author

meganindya commented Jun 3, 2020

Also, sometime later we should improve the complexity of some of the algorithms inside. This example crashes my browser every time. :P

@pikurasa
Copy link
Collaborator

pikurasa commented Jun 3, 2020

Also, sometime later we should improve the complexity of some of the algorithms inside. This examples crashes my browser every time.

It is a long project. It loads, but after about 30-60 seconds.

It would be nice, at the very least, if the user knew how much longer to wait until the project is fully loaded.

@walterbender walterbender merged commit 294002a into sugarlabs:master Jun 4, 2020
@meganindya meganindya changed the title Turtle MVC component Turtles MVC component Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants