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-10712] Android: label 'opacity' property won't change for TableVi... #3043

Merged
merged 6 commits into from
Oct 2, 2012
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ public void processProperties(KrollDict d)
super.processProperties(d);

TextView tv = (TextView) getNativeView();

// Clear any text style left over here if view is recycled
TiUIHelper.styleText(tv, null, null, null);

// Only accept one, prefer text to title.
if (d.containsKey(TiC.PROPERTY_HTML)) {
tv.setText(Html.fromHtml(TiConvert.toString(d, TiC.PROPERTY_HTML)), TextView.BufferType.SPANNABLE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,18 @@ public int getItemViewType(int position) {
return rowTypes.get(item.className);
}

//
// IMPORTANT NOTE:
// getView() is called by the Android framework whenever it needs a view.
// The call to getView() could come on a measurement pass or on a layout
// pass. It's not possible to tell from the arguments whether the framework
// is calling getView() for a measurement pass or for a layout pass. Therefore,
// it is important that getView() and all methods call by getView() only create
// the views and fill them in with the appropriate data. What getView() and the
// methods call by getView MUST NOT do is to make any associations between
// proxies and views. Those associations must be made only for the views
// that are used for layout, and should be driven from the onLayout() callback.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment looks good, but can we put this in a block comment? Something like:

  /**
   * IMPORTANT NOTE
   *
   */

public View getView(int position, View convertView, ViewGroup parent) {
Item item = (Item) getItem(position);
TiBaseTableViewItem v = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;

import org.appcelerator.kroll.KrollDict;
import org.appcelerator.kroll.common.Log;
Expand Down Expand Up @@ -97,7 +98,11 @@ protected TiViewProxy addViewToOldRow(int index, TiUIView titleView, TiViewProxy
return label;
}

protected void refreshControls()
//
// Create views for measurement or for layout. For each view, apply the
// properties from the appropriate proxy to the view.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here about the comment formatting.

protected void createViews()
Copy link
Contributor

Choose a reason for hiding this comment

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

createView seems a little generic for the context of the table view, maybe we can use something like createControls() so it aligns with terminology in this class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem I have with this is that "controls" is confusing here.
The name createViews() describes pretty well what is going on
underneath.

Karl

From: ayeung <notifications@github.commailto:notifications@github.com>
Reply-To: appcelerator/titanium_mobile <reply@reply.github.commailto:reply@reply.github.com>
Date: Sat, 29 Sep 2012 12:32:26 -0700
To: appcelerator/titanium_mobile <titanium_mobile@noreply.github.commailto:titanium_mobile@noreply.github.com>
Cc: Karl Rowley <krowley@appcelerator.commailto:krowley@appcelerator.com>
Subject: Re: [titanium_mobile] [TIMOB-10712] Android: label 'opacity' property won't change for TableVi... (#3043)

In android/modules/ui/src/java/ti/modules/titanium/ui/widget/tableview/TiTableViewRowProxyItem.java:

@@ -97,7 +98,11 @@ protected TiViewProxy addViewToOldRow(int index, TiUIView titleView, TiViewProxy
return label;
}

  • protected void refreshControls()
    
  • //
    
  • // Create views for measurement or for layout.  For each view, apply the
    
  • // properties from the appropriate proxy to the view.
    
  • //
    
  • protected void createViews()
    

createView seems a little generic for the context of the table view, maybe we can use something like createControls() so it aligns with terminology in this class?


Reply to this email directly or view it on GitHubhttps://github.com//pull/3043/files#r1723800.

{
ArrayList<TiViewProxy> proxies = getRowProxy().getControls();
int len = proxies.size();
Expand All @@ -123,8 +128,15 @@ protected void refreshControls()
}
if (view == null) {
// In some cases the TiUIView for this proxy has been reassigned to another proxy
// We don't want to actually release it though, just reassign by creating a new view
view = proxy.forceCreateView();
// We don't want to actually release it though, just reassign by creating a new view.
//
// Not setting modelListener from here because this could be a measurement pass or
// a layout pass through getView(), which means that the view we have here may
// not be the one that gets displayed on the screen. So we don't want to make
// any view-proxy association at this point. We only want to make that association
// on a layout pass (i.e. when onLayout() gets called).
//
view = proxy.forceCreateView(false); // false means don't set modelListener
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use a more detailed comment regarding why we are not setting the model listener here. Since this is a complex bug, we should probably put some comments in relevant places like getView() to describe the situation.

clearChildViews(proxy);
if (i >= views.size()) {
views.add(view);
Expand All @@ -134,9 +146,8 @@ protected void refreshControls()
}

View v = view.getOuterView();
view.setProxy(proxy);
view.processProperties(proxy.getProperties());
applyChildProxies(proxy, view);
applyChildProperties(proxy, view);
Copy link
Contributor

Choose a reason for hiding this comment

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

So we don't want to associate proxies here, but we still want to create it for measuring purposes during the measure pass. Maybe we could leave a note here (or somewhere else more appropriate) to describe that

if (v.getParent() == null) {
content.addView(v, view.getLayoutParams());
}
Expand All @@ -151,17 +162,14 @@ protected void clearChildViews(TiViewProxy parent)
}
}

protected void applyChildProxies(TiViewProxy viewProxy, TiUIView view)
protected void applyChildProperties(TiViewProxy viewProxy, TiUIView view)
{
int i = 0;
TiViewProxy childProxies[] = viewProxy.getChildren();
for (TiUIView childView : view.getChildren()) {
TiViewProxy childProxy = childProxies[i];
childView.setProxy(childProxy);
//Since we wipe out children's views earlier we need to reset them.
childProxy.setView(childView);
childView.processProperties(childProxy.getProperties());
applyChildProxies(childProxy, childView);
applyChildProperties(childProxy, childView);
i++;
}
}
Expand Down Expand Up @@ -273,9 +281,11 @@ public void setRowData(TableViewRowProxy rp) {
content.setLayoutArrangement(TiConvert.toString(props, TiC.PROPERTY_LAYOUT));
}

// hasControls() means that the proxy has children
if (rp.hasControls()) {
refreshControls();
createViews();
} else {
// no children means that this is an old-style row
refreshOldStyleRow();
}
}
Expand Down Expand Up @@ -355,9 +365,18 @@ protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec)
@Override
protected void onLayout(boolean changed, int left, int top, int right, int bottom)
{
// Make this association here to avoid doing it on measurement passes
// Make these associations here to avoid doing them on measurement passes
TableViewRowProxy rp = getRowProxy();
rp.setTableViewItem(this);
if (this.item.proxy.getChildren().length == 0) {
// old-style row
TiUIView childView = views.get(0);
childView.processProperties(filterProperties(rp.getProperties()));
childView.setProxy(rp);
}
else {
associateProxies(this.item.proxy.getChildren(), views);
}

int contentLeft = left;
int contentRight = right;
Expand Down Expand Up @@ -460,4 +479,21 @@ public void release() {
}

}

protected void associateProxies(TiViewProxy[] proxies, List<TiUIView> views)
{
int i = 0;
for (TiUIView view : views) {
if (proxies.length < (i+1)) {
break;
}
TiViewProxy proxy = proxies[i];
proxy.setView(view);
view.setProxy(proxy);
proxy.setModelListener(view);
associateProxies(proxy.getChildren(), view.getChildren());
i++;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -393,32 +393,42 @@ public void setView(TiUIView view)
{
this.view = view;
}

public TiUIView forceCreateView(boolean isLayoutPass)
Copy link
Contributor

Choose a reason for hiding this comment

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

isLayoutPass should be renamed here.

{
view = null;
return getOrCreateView(isLayoutPass);
}

public TiUIView forceCreateView()
{
view = null;
return getOrCreateView();
return forceCreateView(true);
}

/**
* Creates or retrieves the view associated with this proxy.
* @return a TiUIView instance.
* @module.api
*/
public TiUIView getOrCreateView()
public TiUIView getOrCreateView(boolean setModelListener)
{
if (activity == null || view != null) {
return view;
}

if (TiApplication.isUIThread()) {
return handleGetView();
return handleGetView(setModelListener);
}

return (TiUIView) TiMessenger.sendBlockingMainMessage(getMainHandler().obtainMessage(MSG_GETVIEW), 0);
}

/**
* Creates or retrieves the view associated with this proxy.
* @return a TiUIView instance.
* @module.api
*/
public TiUIView getOrCreateView()
{
return getOrCreateView(true);
}

protected TiUIView handleGetView()
protected TiUIView handleGetView(boolean setModelListener)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, 'enableModelListener' sounds better.

it almost seems like we have a method named setModelListener

{
if (view == null) {
Log.d(TAG, "getView: " + getClass().getSimpleName(), Log.DEBUG_MODE);
Expand All @@ -432,15 +442,23 @@ protected TiUIView handleGetView()
Log.w(TAG, "Activity is null", Log.DEBUG_MODE);
}
}
realizeViews(view);
realizeViews(view, setModelListener);
view.registerForTouch();
}
return view;
}

protected TiUIView handleGetView()
{
return handleGetView(true);
}

public void realizeViews(TiUIView view)
public void realizeViews(TiUIView view, boolean isLayoutPass)
Copy link
Contributor

Choose a reason for hiding this comment

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

rename isLayoutPass

{
setModelListener(view);
if (isLayoutPass)
{
setModelListener(view);
}

// Use a copy so bundle can be modified as it passes up the inheritance
// tree. Allows defaults to be added and keys removed.
Expand All @@ -461,6 +479,11 @@ public void realizeViews(TiUIView view)
}
}
}

public void realizeViews(TiUIView view)
{
realizeViews(view, true);
}

public void releaseViews()
{
Expand Down