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

CATTY-377 Add new object choose image before title #1682

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

CATTY-377 Add new object choose image before title #1682

wants to merge 8 commits into from

Conversation

amelak9
Copy link
Contributor

@amelak9 amelak9 commented Jul 26, 2021 •
edited by lucatp
Loading

When adding a new object in Catty, you first have to enter a name for the object, and then choose an image. This order should be changed so that first you have to select an image and then you have to enter a name for the object.
If an image is chosen from the media library, then the name for the object should be prefilled with the name of the downloaded image.

  • 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

@amelak9 amelak9 force-pushed the CATTY-377-Add-new-object-choose-image-before-title branch from 72e3528 to 426b59a Compare July 27, 2021 12:29
@amelak9 amelak9 force-pushed the CATTY-377-Add-new-object-choose-image-before-title branch 3 times, most recently from 6f7a73d to 971e33c Compare July 27, 2021 19:40
@srinner srinner self-requested a review August 17, 2021 13:59
@amelak9 amelak9 force-pushed the CATTY-377-Add-new-object-choose-image-before-title branch from 971e33c to 0d4b972 Compare August 23, 2021 19:14
wallisch
wallisch reviewed Nov 12, 2021
View reviewed changes
src/Catty/ViewController/Continue&New/SceneTableViewController.m
return result;
}
// Alert for Objects with same name
if ([[self.scene allObjectNames] containsObject:name]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work, when trying to save an image with the same name, it just gets assigned a unique name (e.g. "Look (1)")

src/Catty/ViewController/Continue&New/SceneTableViewController.m Outdated
@@ -147,12 +114,47 @@ - (void)addObjectActionWithName:(NSString*)objectName
ltvc.showAddLookActionSheetAtStartForScriptEditor = NO;
ltvc.afterSafeBlock = ^(Look* look) {
[self.navigationController popViewControllerAnimated:YES];
NSString *initialText = @"";
if (look) {
if (![look.name isEqual: @"look"]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?
Default name for drawn images is "Look" (localized), when is this triggered?

Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently it used for showing the name of the downloaded image from the media library. At the moment it looks like: 1) Media Library -> the name is prefilled with the name on the media library.
2) Drawing Image -> there is no prefilled name. It's empty.
3) choosing Image from phone -> the name is prefilled with "My image"
Is it ok when the prefilled string is empty for self drawing images?

src/Catty/ViewController/Continue&New/SceneTableViewController.m Outdated
[self showLoadingView];
[self.scene addObjectWithName:[Util uniqueName:objectName existingNames:[self.scene allObjectNames]]];
[self.scene addObjectWithName:@"default"];
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably initialize with localized "Look" and uniqueName Util, in case the app is killed when the alert is open.

@srinner srinner force-pushed the CATTY-377-Add-new-object-choose-image-before-title branch 2 times, most recently from 3a63c83 to a008b75 Compare March 16, 2022 14:31
@srinner srinner force-pushed the CATTY-377-Add-new-object-choose-image-before-title branch from a008b75 to ca22cc4 Compare March 16, 2022 15:44
@wallisch wallisch added this to the v0.6.22 milestone Jun 8, 2023
@lucatp lucatp force-pushed the CATTY-377-Add-new-object-choose-image-before-title branch from ca22cc4 to 44dc158 Compare August 17, 2023 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wallisch wallisch wallisch left review comments

@srinner srinner srinner approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
code review pending
Projects
None yet
Milestone
v0.6.22
Development

Successfully merging this pull request may close these issues.
5 participants
@amelak9 @wallisch @srinner @GatCode @lucatp