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

Dialogs aren't exactly the same as on Android Lollipop... #5

Closed
AndroidDeveloperLB opened this issue Oct 25, 2014 · 44 comments
Closed

Comments

@AndroidDeveloperLB
Copy link

Compare those:
Android Lollipop theme:
image

library theme:
image

The color of the text isn't the same, and there are dividers in the library itself, while on Lollipop there isn't .
Same happens when using a dark theme.
I also think the title is supposed to be a bit to the right.

Also, I think the layout file "alert_dialog_material.xml" (and others) can be a bit more optimized, as Lint suggests.
Some of its possible optimizations:

  1. merge "customPanel" and "custom" into a single layout.
  2. support RTL, by adding "...start" and "...end" attributes when needed.
  3. merge "buttonPanel" and its single child (which is also a layout).
@AndroidDeveloperLB
Copy link
Author

odd. when trying the library on kitkat, I don't see the dividers.

@Rolf-Smit
Copy link

I highly agree with point number 1 and 3!

@AndroidDeveloperLB
Copy link
Author

There are also other warnings on other files, not just on this layout.
This was just the most visible stuff...

@fengdai
Copy link
Owner

fengdai commented Oct 26, 2014

About the layout optimization. I ported it from the Android platform. I hope this project would perform the same as the platform's AlertDialog. So I suggest not modify the layout structure and keep it consistent with the platform.

About support RTL, I opened a new issue #6.

About the theme, I think the Lollipop's button use the color which is defined as android:colorAccent. It's Lollipop's new feature called "Color palette". I will try to fix it. Use the appcompat-v7:21.0.0 in material theme module may be a good choice.

@AndroidDeveloperLB
Copy link
Author

@fengdai Then I should probably tell Google about those issues instead. I should also tell them about the bug with the customView, which makes the buttons disappear .
I don't get why the divider looks different. Maybe it's a bug on the emulator?

About the Layout, I think I know why they added it: for some cases, the dialog's width would be very small without any reason, making the height very large.
This looks like a workaround. I'm not sure why exactly it occurs though, so I'm not sure how to fix it (other than using what they did).

About the color of the buttons, according to the xml, it's based on "textColorPrimary" , but I usually set it to black, as texts are usually black...
But this doesn't explain why I got a different color on the native dialog. Weird.

Is it possible that the emulator doesn't have the exact version as the code you've extracted from?

@fengdai
Copy link
Owner

fengdai commented Oct 26, 2014

About the bug with customView, It‘s my mistake. I missed android:layout_weight="1". The Android platform's layout doesn't have this issue.

About the color of the buttons, The reason is that the platform's AlertDialog uses the android:colorAccent as it's button color. My project's is based on textColorPrimary. I'm trying to make it the same as the platform with appcompat-v7.

@AndroidDeveloperLB
Copy link
Author

I don't understand.
Do you mean they've fixed it exactly as I've recommended?

On Sun, Oct 26, 2014 at 10:37 AM, Feng Dai notifications@github.com wrote:

About the bug with customView, It‘s my mistake. I missed
android:layout_weight="1". The Android platform's layout
https://github.com/android/platform_frameworks_base/blob/master/core/res/res/layout/alert_dialog_holo.xml
doesn't have this issue.


Reply to this email directly or view it on GitHub
#5 (comment)
.

@fengdai
Copy link
Owner

fengdai commented Oct 26, 2014

No. The platform itself doesn't have that issue. I deleted android:layout_weight="1" by mistake when I ported the layout from the platform.

@AndroidDeveloperLB
Copy link
Author

That's what I meant :
They solved it by using the same thing I've suggested, and you accidentally
removed it.

On Sun, Oct 26, 2014 at 12:52 PM, Feng Dai notifications@github.com wrote:

No. The platform itself doesn't have that issue. I deleted
android:layout_weight="1" by mistake when I ported the layout from the
platform.


Reply to this email directly or view it on GitHub
#5 (comment)
.

@fengdai fengdai closed this as completed Nov 5, 2014
@AndroidDeveloperLB
Copy link
Author

@fengdai Why did you close it? now it's shown like on Lollipop?

@fengdai
Copy link
Owner

fengdai commented Nov 5, 2014

Hi, @AndroidDeveloperLB. I have fixed it. Now it looks the same as the Lollipop's AlertDialog.
You can go to checkout the newest code. If you find some other problems about materal theme, you can open new issues for them respectively. The topic of this issue is a little broad.

