Add OriginAtCenter property to Ball #1823
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some descriptions still need to be updated. This code compiles but has not been tested.
This gives us consisentency with Scratch.
Some of the duplicated code will be replaced in a later commit. I had to make getLeftX() and getTopY() public instead of package-private because they were defined in an interface (MockSprite) that both MockBall and MockImageSprite implement. They have different superclasses, so I could not put the code there.
I also changed setXProperty() and setYProperty() to call mockCanvas.refreshForm() instead of mockCanvas.reorderComponents(). The latter is needed only when the Z-coordinate is changed.
This should be done regardless of other changes. The file animation.html still needs to be updated. I'm waiting until additional changes are made.
TODO in later commits: Update animation.html.
|
Can one of the admins verify this patch? |
|
Ellen,
bravo for working on this.
I won't have time to review it, but I'll try playing with it.
So test programs might be helpful.
== Hal
Hal Abelson
hal@mit.edu
Prof. of Comp. Sci. and Eng.
MIT Dept. of Elec. Eng. and Comp. Sci.
…On Fri, Sep 6, 2019 at 6:07 PM Ellen Spertus ***@***.***> wrote:
This addresses Issue 1777: Fix Ball so the x- and y-coordinates are at
the center <#1777>.
It also contains unrelated minor fixes, primarily to documentation.
An instance is running on http://spertus-ai3.appspot.com
I know it is a big PR. Would you like for me to add comments on GitHub or
provide my test programs?
------------------------------
You can view, comment on, or merge this pull request online at:
#1823
Commit Summary
- Allow Ball coordinates to be at center
- Change CoordsAtCorner to OriginAtCorner
- Hide OriginAtCorner in Blocks view
- Refactor mock components
- Rename OriginAtCorner to OriginAtCenter
- Make MockBall support OriginAtCenter
- Fix error resizing radius
- Update xLeft and yTop on Radius change
- Make code clearer
- Merge branch 'master' of github.com:mit-cml/appinventor-sources into
ball
- Specialize X and Y descriptions for Ball & Sprite
- Continue to specialize documentation
- Add missing documentation
- Improve comments
- Make drag work in Designer
- Merge branch 'ucr' of github.com:mit-cml/appinventor-sources into
ball
- Remove logging/throws that I put in
- Fix comment
- Clean up before PR
File Changes
- *M*
appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockBall.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-0>
(72)
- *M*
appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockCanvasLayout.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-1>
(31)
- *M*
appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockForm.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-2>
(4)
- *M*
appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockImageSprite.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-3>
(32)
- *M*
appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockSprite.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-4>
(36)
- *M*
appinventor/components/src/com/google/appinventor/components/runtime/Ball.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-5>
(88)
- *M*
appinventor/components/src/com/google/appinventor/components/runtime/Canvas.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-6>
(4)
- *M*
appinventor/components/src/com/google/appinventor/components/runtime/ImageSprite.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-7>
(63)
- *M*
appinventor/components/src/com/google/appinventor/components/runtime/Sprite.java
<https://github.com/mit-cml/appinventor-sources/pull/1823/files#diff-8>
(163)
Patch Links:
- https://github.com/mit-cml/appinventor-sources/pull/1823.patch
- https://github.com/mit-cml/appinventor-sources/pull/1823.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1823?email_source=notifications&email_token=AAGAXN4KAXT4OP7ULMR5B5TQILIA7A5CNFSM4IUNRJGKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HJ5CCYQ>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGAXN3TZZ4CDABUTFI5ILDQILIA7ANCNFSM4IUNRJGA>
.
|
|
@AppInventorWorkerBee ok to test |
|
Thanks @halatmit. I've attached a zip file with 4 test programs: two I wrote (BallDragTest and BallPlacementTest), one from the gallery that I modified (Pong) and one from the gallery that I didn't modify (BrickBreakFinal). The modification I made to Pong was to fix the drag issue I recently reported. I've also attached a companion APK if you don't want to build your own and trust me. |
ewpatton
requested changes
Sep 9, 2019
There was a problem hiding this comment.
A couple of minor things, but I'm wondering if we could make a change to the documentation generation that would make it easier to handle the component name in the description rather than having to duplicate all of the descriptions in each child class. Maps suffers a similar issue because most shared properties are defined in MapFeatureBase rather than the concrete classes. For example, we could allow for {type} as a placeholder and have the ComponentProcessor replace it with the class name.
appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockBall.java
Outdated
Show resolved
Hide resolved
appinventor/appengine/src/com/google/appinventor/client/editor/simple/components/MockBall.java
Outdated
Show resolved
Hide resolved
...or/appengine/src/com/google/appinventor/client/editor/simple/components/MockImageSprite.java
Outdated
Show resolved
Hide resolved
ewpatton
added
pull request: in review
and removed
pull request: needs reply to review
labels
ewpatton
requested changes
Sep 18, 2019
appinventor/components/src/com/google/appinventor/components/runtime/Sprite.java
Outdated
Show resolved
Hide resolved
|
is it centerAtOrigin or originAtCenter?
== Hal
Hal Abelson
hal@mit.edu
Prof. of Comp. Sci. and Eng.
MIT Dept. of Elec. Eng. and Comp. Sci.
…On Thu, Sep 19, 2019 at 12:25 AM Evan W. Patton ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I have one minor issue with the location of an @simplefunction
annotation. Otherwise, LGTM unless you are planning to make more changes.
------------------------------
In
appinventor/components/src/com/google/appinventor/components/runtime/Sprite.java
<#1823 (comment)>
:
> @@ -697,7 +752,12 @@ protected int hitEdge() {
* canvas. If the sprite is too tall to fit on the canvas, this aligns the
* top side of the sprite with the top side of the canvas.
*/
- @simplefunction
+ @simplefunction(
Technically only public methods should be marked @simplefunction. There's
a public version MoveIntoBounds on line 664 that covers this requirement,
so I think you can kill this annotation.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1823?email_source=notifications&email_token=AAGAXN5ZI7RJ4BI46FDGDI3QKKLD3A5CNFSM4IUNRJGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFGDCCY#pullrequestreview-290205963>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGAXN5SYZ23BHSNG3OPPCTQKKLD3ANCNFSM4IUNRJGA>
.
|
originAtCenter |
This has a known bug: The value of the Z property does not get initialized.
|
I have made the requested change. |
|
I'll do that @ewpatton |
|
I made the changes @ewpatton but think I must have done something wrong: When I try loading my old programs on the instance (spertus-ai3), I get the below error: |
|
Wait, I see a bug. |
|
I fixed the bug I was aware of, but I'm still getting the same error. |
|
The blocks upgrade is now handled in blocklyeditor/src/versioning.js. There's a dictionary mapping component to a version mapping of upgrades to perform. Since you've only added a property, you can specify "noUpgrade" for the corresponding new version number. |
|
Thanks. Is this documented anywhere, or should I add it?
…On Sun, Sep 22, 2019 at 7:36 PM Evan W. Patton ***@***.***> wrote:
The blocks upgrade is now handled in blocklyeditor/src/versioning.js.
There's a dictionary mapping component to a version mapping of upgrades to
perform. Since you've only added a property, you can specify "noUpgrade"
for the corresponding new version number.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1823?email_source=notifications&email_token=AAFBMQHG5UFSFEKNTB5OH23QLATUPA5CNFSM4IUNRJGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7JVQBY#issuecomment-533944327>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFBMQCBGQRVA4BJ6VAFAFLQLATUPANCNFSM4IUNRJGA>
.
|
|
@espertus AFAIK, it's only mentioned in YaVersion.java. It would be good to update the "How to add a property" doc with the new location. I've also started some new templates in #1850 that should guide people in the future as to the steps required to make a complete PR that more saliently outlines all of the steps. |
|
Thanks for the pointer. Please take another look. The latest version is running at http://spertus-ai3.appspot.com, as usual. |
|
Why not add CenterAtOrigin to ImageSprite? |
|
That is a more complicated issue that needs to be discussed. See the bug
report.
…On Mon, Sep 30, 2019 at 1:46 AM 皮皮蟹 ***@***.***> wrote:
Why not add CenterAtOrigin to ImageSprite?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1823?email_source=notifications&email_token=AAFBMQBYPFVHQ2AJRXOQDELQMG4HRA5CNFSM4IUNRJGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7447BQ#issuecomment-536465286>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFBMQHG2RRQGCPUIFTN6OTQMG4HRANCNFSM4IUNRJGA>
.
|
|
Got it, thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses Issue 1777: Fix Ball so the x- and y-coordinates are at the center. It also contains unrelated minor fixes, primarily to documentation.
An instance is running on http://spertus-ai3.appspot.com
I know it is a big PR. Would you like for me to add comments on GitHub or provide my test programs?