[TIMOB-24174]: Android: Update ListView check/disclosure/child graphics #8832
Conversation
| @@ -439,6 +441,36 @@ public void onScroll(AbsListView view, int firstVisibleItem, int visibleItemCoun | |||
| setNativeView(wrapper); | |||
| } | |||
|
|
|||
| private int getImageRessource(String img){ | |||
There was a problem hiding this comment.
This should be moved into TiRHelper so we can use this elsewhere 👍
garymathews
changed the title
|
They should retain the old color, otherwise developers might complain about the change. |
| @@ -439,6 +441,36 @@ public void onScroll(AbsListView view, int firstVisibleItem, int visibleItemCoun | |||
| setNativeView(wrapper); | |||
| } | |||
|
|
|||
| private int getImageRessource(String img){ | |||
| // | |||
| String resName = "drawable."+img+"_48"; // default medium | |||
| @@ -43,7 +43,7 @@ | |||
| // every time we add a row. No sense checking it each time. | |||
| private static boolean ICS_OR_GREATER = (Build.VERSION.SDK_INT >= TiC.API_LEVEL_ICE_CREAM_SANDWICH); | |||
|
|
|||
| private static final int LEFT_MARGIN = 5; | |||
| private static final int LEFT_MARGIN = 22; | |||
There was a problem hiding this comment.
Does Android natively has 22dp left and 27dp right?
There was a problem hiding this comment.
A bit hard to find out as I find various examples (from 0 margin to 48). But I found this theme.xml:
https://github.com/android/platform_frameworks_base/blob/82834baa358f55acb542e17da828b2d497cf8332/core/res/res/values/themes.xml#L146
and it looks like 6dp is a default value for listitems in a basic theme. I will update ListView and TableView to look the same
There was a problem hiding this comment.
finally solved it :) Stupid dp vs pixels. Now both images (listview/tableview) have a 6dp padding
|
@garymathews or @fmerzadyan Can one of you review this? Thxxxx! |
There was a problem hiding this comment.
CR: PASS
FT: PASS
TEST CASE
var win = Ti.UI.createWindow({
backgroundColor: 'gray',
layout: 'vertical'
}),
tv = Ti.UI.createTableView({
headerTitle: 'TableView',
data: [
{
title: 'Apples',
hasChild: true
}, {
title: 'Bananas'
}
],
height: Ti.UI.SIZE
}),
ls = Ti.UI.createListSection({
headerTitle: 'ListView'
}),
lv = Ti.UI.createListView({
height: Ti.UI.SIZE
});
ls.setItems([
{
properties: {
title: 'Apples',
accessoryType: Ti.UI.LIST_ACCESSORY_TYPE_CHECKMARK
}
}, {
properties: {
title: 'Bananas',
accessoryType: Ti.UI.LIST_ACCESSORY_TYPE_DETAIL
}
}
]);
lv.sections = [ls];
win.add(tv);
win.add(lv);
win.open();| BEFORE | AFTER |
|---|---|
 |
 |
👍
|
@hansemannn , Can you please rebase/update this branch, so that merge is enabled. Thanks. |
|
@lokeshchdhry done! |
|
@m1ga We need a backport for this asap to include it in 6.1.0. Should I do or can you? |
|
@hansemannn ,Cool thanks. I still see the merge not enabled due to statuses/checks did not pass. |
|
@m1ga, @hansemannn, @garymathews - I am seeing some differences in phone & emulator between tableview & listview icon sizes.
If you see the apple icon in table view is slightly larger than the 2 list view icons where as in the emulator the tableview icon is smaller compared to the listview icons. Is this expected ? |
|
@lokeshchdhry No, that shouldn't be. I'll check that again, thanks for the feedback! |
|
No luck in finding the culprit at the moment. Guess it's somewhere in And I have a strange crash on 5.0.2, that
|
|
Sorry about that. Should be fine again. @lokeshchdhry the size of the tableview image was different (not 17dp like the listview) and one image was missing (disclosure_96.png). That is fixed now and tested on a Nexus 6 (emulator), Moto G and HTC A9. |
|
Thanks Michael! I'm wondering why the header title background-color is different between tableview and listview? |
|
Which one should I make the default one? Dark or light one? |
|
What ever is used natively :-) All sizes, background-colors, text colors etc pp. Those values are from old times. Maybe we do those in 7.0.0, but better soon than later. |
|
There is no real native version of this. In a ListView you would just create a View and add it (and style it yourself), so like the Ti-ListView is doing already. And you can already use custom views so the headerTitle is just a short cut. The closest thing I could find in the Material style guide is: For this PR I'll use the bright version in both headerTitles so it looks fresher and a bit more like the iOS version. |
|
Yeah, let's keep that part as it is. Thanks @m1ga! |
|
Headers look the same now:
|
|
@garymathews , @m1ga , I am getting this crash when I run test code above with locally built 6.2.0 on android API 24 7.0.0 & above. |
|
@lokeshchdhry Does it crash with master (non-PR) as well? Wondering because there is no proxy-change related to the crash. |
|
I'll have a look at that again! It might be related to this PR: #8948 ? |
|
@hansemannn , Yes I get the same error with a non-PR build as well when I run the test code above. |
|
@garymathews might be the Android 7 PR then. |
|
@lokeshchdhry Can we revalidate that now that the list-view issue is fixed? I would need this PR in one of my projects as well. :) |
|
@hansemannn @lokeshchdhry #9144 needs to be merged before this can be reviewed (to fix the crash bug). |
[TIMOB-24174] (6_1_X) Android: Update ListView check/disclosure/child graphics #8832
|
@m1ga, I know I'm late to the party here, but I'd like to add some feedback. I don't think we should be custom loading these drawable images via the _48, _72, _96, etc. postfix notation. Instead, it would be better to take advantage of Android's built-in density aware drawable folders (ex: drawable-mdpi, drawable-hdpi, etc.) since the Android OS will automatically load the correct density scaled drawable for us... or the closest match in case we're running on under a DPI that we don't have an image. This will help make it more future proof for DPIs that we current do not support, such as tvdpi (sometimes used by tablets like the 1st generation Nexus 7), and Android will also auto-scale the drawable up to match the current DPI. An additional advantage of this approach is that you can then always assume a drawable will always be loaded as "48dp" (the mdpi base resolution), even if you don't have a matching drawable matching the device's DPI. So, I recommend we do the following:
Sorry to picky about this. I know you didn't come up with this custom image loading scheme. But I think this is the best way to resolve this for good. And I appreciate you take a stab at this. |
|
Also, if we do what I suggested above, we may want to prefix these drawable images with "titanium_*.png" to avoid name collision with the Titanium developer's own resource image files. Standard practice on Android is to use a C-style prefixing notation like this. (Ex: Facebook SDK uses a "fb_" prefix for its resources.) |
|
@jquick-axway Sounds like a good idea! I'll have a look at this, thank you for the feedback |
JIRA: https://jira.appcelerator.org/browse/TIMOB-24174