[TIMOB-10712] Android: label 'opacity' property won't change for TableVi... #3043

krowley · 2012-09-25T21:15:34Z

...ewRows outside the visible screen on touch event

Re-factor code so that view-proxy associations are only made at layout time, but not
during any non-layout passes through getView() (i.e. measurement passes).

Needs review by Opie -- Thanks!

…eViewRows outside the visible screen on touch event

krowley · 2012-09-25T23:21:27Z

I can see that this change breaks "old style" rows -- a fix for that is in progress and will be added here.

…eViewRows outside the visible screen on touch event

krowley · 2012-09-26T15:25:18Z

There's another change for old-style rows coming here.

…eViewRows outside the visible screen on touch event

billdawson · 2012-09-26T21:46:57Z

...oid/modules/ui/src/java/ti/modules/titanium/ui/widget/tableview/TiTableViewRowProxyItem.java

@@ -124,7 +125,7 @@ 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();
+				view = proxy.forceCreateView(false);


We could use a comment here as to why it's not setting the model listener.

billdawson · 2012-09-26T21:58:54Z

Functional test passes. Also tested table view examples in KitchenSink to look for regressions but it seemed fine.

Left comments for review. Also, I'll get a second reviewer on this after the next round of changes. I think any somewhat-major change to TableView should have two reviewers.

ayeung · 2012-09-27T00:54:36Z

From what we talked about it seemed like you moved the code to associate a proxy to a view from refreshControls() to onLayout() so that it would not occur during measure passes, but we still have a lot of logic inside refreshControls() like calling processProperties(). Do we still need this during a measure pass, or should that be moved as well?

Also, the test case should include the old style rows test case too since you added logic for that.

krowley · 2012-09-27T03:46:52Z

Yes I agree with your summary of the situation.

refreshControls() is a poorly named method in this
case. What it really does after my change is to
create the child views if needed, and then apply
the properties to each from the corresponding
proxy. It no longer makes the proxy-view
association. So it gets called for both
measurment and layout passes through
getView().

I'm going to rename it to something like
createChildViews() to help clear things up.

Karl

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

From what we talked about it seemed like you moved the code to associate a proxy to a view from refreshControls() to onLayout() so that it would not occur during measure passes, but we still have a lot of logic inside refreshControls() like calling processProperties(). Do we still need this during a measure pass, or should that be moved as well?

Also, the test case should include the old style rows test case too since you added logic for that.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3043#issuecomment-8915124.

krowley · 2012-09-27T04:00:32Z

Thanks for the comments, they are all good ones and I'll address them.

I'm looking for more test cases for all of this. Arthur Evans found one by
digging out an old bug that used old-style rows.

Karl

From: Bill Dawson <notifications@github.com mailto:notifications@github.com>
Reply-To: appcelerator/titanium_mobile <reply@reply.github.com mailto:reply@reply.github.com>
Date: Wed, 26 Sep 2012 14:58:56 -0700
To: appcelerator/titanium_mobile <titanium_mobile@noreply.github.com mailto:titanium_mobile@noreply.github.com>
Cc: Karl Rowley <krowley@appcelerator.com mailto:krowley@appcelerator.com>
Subject: Re: [titanium_mobile] [TIMOB-10712] Android: label 'opacity' property won't change for TableVi... (#3043)

Functional test passes. Also tested table view examples in KitchenSink to look for regressions but it seemed fine.

Left comments for review. Also, I'll get a second reviewer on this after the next round of changes. I think any somewhat-major change to TableView should have two reviewers.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3043#issuecomment-8907984.

…eViewRows outside the visible screen on touch event

krowley · 2012-09-27T20:39:19Z

OK I've updated code to respond to comments.

ayeung · 2012-09-27T22:09:36Z

...oid/modules/ui/src/java/ti/modules/titanium/ui/widget/tableview/TiTableViewRowProxyItem.java

+				// 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().
+				view = proxy.forceCreateView(false);  // false means don't set modelListener


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.

ayeung · 2012-09-27T22:17:56Z

Code reviewed. please address comments.

…eViewRows outside the visible screen on touch event

krowley · 2012-09-27T23:26:45Z

OK, I addressed comments from Allen.

btknorr · 2012-09-28T15:42:26Z

Hi there...I submitted the original bug for GameStop. Now when I build the sdk manually using scons, the bug seems to be fixed, but now the images don't show up...see example code http://jira.appcelerator.org/browse/TIMOB-10712

This is the error I see in logcat:

E/TiDownloadManager( E/TiDownloadManager( E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( E/TiDownloadManager( E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): E/TiDownloadManager( 5900): 5900): (pool-3-thread-2) [0,1970] Exception downloading https://github.com/appcelerator/titanium_mobile/raw/master/demos/KitchenSink/Resources/images/custom_tableview/eventsButton.png
5900): java.lang.ArrayIndexOutOfBoundsException: length=1; index=1
at org.appcelerator.titanium.util.TiResponseCache.get(TiResponseCache.java:276)
at libcore.net.http.HttpEngine.initResponseSource(HttpEngine.java:249)
at libcore.net.http.HttpEngine.sendRequest(HttpEngine.java:211)
at libcore.net.http.HttpURLConnectionImpl.getResponse(HttpURLConnectionImpl.java:273)
at libcore.net.http.HttpURLConnectionImpl.getInputStream(HttpURLConnectionImpl.java:168)
at libcore.net.http.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:270)
at java.net.URL.openStream(URL.java:462)
at org.appcelerator.titanium.util.TiDownloadManager$DownloadJob.run(TiDownloadManager.java:157)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1076)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:569)
at java.lang.Thread.run(Thread.java:856)
5900): (pool-3-thread-1) [1970,1970] Exception downloading https://github.com/appcelerator/titanium_mobile/raw/master/demos/KitchenSink/Resources/images/custom_tableview/user.png
5900): java.lang.ArrayIndexOutOfBoundsException: length=1; index=1
at org.appcelerator.titanium.util.TiResponseCache.get(TiResponseCache.java:276)
at libcore.net.http.HttpEngine.initResponseSource(HttpEngine.java:249)
at libcore.net.http.HttpEngine.sendRequest(HttpEngine.java:211)
at libcore.net.http.HttpURLConnectionImpl.getResponse(HttpURLConnectionImpl.java:273)
at libcore.net.http.HttpURLConnectionImpl.getInputStream(HttpURLConnectionImpl.java:168)
at libcore.net.http.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:270)
at java.net.URL.openStream(URL.java:462)
at org.appcelerator.titanium.util.TiDownloadManager$DownloadJob.run(TiDownloadManager.java:157)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1076)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:569)
at java.lang.Thread.run(Thread.java:856)

krowley · 2012-09-28T16:44:33Z

It's working for me right now. I"m wondering if the errors above for kitchen sink images were due to a server problem. Anyway, I"m eager to get this pull request merged.

btknorr · 2012-09-28T16:53:24Z

Are you testing on a device or the emulator? I am on a device, but if the images are showing for you then maybe it is a server issue or something weird with the Ti response cache on my device.

krowley · 2012-09-28T16:59:32Z

I"m testing using a device -- Samsung Galaxy S2 (AT&T), Android 2.3.6

btknorr · 2012-09-28T18:27:09Z

FYI...I just pulled down the latest from timob-10712-2 and built it and the bug fix doesn't work anymore. Should I be pulling from a different branch now?

btknorr · 2012-09-28T18:32:04Z

Nevermind...for some reason Ti Studio changed my Ti SDK back to 2.1.2GA

ayeung · 2012-09-29T19:32:23Z

...oid/modules/ui/src/java/ti/modules/titanium/ui/widget/tableview/TiTableViewRowProxyItem.java

+	// 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?

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.com mailto:notifications@github.com>
Reply-To: appcelerator/titanium_mobile <reply@reply.github.com mailto:reply@reply.github.com>
Date: Sat, 29 Sep 2012 12:32:26 -0700
To: appcelerator/titanium_mobile <titanium_mobile@noreply.github.com mailto:titanium_mobile@noreply.github.com>
Cc: Karl Rowley <krowley@appcelerator.com mailto: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.

ayeung · 2012-09-29T19:39:12Z

android/titanium/src/java/org/appcelerator/titanium/proxy/TiViewProxy.java


-	public void realizeViews(TiUIView view)
+	public void realizeViews(TiUIView view, boolean isLayoutPass)


rename isLayoutPass

ayeung · 2012-09-29T19:44:30Z

Code reviewed, left several comments.

…eViewRows outside the visible screen on touch event -- Address code review comments

ayeung · 2012-10-01T21:29:44Z

Code reviewed. Request Accepted

billdawson · 2012-10-01T22:13:27Z

This PR breaks KitchenSink. (KitchenSink crashes immediately on opening.)

When we make "big" changes to something that is also used in KitchenSink, we also test KS before submitting a PR, as a kind of "smoke test" beyond the test case in the individual JIRA ticket. It's a good habit to get into.

krowley · 2012-10-02T17:21:44Z

I'm not seeing failures in Kitchen Sink so far. What is the stack trace you are seeing for the Kitchen Sink crash?

billdawson · 2012-10-02T17:27:54Z

Uh oh, hopefully I'm not leading us on a wild goose chase. I'll try again.
And I should have given you the trace last time, sorry about that.

On Tue, Oct 2, 2012 at 7:21 PM, krowley notifications@github.com wrote:

I'm not seeing failures in Kitchen Sink so far. What is the stack trace
you are seeing for the Kitchen Sink crash?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3043#issuecomment-9079098.

Bill Dawson
Platform Engineer
Appcelerator, Inc.

bdawson@appcelerator.com
http://www.appcelerator.com

Appcelerator, Inc.
The Next Generation Mobile Platform

rusticphilosopher · 2012-10-02T18:46:26Z

Code reviewed without comments. Waiting on further update following Bill's feedback

billdawson · 2012-10-02T19:34:57Z

KS is fine. I wonder if maybe I accidentally pulled in your "timob-10712" rather than "timob-10712-2" branch yesterday. Not sure what happened.

