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

CATROID-825 "Set thread color to __" Brick #3942

Merged
merged 1 commit into from
Apr 20, 2021
Merged

CATROID-825 "Set thread color to __" Brick #3942

merged 1 commit into from
Apr 20, 2021

Conversation

Rawbat
Copy link
Contributor

@Rawbat Rawbat commented Jan 17, 2021 •
edited
Loading

Added brick and embroidery support for it. Also changed all tests affected by the change to include a color parameter.

Your checklist for this pull request

Please review the contributing guidelines and wiki pages of this repository.

  • Include the name of the Jira ticket in the PR’s title
  • Include a summary of the changes plus the relevant context
  • Choose the proper base branch (develop)
  • Confirm that the changes follow the project’s coding guidelines
  • Verify that the changes generate no compiler or linter warnings
  • Perform a self-review of the changes
  • Verify to commit no other files than the intentionally changed ones
  • Include reasonable and readable tests verifying the added or changed behavior
  • Confirm that new and existing unit tests pass locally
  • Check that the commits’ message style matches the project’s guideline
  • Stick to the project’s gitflow workflow
  • Verify that your changes do not have any conflicts with the base branch
  • After the PR, verify that all CI checks have passed
  • Post a message in the catroid-stage or catroid-ide Slack channel and ask for a code reviewer

@Rawbat Rawbat force-pushed the CATROID-825 branch 4 times, most recently from 893627e to 0732787 Compare January 18, 2021 19:18
@Rawbat Rawbat force-pushed the CATROID-825 branch 2 times, most recently from adc187b to 665aaca Compare February 3, 2021 18:38
wslany
wslany requested changes Feb 23, 2021
View reviewed changes
Copy link
Member

@wslany wslany left a comment

Choose a reason for hiding this comment

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

Please increase the language version number.

Please change the parameter to a single string in hex-color format instead of the separate RGB colors.

@Rawbat Rawbat force-pushed the CATROID-825 branch 2 times, most recently from d650218 to af76be9 Compare February 26, 2021 16:20
@Rawbat Rawbat requested a review from adnaribo February 26, 2021 16:21
adnaribo
adnaribo requested changes Mar 7, 2021
View reviewed changes
catroid/src/main/java/org/catrobat/catroid/ui/fragment/CategoryBricksFactory.java Outdated
@@ -841,6 +842,7 @@
private List<Brick> setupEmbroideryCategoryList(Context context) {
List<Brick> embroideryBrickList = new ArrayList<>();
embroideryBrickList.add(new StitchBrick());
embroideryBrickList.add(new SetThreadColorBrick(new Formula("0xff0000")));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a constant here, like for the other bricks

@Rawbat Rawbat force-pushed the CATROID-825 branch 2 times, most recently from 2006f01 to 56234d4 Compare March 7, 2021 20:42
@Rawbat Rawbat requested a review from adnaribo March 7, 2021 20:42
wslany
wslany requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@wslany wslany left a comment

Choose a reason for hiding this comment

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

Cannot build because of conversion error, see in-context comment.

catroid/src/main/java/org/catrobat/catroid/content/actions/SetThreadColorAction.java Outdated
try {
String colorStringInterpretation = "0xff0000";
if (color != null) {
colorStringInterpretation = color.interpretString(sprite);
Copy link
Member

Choose a reason for hiding this comment

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

Error, see https://jenkins.catrob.at/job/Catroid/job/PR-3942/32/execution/node/54/log/

21:42:56  > Task :catroid:compileStandaloneDebugJavaWithJavac
21:42:56  /home/catroid/jenkins_slave/workspace/Catroid_PR-
      3942/catroid/src/main/java/org/catrobat/catroid/content/actions/SetThreadColorAction.java:44: 
      error: incompatible types: Sprite cannot be converted to Scope
21:42:56  				colorStringInterpretation = color.interpretString(sprite);

@wslany
Copy link
Member

wslany commented Mar 18, 2021

Please also include my merge-conflict resolution commit for the Catrobat language version increase.

@Rawbat Rawbat force-pushed the CATROID-825 branch 3 times, most recently from d5c3e19 to 43fad6e Compare March 20, 2021 12:14
@Rawbat Rawbat requested a review from wslany March 20, 2021 12:36
wslany
wslany requested changes Mar 20, 2021
View reviewed changes
Copy link
Member

@wslany wslany left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately a crash happens when the color is not formatted as expected, and especially when it comes from the color picker: enter any color with the color picker in the formula editor:
image

After selecting the color, it looks like this:
image

The crash happens in SetThreadColorAction.java:49

Stacktrace:

03-20 19:26:10.909 E/AndroidRuntime(18805): FATAL EXCEPTION: GLThread 6642
03-20 19:26:10.909 E/AndroidRuntime(18805): Process: org.catrobat.catroid.independent_20210301_2128, PID: 18805
03-20 19:26:10.909 E/AndroidRuntime(18805): java.lang.StringIndexOutOfBoundsException: length=7; index=8
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at java.lang.String.substring(String.java:2032)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at org.catrobat.catroid.content.actions.SetThreadColorAction.update(SetThreadColorAction.java:49)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at com.badlogic.gdx.scenes.scene2d.actions.TemporalAction.act(TemporalAction.java:55)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at com.badlogic.gdx.scenes.scene2d.actions.SequenceAction.act(SequenceAction.java:65)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at org.catrobat.catroid.common.ThreadScheduler.runThreadsForOneTick(ThreadScheduler.java:77)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at org.catrobat.catroid.common.ThreadScheduler.tick(ThreadScheduler.java:62)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at org.catrobat.catroid.content.Look.act(Look.java:207)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at com.badlogic.gdx.scenes.scene2d.Group.act(Group.java:49)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at com.badlogic.gdx.scenes.scene2d.Stage.act(Stage.java:222)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at org.catrobat.catroid.stage.StageListener.render(StageListener.java:542)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at com.badlogic.gdx.backends.android.AndroidGraphics.onDrawFrame(AndroidGraphics.java:494)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at android.opengl.GLSurfaceView$GLThread.guardedRun(GLSurfaceView.java:1571)
03-20 19:26:10.909 E/AndroidRuntime(18805): 	at android.opengl.GLSurfaceView$GLThread.run(GLSurfaceView.java:1270)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): uncaughtException:
03-20 19:26:10.909 E/BaseExceptionHandler(18805): java.lang.StringIndexOutOfBoundsException: length=7; index=8
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at java.lang.String.substring(String.java:2032)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at org.catrobat.catroid.content.actions.SetThreadColorAction.update(SetThreadColorAction.java:49)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at com.badlogic.gdx.scenes.scene2d.actions.TemporalAction.act(TemporalAction.java:55)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at com.badlogic.gdx.scenes.scene2d.actions.SequenceAction.act(SequenceAction.java:65)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at org.catrobat.catroid.common.ThreadScheduler.runThreadsForOneTick(ThreadScheduler.java:77)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at org.catrobat.catroid.common.ThreadScheduler.tick(ThreadScheduler.java:62)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at org.catrobat.catroid.content.Look.act(Look.java:207)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at com.badlogic.gdx.scenes.scene2d.Group.act(Group.java:49)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at com.badlogic.gdx.scenes.scene2d.Stage.act(Stage.java:222)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at org.catrobat.catroid.stage.StageListener.render(StageListener.java:542)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at com.badlogic.gdx.backends.android.AndroidGraphics.onDrawFrame(AndroidGraphics.java:494)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at android.opengl.GLSurfaceView$GLThread.guardedRun(GLSurfaceView.java:1571)
03-20 19:26:10.909 E/BaseExceptionHandler(18805): 	at android.opengl.GLSurfaceView$GLThread.run(GLSurfaceView.java:1270)

Kotlin might have helped, as it is more difficult to leave exceptions uncaught.

One more issue: The selected color incorrectly persists over runs of the stage. For instance, look at the following code. The color of the thread in the first run is black for 5 seconds, then it switches to red. If you exit the stage and run the stage again, the thread will be red from the beginning. Only after you left the project and then restarted it completely will it revert to black.
image

catroid/src/main/java/org/catrobat/catroid/common/BrickValues.java Outdated
@@ -180,6 +180,7 @@
public static final int STITCH_LENGTH = 10;
public static final int ZIGZAG_STITCH_LENGTH = 2;
public static final int ZIGZAG_STITCH_WIDTH = 10;
public static final String THREAD_COLOR = "0xff0000";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static final String THREAD_COLOR = "0xff0000";
public static final String THREAD_COLOR = "#ff0000";

We use the #XXXXXX format for colors in Catrobat, see all other color related bricks, and that's also what the color picker returns.

@Rawbat Rawbat force-pushed the CATROID-825 branch 2 times, most recently from 2bd7bd0 to 5b5ffee Compare March 20, 2021 20:00
CATROID-825 "Set thread color to __" Brick
41a64b9 
Added Brick and Embroidery functionality. Also changed all tests regarding this.
@Rawbat Rawbat requested a review from adnaribo March 22, 2021 14:16
@wslany wslany merged commit 515777a into Catrobat:develop Apr 20, 2021
@wslany
Copy link
Member

wslany commented Apr 20, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adnaribo adnaribo adnaribo approved these changes

@wslany wslany wslany approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@Rawbat @wslany @adnaribo