739824 - Improve PromptService to be more ICS ready

Status: RESOLVED INVALID

(GeckoView :: General, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Chrome Intent UI

Chrome's UI is equivalent to the native one.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Intent UI on ICS with the patch

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 609939 [details] [diff] [review]
Patch v1

Review of attachment 609939 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks fine, but I'm just not a huge fan of this solution. We should be using android.R.layout.select_dialog_item_holo here I think, but its not a public resource. Sriram mentioned he knew some tricks to get to it. I'll push him to write out the method in here.

Even if we can't do that, I think I would then rather we put together our own os specific layout for these. I put the original margin stuff in here to indent items that were inside groups, but it feels like its getting a bit out of hand mixing theme code with real logic in our code.

If someone thinks its important that we get this in for this first release (I don't), I'd reconsider taking this.

::: mobile/android/base/PromptService.java
@@ +126,2 @@
>                                                     res.getDisplayMetrics());
> +        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {

Just for cleaner code at least, lets cache this above

boolean isICS = Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH; 

and then reuse it. I wonder if we want this change on Honeycomb as well.

Comment 4

[Sriram Ramasubramanian [:sriram]]

The theme can be set at the builder when the dialog box is created.
http://developer.android.com/reference/android/app/AlertDialog.Builder.html#AlertDialog.Builder%28android.content.Context,%20int%29

However, we are explicitly asking Android to use a particular resourceId for TextViews. If we don't do that, we will be fine. The getView() of PromptListAdapter should be like.

@Override
public View getView(some params) {
   View view = super.getView();
   // do what we want to do with the view
   return view;
}

This will ensure correct textview getting picked for the theme choosen.

Comment 5

[Mounir Lamouri (:mounir)]

I'm not sure I understand what you mean. Could you write a patch that should roughly do that?

Comment 6

[Mounir Lamouri (:mounir)]

Wes, a year have passed and this bug was not fixed as far as I can see. Should we land this to reduce the problem or do you still want to find another solution?

Comment 7

[Wesley Johnston (:wesj)]

I'll have to look for an ICS device. We've landed some fixes to these menus over the last year that may have fixed this?

On my (JB) phone at least, Android has moved away from a list of items to a grid, and that's what Chrome uses now (although Chrome appears broken... at least the filename never appears and now its telling me it can't because of "low memory"). I actually like our lists better that the grid, so I'm fine leaving them in. But it would be trivial to switch (I'm always a bit surprised we haven't heard complaints about not being "native").

Comment 8

[Mounir Lamouri (:mounir)]

Ok. I guess there is not much I can do about this bug for the moment.

Comment 9

[Wesley Johnston (:wesj)]

If you really want to put these tweaks in, I actually wouldn't mind them if we moved all of these numbers to resources. i.e. add the dimens in here for Gingerbread:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/dimens.xml

and in here for ICS:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values-v11/

and then update our Prompt.java code to pull them out using:

context.getResources().getDimension(R.id.my_dimen);

then the prompt service can be kinda oblivious to these theme differences.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 14

[David Lawrence [:dkl]]

Moving all open Core::Widget: Android bugs to GeckoView::General (then the triage owner of GeckoView will decide which ones are valuable and which ones should be closed).

Comment 15

[Emily Toop (:fluffyemily)]

Not relevant to GeckoView