Resuming review....

rusticphilosopher · 2012-10-02T19:48:24Z

CR Accepted Leaving FR to Allen and Bill. Merge approved once FR from Bill/Allen passes.

billdawson · 2012-10-02T20:08:23Z

CR/FR accepted. Thank you especially for the code commenting.

For FR, I re-tested the fail case, went through KS tableview tests and made my own test app to check proxy property values while scrolling through the table.

[TIMOB-10712] Android: label 'opacity' property won't change for TableVi...

[TIMOB-10712] Android: label 'opacity' property won't change for Tabl…

797e6f9

…eViewRows outside the visible screen on touch event

[TIMOB-10712] Android: label 'opacity' property won't change for Tabl…

bd8b2c2

…eViewRows outside the visible screen on touch event

[TIMOB-10712] Android: label 'opacity' property won't change for Tabl…

4fd21fb

…eViewRows outside the visible screen on touch event

billdawson reviewed Sep 26, 2012
View reviewed changes

[TIMOB-10712] Android: label 'opacity' property won't change for Tabl…

54ab0d0

…eViewRows outside the visible screen on touch event

ayeung reviewed Sep 27, 2012
View reviewed changes

[TIMOB-10712] Android: label 'opacity' property won't change for Tabl…

8600fd1

…eViewRows outside the visible screen on touch event

ayeung reviewed Sep 29, 2012
View reviewed changes

[TIMOB-10712] Android: label 'opacity' property won't change for Tabl…

2059f41

…eViewRows outside the visible screen on touch event -- Address code review comments

ghost assigned rusticphilosopher Oct 1, 2012

ghost assigned billdawson Oct 2, 2012

billdawson added a commit that referenced this pull request Oct 2, 2012

Merge pull request #3043 from krowley/timob-10712-2

09cbaa3

[TIMOB-10712] Android: label 'opacity' property won't change for TableVi...

billdawson merged commit 09cbaa3 into tidev:master Oct 2, 2012

krowley mentioned this pull request Oct 3, 2012

[TIMOB-6723] Android: TableViewRow fonts change when row is scrolled #3103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-10712] Android: label 'opacity' property won't change for TableVi... #3043

[TIMOB-10712] Android: label 'opacity' property won't change for TableVi... #3043

krowley commented Sep 25, 2012

krowley commented Sep 25, 2012

krowley commented Sep 26, 2012

billdawson Sep 26, 2012

billdawson commented Sep 26, 2012

ayeung commented Sep 27, 2012

krowley commented Sep 27, 2012

krowley commented Sep 27, 2012

krowley commented Sep 27, 2012

ayeung Sep 27, 2012

ayeung commented Sep 27, 2012

krowley commented Sep 27, 2012

btknorr commented Sep 28, 2012

krowley commented Sep 28, 2012

btknorr commented Sep 28, 2012

krowley commented Sep 28, 2012

btknorr commented Sep 28, 2012

btknorr commented Sep 28, 2012

ayeung Sep 29, 2012

krowley Oct 1, 2012

ayeung Sep 29, 2012

ayeung commented Sep 29, 2012

ayeung commented Oct 1, 2012

billdawson commented Oct 1, 2012

krowley commented Oct 2, 2012

billdawson commented Oct 2, 2012

rusticphilosopher commented Oct 2, 2012

billdawson commented Oct 2, 2012

rusticphilosopher commented Oct 2, 2012

billdawson commented Oct 2, 2012


		public void realizeViews(TiUIView view)
		public void realizeViews(TiUIView view, boolean isLayoutPass)

[TIMOB-10712] Android: label 'opacity' property won't change for TableVi... #3043

[TIMOB-10712] Android: label 'opacity' property won't change for TableVi... #3043

Conversation

krowley commented Sep 25, 2012

krowley commented Sep 25, 2012

krowley commented Sep 26, 2012

billdawson Sep 26, 2012

Choose a reason for hiding this comment

billdawson commented Sep 26, 2012

ayeung commented Sep 27, 2012

krowley commented Sep 27, 2012

krowley commented Sep 27, 2012

krowley commented Sep 27, 2012

ayeung Sep 27, 2012

Choose a reason for hiding this comment

ayeung commented Sep 27, 2012

krowley commented Sep 27, 2012

btknorr commented Sep 28, 2012

krowley commented Sep 28, 2012

btknorr commented Sep 28, 2012

krowley commented Sep 28, 2012

btknorr commented Sep 28, 2012

btknorr commented Sep 28, 2012

ayeung Sep 29, 2012

Choose a reason for hiding this comment

krowley Oct 1, 2012

Choose a reason for hiding this comment

ayeung Sep 29, 2012

Choose a reason for hiding this comment

ayeung commented Sep 29, 2012

ayeung commented Oct 1, 2012

billdawson commented Oct 1, 2012

krowley commented Oct 2, 2012

billdawson commented Oct 2, 2012

rusticphilosopher commented Oct 2, 2012

billdawson commented Oct 2, 2012

rusticphilosopher commented Oct 2, 2012

billdawson commented Oct 2, 2012