1169435 - Resources.getColor deprecated

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Android M deprecates Resources.getColor(), which we use in at least one place:

 0:09.71 /Volumes/GeckoDev/gecko/mobile/android/base/../search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java:215: warning: [deprecation] getColor(int) in Resources has been deprecated
 0:09.71             suggestionHighlightColor = context.getResources().getColor(R.color.suggestion_highlight);

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

From the M preview developer documentation:

 public int getColor (int id) 
   This method is deprecated.
   Use getColor(int, Theme) instead.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Mh, getColor(int, Theme) seems to be only available on M+. ResourceCompat in the support library doesn't seem to be helpful here either.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

According to the API overview helper methods should be available in ContextCompat eventually. Either we wait for an updated support library or write our own helper method in the meantime.

http://developer.android.com/preview/api-overview.html#behavior-themeable-colorstatelists

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1169435-colorutils.patch

This patch introduces a new utility class ColorUtils. I replaced all occurrences of getResources().getColor() with ColorUtils.getColor().

This patch contains two todos for the time after we switched to the M SDK:
* Add a call to resources.getColor(color, theme) to ColorUtils.getColor() for API 23+
* After switching to support library 23.* we should be able to use ContextCompat.getColor() making ColorUtils.getColor() obsolete again.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8643029 [details] [diff] [review]
1169435-colorutils.patch

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

::: mobile/android/base/util/ColorUtils.java
@@ +9,5 @@
> +import android.content.res.Resources;
> +import android.support.annotation.ColorRes;
> +import android.support.annotation.NonNull;
> +
> +public class ColorUtils {

Preliminary review: Can this be CompatUtils? We have a similar issue with getDrawable(id, themeId), I think.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #5)
> Preliminary review: Can this be CompatUtils? We have a similar issue with
> getDrawable(id, themeId), I think.

I just realized there's ContextCompat.getDrawable for that.

But maybe we can merge in DrawableUtil.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #6)
> But maybe we can merge in DrawableUtil.

That's what I was going for at first. But then I decided against it because it has nothing to do with Drawables. It's a temporary solution to get the build running anyways. Sooner or later support library 23 will be released and we are going to have it on the builders eventually.

But talking about Drawables. I touched a lot of getColorDrawable() methods that take a color resource and create a ColorDrawable from it. That's something that we could move to DrawableUtil (in another bug).

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I agree with comment 7. :)

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8643029 [details] [diff] [review]
1169435-colorutils.patch

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

Having to get the context differently depending on the file... I bet this was a fun patch. :P

If it works, wfm.

::: mobile/android/base/util/ColorUtils.java
@@ +10,5 @@
> +import android.support.annotation.ColorRes;
> +import android.support.annotation.NonNull;
> +
> +public class ColorUtils {
> +    // TODO: Eventually this functionality should be available in ContextCompat (support library)

File a follow-up to remove this and mention the bug # here.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

I'm going to land this at the beginning of the 43 cycle.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06cd6d6328d3

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/d548ba019a7cfe6cefd567a8d4baa42b904e7639
changeset:  d548ba019a7cfe6cefd567a8d4baa42b904e7639
user:       Sebastian Kaspari <[e-mail]>
date:       Tue Aug 11 11:09:37 2015 +0200
description:
Bug 1169435 - Replace deprecated Resources.getColor() with call to ColorUtils.getColor(). r=mcomella

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d548ba019a7c