Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

timob-8362: scrollview layout fix. #2015

Merged
merged 4 commits into from Apr 19, 2012

Conversation

Projects
None yet
2 participants
Contributor

hieupham007 commented Apr 16, 2012

JIRA: https://jira.appcelerator.org/browse/TIMOB-8362
Testing steps in JIRA. Also test the tests in #1988

@ayeung ayeung commented on an outdated diff Apr 18, 2012

...ava/ti/modules/titanium/ui/widget/TiUIScrollView.java
@@ -135,9 +134,14 @@ protected int getMeasuredWidth(int maxWidth, int widthSpec)
if (contentWidth == AUTO) {
contentWidth = maxWidth; // measuredWidth;
}
+
@ayeung

ayeung Apr 18, 2012

Contributor

Can we remove the extra space here? =)

@ayeung ayeung commented on an outdated diff Apr 18, 2012

...ava/ti/modules/titanium/ui/widget/TiUIScrollView.java
// Force the minimum width of the content view to be the size of its parent (scroll view)
- return Math.max(contentWidth, parentWidth);
+ if (contentWidth > parentWidth) {
+ return Math.max(contentWidth, parentWidth);
@ayeung

ayeung Apr 18, 2012

Contributor

We can probably just return contentWidth here instead of using max since you already check for it in the if statement.

@ayeung ayeung commented on an outdated diff Apr 18, 2012

...ava/ti/modules/titanium/ui/widget/TiUIScrollView.java
@@ -149,8 +153,12 @@ protected int getMeasuredHeight(int maxHeight, int heightSpec)
}
// Force the minimum height of the content view to be the size of its parent (scroll view)
@ayeung

ayeung Apr 18, 2012

Contributor

Since the logic here changed, we should also update the comments as well.

Contributor

ayeung commented Apr 18, 2012

Code reviewed. Please address comments.

@ayeung ayeung commented on an outdated diff Apr 18, 2012

...ava/ti/modules/titanium/ui/widget/TiUIScrollView.java
- // Force the minimum width of the content view to be the size of its parent (scroll view)
- return Math.max(contentWidth, parentWidth);
+ // Sets the minimum width of the content view to be the size of its parent (scroll view)
@ayeung

ayeung Apr 18, 2012

Contributor

Sorry, should have been more clear with this. We aren't really setting the minimum width of the content view to be the size of its parent here. We are essentially delegating measurement process to resolveSize. Since we set the content view to have fill behavior (should be in tiuiscrollview somewhere), android will measure the content view to be the same size as its parent. We do force the size to be the content width if the user specifies it to be greater than the parent width. A more appropriate comment here would be something like "Return the content width when it's greater than the scrollview width".

@ayeung ayeung commented on an outdated diff Apr 18, 2012

...ava/ti/modules/titanium/ui/widget/TiUIScrollView.java
@@ -148,9 +151,13 @@ protected int getMeasuredHeight(int maxHeight, int heightSpec)
contentHeight = maxHeight; // measuredHeight;
}
- // Force the minimum height of the content view to be the size of its parent (scroll view)
- return Math.max(contentHeight, parentHeight);
- }
+ // Sets the minimum height of the content view to be the size of its parent (scroll view)
@ayeung

ayeung Apr 18, 2012

Contributor

same thing here with height

Contributor

ayeung commented Apr 18, 2012

Left minor comments.

ayeung pushed a commit that referenced this pull request Apr 19, 2012

Merge pull request #2015 from hieupham007/timob-8470
timob-8362: scrollview layout fix.

@ayeung ayeung merged commit 6b172c9 into appcelerator:master Apr 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment