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

Fixed a little thing in licenses text loading and added some methods for customization #30

Closed
wants to merge 3 commits into from

Conversation

barondem
Copy link
Contributor

Fixed missing space in while construct during licenses text loading (I don't think it's done that way on purpose) and added three methods to customize the dialog theme and title divider with some tricks found on the Web.

public void show() {
create().show();
}

public void show(int themeResourceId) {
create(themeResourceId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are missing the show() call here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Fixed.

@hameno
Copy link
Member

hameno commented Jul 21, 2014

Could you please also implement this for the Fragment?

@barondem
Copy link
Contributor Author

I already thought to this and I'll try to do this; if it works, I'll commit it too.

@barondem
Copy link
Contributor Author

OK, it works with dialogfragment; maybe you'd like to check the code...
Anyway, I'd like to ask you a few things: is there a precise reason because of which you didn't include any parameters for custom title and close button in the dialogfragment newInstance methods?
What about a dedicate version for who wants to develop only for API level 14+ for example (i.e. a version which uses FragmentManager from framework and not from support library), because at the moment the dialogfragment can't be used if the activity isn't extending fragmentactivity or actionbaractivity.

});
final AlertDialog.Builder builder;
if (mThemeResourceId != 0) {
builder = new AlertDialog.Builder(new ContextThemeWrapper
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the line break

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@hameno
Copy link
Member

hameno commented Jul 23, 2014

is there a precise reason because of which you didn't include any parameters for custom title and close button in the dialogfragment newInstance methods?

Yes, because you can simply override those inside your resources.

What about a dedicate version for who wants to develop only for API level 14+ for example (i.e. a version which uses FragmentManager from framework and not from support library), because at the moment the dialogfragment can't be used if the activity isn't extending fragmentactivity or actionbaractivity.

Maybe, but I'm still only using the FragmentActivitys. IMHO to have consistent behavior through all platform versions you should always use the Fragments from the support library.

@hameno
Copy link
Member

hameno commented Jul 27, 2014

Okay, just one little thing left: could you please squash all the commits into one? (see http://rebaseandsqua.sh/ for more info)

@barondem
Copy link
Contributor Author

Tried that script bit it says no local chance to save... Could it be an idea to do a new pull request?

@hameno
Copy link
Member

hameno commented Jul 27, 2014

No, try this http://davidwalsh.name/squash-commits-git

@barondem
Copy link
Contributor Author

I couldn't manage to squash the commits like you suggested, so I rolled back to your last commit and made only one commit; maybe it's not the most straightforward way, but I think it should work.

@barondem
Copy link
Contributor Author

Just a few other questions:

  1. Can you tell me the part of code (or the xml file) which makes the dialog title and button have a light gray background? I think it's not the default style because if I create and show a common dialog it just has a plain white background also on title and button
  2. If I'd need to create a custom version of this to use framework dialog (and framework fragment manager), I should also remove/alter your code about on dismiss listener; is it all strictly necerssary? Should it be safe to remove some setOnDismissListener call?

@hameno
Copy link
Member

hameno commented Jul 27, 2014

  1. There is no code which handles this. It is all androids default style. I depends on the targeted api version.
  2. The only difference should be with the imports.

@barondem
Copy link
Contributor Author

  1. OK, though I'm not sure about this, since I tried to change target and compile API version without any visual change; maybe I'll investigate more deeply
  2. I also thought this but this code causes an exception and a crash with framework dialog:
    dialog.setOnDismissListener(new DialogInterface.OnDismissListener() {
    @OverRide
    public void onDismiss(final DialogInterface dialog) {
    if (mOnDismissListener != null) {
    mOnDismissListener.onDismiss(dialog);
    }
    }
    });
    So, I modified this by deleting it from its position, implementing the interface in the outer class and overriding the method there with your code; I didn't find any drawback for now. What do you think about it?

@hameno
Copy link
Member

hameno commented Jul 29, 2014

  1. What was the exception?

Regarding you latest commit: I don't like the massive code duplications. Please improve this by reusing more code (like with show and createAndShow -> should be create().show()). Also please use delegating constructors to remove duplication.

@barondem
Copy link
Contributor Author

First, sorry for the late response; second, it was an IllegalStateException( "You can not set Dialog's OnCancelListener or OnDismissListener"); third, I'll try to optimize the code, where possible...

@barondem
Copy link
Contributor Author

Optimization done: source code is now 48 lines lighter, if I'm not wrong; I used delegating constructors like you recommended and I also fixed some really ugly bug (my fault, honestly) in show() and createAndShow() methods. Shall I commit?

@hameno
Copy link
Member

hameno commented Aug 14, 2014

Sure, let me see it.

@barondem
Copy link
Contributor Author

Pushed commit.

@hameno hameno mentioned this pull request Aug 21, 2014
@hameno
Copy link
Member

hameno commented Aug 23, 2014

I have reviewed your changes. I've made some changes to replace the excess of constructors with a builder class. Can you please checkout https://github.com/PSDev/LicensesDialog/tree/pr/30 and report back if your theme handling still works? Also it would be great if you could rebase this pull-request onto this branch and add a sample to the sample module.

@barondem
Copy link
Contributor Author

Looks good; just a thing: in the Builder class I think the method should be setDividerColor(final int dividerColor) as well as the local variable should be mDividerColor since the passed argument is assumed to be actually the color value, not its I'd (and I don't see anywhere a line of code which does int dividerColor = getResources.getColor(mDividerColorId);). Hence if a developer passes a color id I think it wouldn't work and it would be really sad. So, I think you should either change those names or retrieve the color somewhere in the code (or even better create an overload to allow both the usages).

@barondem
Copy link
Contributor Author

Tried and in fact it doesn't work with a color id. Oh, and in LicensesDialogFragment's onCreateDialog it should be .show(), not .create() (last line); the divider color isn't set otherwise.

@barondem
Copy link
Contributor Author

Any news about this pull request?

@hameno
Copy link
Member

hameno commented Sep 16, 2014

Sorry, I have been very busy. I'll try to take a look at it soon.

@barondem
Copy link
Contributor Author

OK, if you want I could commit the necessary fixes in the meantime.

@barondem
Copy link
Contributor Author

I added custom theme samples and proposed fixes and additions for some things and committed and made a pull request on the pr/30 branch, as you asked. It should be quite good now, I think.
Oh, I saw custom themes don't work on gingerbread (with touchwiz, to say the truth), maybe the trick I used isn't supported there. Though, it doesn't crash, at least, and works really good on newer platforms.

@hameno
Copy link
Member

hameno commented Sep 22, 2014

I merged your PR to my branch - will take a final look at it soon and hopefully do the release this week.

@hameno
Copy link
Member

hameno commented Sep 27, 2014

Manually merged

@hameno hameno closed this Sep 27, 2014
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

Successfully merging this pull request may close these issues.

2 participants