@AndroidDeveloperLB
Copy link
Author

@fengdai Nice. But I see the buttons of the dialog on the sides. Did Google make it?
Also, how come the custom View is again minimized in width? It makes my listView to become very thin this way, and it hides the buttons on the bottom this way...
Looking at the previous version, I don't see much difference. How could it be?
How can I fix this?

@fengdai
Copy link
Owner

fengdai commented Nov 6, 2014

@AndroidDeveloperLB can you post screen capture and code snippets?

BTW: I suggest you to open a new issue. Do not combine multiple problems. Write different reports for each problem.

@AndroidDeveloperLB
Copy link
Author

yes, here's a screenshot. I will try to make a new post when I get back home, and maybe add a code to demonstrate it.
device-2014-11-06-082904

@AndroidDeveloperLB
Copy link
Author

Well this is weird. I've tried using the sample app you've created, and it seems to work fine with a listView that I've giving it.
However, when using "material" style, it seems to have quite odd colors.
I'll need to investigate it further.
Thanks for all the help.

@fengdai
Copy link
Owner

fengdai commented Nov 7, 2014

What do you mean by "odd colors"?

@AndroidDeveloperLB
Copy link
Author

A list alone doesn't cause the problem. It's the whole view itself that caused it, but it worked fine before...
ok, I've come up with a sample to show what I mean, but since I can't put files here (except for images), here's a Google drive link instead:
https://drive.google.com/file/d/0B-PZZGk2vPohanpDcWd0YmpEMEE

The sample shows 2 problems that I didn't have on the previous version I've tested:

  1. when choosing the "material" theme (not the light one) , the text color is black on a gray background, hence I call it weird colors. Pretty sure I can fix it, but I don't understand why it happened. It occurs for case How to use a single library, just for "Material-Design" ?  #2 too.
  2. when using my custom view, which has a listView and a spinner, it becomes very narrow. I have no idea why it happens. This doesn't occur on the native alertDialog (I've also added a proof for it, by adding a native dialog option).

Please check it out.
Here are some screenshots:
case #1 , using "material" theme (not the light one) :
device-2014-11-07-133531

case #2 , using custom view (listView+spinner) and "material light" :
device-2014-11-07-133612

case #2, using custom view (listView+spinner) and the native theme :
device-2014-11-07-133620

This all was running on Kitkat. I've also tested it on the Lollipop emulator and for some reason, only case #1 occurred there. case #2 was working just fine...

@fengdai
Copy link
Owner

fengdai commented Nov 8, 2014

  1. when choosing the "material" theme (not the light one) , the text color is black on a gray background, hence I call it weird colors. Pretty sure I can fix it, but I don't understand why it happened. It occurs for case How to use a single library, just for "Material-Design" ?  #2 too.

That's because when you inflate the custom ListView, the platform use the activity's default theme AppTheme. So the text color is black. To fix this, use ContextThemeWrapper:

LayoutInflater.from(new ContextThemeWrapper(MainActivity.this,mTheme)).inflate(R.layout.select_dialog_item,parent,false);

@AndroidDeveloperLB
Copy link
Author

I see. How do I fix the other issue?
On Nov 8, 2014 5:11 AM, "Feng Dai" notifications@github.com wrote:

  1. when choosing the "material" theme (not the light one) , the text
    color is black on a gray background, hence I call it weird colors. Pretty
    sure I can fix it, but I don't understand why it happened. It occurs for
    case How to use a single library, just for "Material-Design" ?  #2 How to use a single library, just for "Material-Design" ?  #2 too.

    That's because when you inflate the custom ListView, the platform use
    the activity's default theme AppTheme. So the text color is back. To fix
    this, use ContextThemeWrapper:

LayoutInflater.from(new ContextThemeWrapper(MainActivity.this,mTheme)).inflate(R.layout.select_dialog_item,parent,false);


Reply to this email directly or view it on GitHub
#5 (comment)
.

@fengdai
Copy link
Owner

fengdai commented Nov 8, 2014

I haven't found the reason. I'll reply you as soon as possible when I find it.

@AndroidDeveloperLB
Copy link
Author

Thank you.

@fengdai
Copy link
Owner

fengdai commented Nov 10, 2014

To some extent this commit 1576799 can fix the second issue.
But this only works in android 3.0 and above. I'll continue digging.

@AndroidDeveloperLB
Copy link
Author

Odd.
How could it be that the previous version of the library worked, and now it doesn't? maybe you could compare between the two?

@fengdai
Copy link
Owner

fengdai commented Nov 11, 2014

The previous version also has this issue, if you run it on Android version below 3.0.

@fengdai
Copy link
Owner

fengdai commented Nov 11, 2014

Modify the dialog_share.xml file. Let the two TextViews' layout_width "match_parent". Then it works fine.

@AndroidDeveloperLB
Copy link
Author

Hey you are right. I wonder why it's different between the APIs...
Also, when using "android.support.v7.widget.LinearLayoutCompat" , it works fine, so it's probably something in how the LinearLayout is implemented.

@AndroidDeveloperLB
Copy link
Author

Say, why did you make the class "CheckedTextViewCompat" ?

@fengdai
Copy link
Owner

fengdai commented Nov 11, 2014

Say, why did you make the class "CheckedTextViewCompat" ?

To draw checkmark to the left of TextView as Lollipop's CheckedTextView.

@AndroidDeveloperLB
Copy link
Author

I see. Maybe you should write about it in this class.

@fengdai
Copy link
Owner

fengdai commented Nov 11, 2014

Fine.

@AndroidDeveloperLB
Copy link
Author

Also, does the "marquee" really do anything in "select_dialog_multichoice_material.xml" ? I ask this since I see that there are no restrictions about the number of lines...
I also don't see the style have any restrictions of this .

Personally I prefer it to be multi-lines instead of being truncated (some languages use long text).

@fengdai
Copy link
Owner

fengdai commented Nov 11, 2014

You are right. It seems do nothing...
Since the platform's "select_dialog_multichoice_material.xml" also contain this line, I prefer to keep it.

@AndroidDeveloperLB
Copy link
Author

Maybe it's a leftover that whoever updated this code didn't notice it won't do anything, or was afraid to ruin something when changing it.

@AndroidDeveloperLB
Copy link
Author

I've noticed another thing:
In the sample project, the "showCustomView" button is invisible, and it doesn't really show a custom view (it uses : "setView(null)" ).

@fengdai
Copy link
Owner

fengdai commented Nov 13, 2014

@AndroidDeveloperLB, I haven't completed it. I found an issue when I tried to set a custom view which contains a single EditText. The EditText can't be clicked to get focus.

@AndroidDeveloperLB
Copy link
Author

Does it work well for Android Lollipop, at least, as the library probably uses the official API if possible?

@fengdai
Copy link
Owner

fengdai commented Nov 13, 2014

I have fixed it. Look at this issue #11 for details.

@AndroidDeveloperLB
Copy link
Author

Thank you.

@fengdai
Copy link
Owner

fengdai commented Nov 13, 2014

Thank you for your help.
I also strongly recommend you to open new issues instead of write them here when you find some other problems.

@AndroidDeveloperLB
Copy link
Author

but they all relate to the fact it wasn't as it was supposed to look like.
So now, if I run a dialog on Lollipop and an AlertDialogPro, both look the same?

On Thu, Nov 13, 2014 at 11:32 AM, Feng Dai notifications@github.com wrote:

Thank you for your help.
I also strongly recommend you to open new issues instead of write them
here when you find some other problems.


Reply to this email directly or view it on GitHub
#5 (comment)
.

@fengdai
Copy link
Owner

fengdai commented Nov 13, 2014

"it wasn't as it was supposed to look like" is not a specific description. This kind of description can contains multiple problems. So it becomes difficult to track a single problem. Write different reports for each problem is better.

@AndroidDeveloperLB
Copy link
Author

I don't understand. What's wrong with the way I've presented it? I've shown description and images...
Now I've found another issue (on the emulator, at least). This time it's with the "Show single-choice list" feature that's shown on the sample.
It seems that when you use it, you need to tap the item in the list twice instead of once.
I've made a new post here:
#13

This time, since it's hard to show in images, I've put a video and full project code.

Also, I wish to ask :
What's the purpose of "list_divider_material_light.xml" and "list_divider_material_dark.xml" ? I've ran Lint and also searched for the usage of them, but they seem unused.

@fengdai
Copy link
Owner

fengdai commented Nov 14, 2014

@AndroidDeveloperLB Thank you for your report. They are unused. I'll delete them.

About #13 , I'll check it out soon. I suppose it's caused by CheckedTextViewCompat.

@AndroidDeveloperLB
Copy link
Author

Thank you.
Please run Lint (and configure it first to show you very interesting things) in order to find such things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants