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

Add ability to resolve attr for view styles #187

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jlandrum
Copy link

@jlandrum jlandrum commented Jul 8, 2015

Currently, setting the font to an attr value causes it to look for a font named - for example - "?attr/myThemeSpecificFont" instead of resolving it to the theme's value. This adds support for setting fontPath to an attr value per view.

@chrisjenx
Copy link
Owner

Quite nice addition. checking the value for ?attr seems odd. This should work without any special use case. (as in when resolving the style it should work out the reference for us).

@chrisjenx
Copy link
Owner

For example. context.obtainStyledAttributes( attrs, com.android.internal.R.styleable.View, defStyleAttr, defStyleRes); will resolve background resource correctly when you do: android:background="?attr/backgroundWindow" etc.

This might be an underlying issue with the way we resolve the fontPath attribute.

@jlandrum
Copy link
Author

jlandrum commented Jul 9, 2015

I haven't fully tested every case to make sure absolutely nothing broke - would be nice if it were possible to have sent this as a fix - maybe I just don't know GitHub as well as I know git but I didn't see how to do that. As far as resolving ?attr properties, it's been working wonderfully though.

I included a check for "?" to offset any possible notable performance hit (I did notice a few StackOverflow comments mentioning some portions of attr resolution using iteration), but I ended up going the route I did as obtainStyledAttributes was resolving to android.R instead of my application's package, even with the given context (From the examples I was reading, that shouldn't be the case - not sure if it's a bug or just my own misunderstanding of how obtainStyledAttributes should work)

The main purpose of doing this is the fact we have a product with a theme for the visually impaired that gets changed onCreate - and I needed a way to switch 4 different fonts. I was also under the impression that ? and @ were supposed to auto resolve, but as this library does make use of classes with minimal documentation as Google intended them for internal use, I'm sure you understand how difficult it was to make complete sense of how the system works internally.

@chrisjenx
Copy link
Owner

@jlandrum @string/myFontPath will autoresolve, if not then that might be bug which is causing the issues you are seeing.

I wonder if changing the activity theme breaks it as you would need to recreate the baseContext with the new theme to lookup. When you change the theme do you recreate the activity?

@jlandrum
Copy link
Author

jlandrum commented Jul 9, 2015

Sorry, when I said "? and @" I meant that parts of the documentation imply that it will resolve both but it only resolves "@."

@chrisjenx
Copy link
Owner

Ahh OK, that is odd then. :/

On Thu, 9 Jul 2015 15:48 James Landrum [email protected] wrote:

Sorry, when I said "? and @" I meant that parts of the documentation imply
that it will resolve both but it only resolves "@."


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

@jachenry
Copy link

jachenry commented Mar 9, 2017

We came across this issue when trying to supply custom attribute to fontPath in styles.

  <declare-styleable name="ProjectTheme">
    <attr name="fontPathPrimary" format="reference"/>
  </declare-styleable>

  <style name="TextAppearance.Project.Headline1">
    <item name="android:textSize">@dimen/project_text_size_headline_1</item>
    <item name="fontPath">?attr/fontPathPrimary</item>
  </style>

Is this related to this merge? Any plans on pushing this forward?

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.

3 participants