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

CATTY-369 Implement place visually feature #1728

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

mstoeg
Copy link
Contributor

@mstoeg mstoeg commented Mar 3, 2022

(https://jira.catrob.at/browse/CATTY-369)

  • Refactor PlaceAtBrick, PlaceAtBrick+CBXMLHandler, GlideToBrick, GlideToBrick+CBXMLHandler to Swift
  • add functionality to XCTTestCaseExtension to directly tap on a brick or on a brick cell

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
  • Verify that the Jira ticket is in the status Ready for Development
  • 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 git workflow (rebase and squash your commits)
  • 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 #catty Slack channel and ask for a code reviewer

Copy link
Contributor

@wallisch wallisch left a comment

Choose a reason for hiding this comment

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

And rebase to develop please, then we're done IMO

@wallisch
Copy link
Contributor

Some issues:

  • When placing the moles in the demo project, sometimes the app crashes on "place visually"
  • When the project has never been started, the scene background is empty
  • IF the scene background is there on the demo project (after starting once), the previous position of the look is not displayed (layer error?)

Additionally, i would tint the status bar with light color mode (white) on the "place visually" screen

@mstoeg mstoeg force-pushed the CATTY-369 branch 2 times, most recently from ca8919c to a8b77d9 Compare May 12, 2022 09:34
@mstoeg
Copy link
Contributor Author

mstoeg commented May 12, 2022

Thank you, I fixed the requested changes.

Concerning the navigation bar: I deliberately made it transparent, because if someone is creating a fullscreen app and wants to visually place an object at the top of the screen it would not be possible as the top area would be obscured by the navigation bar.

@wallisch
Copy link
Contributor

I rebased and also made some small changes:

  • Fixed storyboard error
  • System status bar is now white instead of black
  • Layout/spacing/font adjustments in the storyboard
  • Better pocketcode logo image for empty looks

I think we're good to go now.

Copy link
Member

@m-herold m-herold left a comment

Choose a reason for hiding this comment

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

Very nice PR, thanks @mstoeg!

While this feature works perfectly for objects without any transformation, the place visually feature is misleading/does not apply transformations when an object is

  • Turned left/right
  • Pointed in a certain direction
  • Changed its look

It however works fine when changing an object's size. In Catroid it seems that the last visible appearance is being cached and reused when using the place visually feature. Could we do the same for Catty?

Finally, this feature does not work for landscape mode. In case landscape mode needs a significant amount of rework, I am more than happy to implement the place visually feature for landscape mode in a separate ticket while merging this already (after the previously mentioned bugs have been implemented).

@wallisch wallisch added this to the v0.6.22 milestone Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants