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

TIMOB-8996: Complete remaining issues from code review #60

Merged

Conversation

dcampbell-macadamian
Copy link

Reviewers: Alex, Harut

Description:
Clean up code from previous code reviews

Change Log:

  • protect NativeContainerObject::initialize being double initialized
  • added "bottom" property
  • added "left" property
  • added "right" property
  • added debug code
  • reduced code duplication in UI object creation
  • added "s" to "prop" in "setTiMappingProperties"
  • moved TiInternalEventListener to private internal of TiV8EventContainer class

Test Cases:

  • build "tibb"
  • build "tibbtest"
  • deploy and run "tibbtest"
  • BB app should run with red background, a label, progress bar, slider, and a button

@alexandergalstyan
Copy link

The one problem found is setLeft, setRight, setTop, setBottom don't work with current solution.

@@ -15,6 +15,9 @@
{
isInitialized_ = false;
parentObject_ = NULL;
#ifdef _TI_DEBUG_
cstrName_ = NULL;
#endif // _TI_DEBUG_

Choose a reason for hiding this comment

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

Probably it will correct to initialize other debug members here.

Choose a reason for hiding this comment

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

done

@alexandergalstyan
Copy link

This PR, needs to be merged with upstream.
Reviewed.

@jpl-mac
Copy link

jpl-mac commented May 23, 2012

David, do add the url to this PR in the jira issue comments

{
return error;
}
((bb::cascades::Control*)getNativeHandle())->setBottomMargin(bottom);

Choose a reason for hiding this comment

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

setBottom should set bottom property which is View's bottom position in container, but bottomMargin margins specifies the space around a control.
e.g. for absolute layout it doesn't work.

Copy link

Choose a reason for hiding this comment

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

These should likely also take into account the layout rules as set out by appcel. These props (top, bottom, left, right) have relative priority and depending on the cases, some may need to be ignored.

Choose a reason for hiding this comment

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

These will change in future patches. All view objects will be embedded within a Cascades container.

@Harutyun
Copy link

reviewed

@@ -210,16 +225,14 @@ void TiUIBase::setParametersFromObject(Local<Object> obj)
Handle<Array> propNames = obj->GetPropertyNames();
uint32_t props = propNames->Length();
Local<Value> propValue;
Copy link

Choose a reason for hiding this comment

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

This line should be removed

@jpl-mac
Copy link

jpl-mac commented May 23, 2012

Looks like

  1. we should review the logical separation of classes with regards to cascades awareness, separation into folders

  1. review error handling strategy

run it by appcelerator

Are the only issues not addressed in this patch, David can you create another issue for 7 and link it to 8996.
Also please follow up with appcelerator about the error handling strategy. Although i suspect from what i saw on iphone that it basically needs to return the string of the encountered exception.

@dcampbell-macadamian
Copy link
Author

The issues not addressed have been moved into separate tasks:
https://jira.appcelerator.org/browse/TIMOB-9228
https://jira.appcelerator.org/browse/TIMOB-9203

@jpl-mac
Copy link

jpl-mac commented May 25, 2012

Thanks Dave, please also add the url of this PR to the JIRA issue

@Harutyun
Copy link

Approved

@jpl-mac
Copy link

jpl-mac commented Jun 8, 2012

Alex, have you approved this patch?

@alexandergalstyan
Copy link

Rather than this: "Please use prefix _ for static member functions, instead of suffix" (TiUIObject.*), I have nothing to add. Though that could be done separately.
Otherwise approved.

Conflicts:
	blackberry/tibb/NativeControlObject.cpp
	blackberry/tibb/TiUIBase.cpp
	blackberry/tibb/TiUIObject.h
dcampbell-macadamian added a commit that referenced this pull request Jun 8, 2012
TIMOB-8996: Complete remaining issues from code review
@dcampbell-macadamian dcampbell-macadamian merged commit b733e5c into Macadamian:blackberry Jun 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants