Closed Bug 1084529 Opened 5 years ago Closed 5 years ago

Move hardcoded colors to resources

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: mcomella, Assigned: orangeshark, Mentored)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(2 files, 1 obsolete file)

Attached file Potential matches
Some of our colors are hard-coded into the code, but should really be declared in resources. See the attachment for some potential matches.

For updated line numbers, grep for yourself (the arguments are likely the same for grep and ag):

ag "0x[A-Fa-f0-9]{8,10}"

If these colors are retrieved often, consider storing them as member variables.
Hello, I would like to work on this. Just to be clear, color resources should be declared in mobile/android/base/resources/values/colors.xml? 

Should color values which are used in bitwise operations like FaviconView.java's "mDominantColor & 0x46FFFFFFFF" also be declared in resources?

Finally for the regular expression, would "0x[A-Fa-f0-9]{6,8}" be more appropriate? It would include color values of 0xRRGGBB. I already used grep and only noticed two hard-coded values of the 0xRRGGBB type that could be possibly replaced with declarations in resources.
(In reply to Erik Edrosa (:orangeshark) from comment #1)
> Hello, I would like to work on this.

Great! Get started; please upload a patch for Michael to review when you have something working!


> Just to be clear, color resources
> should be declared in mobile/android/base/resources/values/colors.xml? 

Mostly, yes.

Colors that are used in non-'base' resources -- fxaccount, for example, but those are already in base/resources/values/fxaccount_colors.xml -- can stay where they are.

Those will pop up during review.


> Should color values which are used in bitwise operations like
> FaviconView.java's "mDominantColor & 0x46FFFFFFFF" also be declared in
> resources?

That particular '&' is for transparency (note that there are four hex pairs in the color).

In general, bitwise masks should not be lifted to resources.



The colors I see that aren't related to transparency, and are thus candidates for moving to resources:

search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java
52:    private static final int SUGGESTION_HIGHLIGHT_COLOR = 0xFF999999;

base/toolbar/BackButton.java
44:        mBorderPaint.setColor(isPrivate ? 0xFF363B40 : 0xFFB5B5B5);

base/toolbar/CanvasDelegate.java
35:        mPaint.setColor(0xFFFF0000);

base/toolbar/ForwardButton.java
43:        mBorderPaint.setColor(isPrivate ? 0xFF363B40 : 0xFFBFBFBF);

base/home/TopSitesThumbnailView.java
29:    private static final int DEFAULT_COLOR = 0xFFECF0F3;
40:        sBorderPaint.setColor(0xFFCFD9E1);

Particular instances might get ruled out in review, but we'll cross those bridges when we come to them.
Hey, Erik - welcome to Bugzilla! I've assigned you to the bug.

Do you already have set up a build environment? If not, you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

(In reply to Erik Edrosa (:orangeshark) from comment #1)
> Finally for the regular expression, would "0x[A-Fa-f0-9]{6,8}" be more
> appropriate?

Yep! That was a mistake on my part.

Note that the BackButton.java and ForwardButton.java changes that rnewman recommended in comment 2 have been fixed in bug 1084545. You can see that bug for an example of what needs to be changed here in the other files.

Thanks for your help and good luck!
Assignee: nobody → erik.edrosa
Status: NEW → ASSIGNED
Hello Michael,

I have already set up a build environment and built Fennec.

search/java/org/mozilla/search/autocomplete/SuggestionsFragment.java
52:    private static final int SUGGESTION_HIGHLIGHT_COLOR = 0xFF999999;

I don't believe I will be able to declare this as a resource because SUGGESTION_HIGHLIGHT_COLOR is later used in an inner static class. I have not seen any method to get the resources for use in a static context.

I have made changes to CanvasDelegate.java and TopSitesThumbnailView. Can you provide feedback on the attached patch?
Attachment #8508145 - Flags: feedback?(michael.l.comella)
Comment on attachment 8508145 [details] [diff] [review]
Move hardcoded colors to resources.

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

This is looking pretty good to me, Erik!

(In reply to Erik Edrosa (:orangeshark) from comment #4)
> I don't believe I will be able to declare this as a resource because
> SUGGESTION_HIGHLIGHT_COLOR is later used in an inner static class. I have
> not seen any method to get the resources for use in a static context.

After doing a bit of googling, I found [1]. Seems fine to me, but I'm not sure how other people on the team might feel about this.

Richard, what do you think? ^ (You don't have to review the whole patch)

[1]: http://stackoverflow.com/a/4391811

::: mobile/android/base/toolbar/CanvasDelegate.java
@@ +31,5 @@
>  
>          // DST_IN masks, DST_OUT clips.
>          mMode = new PorterDuffXfermode(mode);
>  
> +        final Resources res = ((ShapedButton) drawManager).getResources();

I don't think we should assume the drawManager will always be a `ShapedButton` - I think you can expand the CanasDelegate constructor to also take a `Context`.
Attachment #8508145 - Flags: feedback?(rnewman)
Attachment #8508145 - Flags: feedback?(michael.l.comella)
Attachment #8508145 - Flags: feedback+
(In reply to Michael Comella (:mcomella) from comment #5)
> Comment on attachment 8508145 [details] [diff] [review]
> Move hardcoded colors to resources.
> 
> Review of attachment 8508145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking pretty good to me, Erik!
> 
> (In reply to Erik Edrosa (:orangeshark) from comment #4)
> > I don't believe I will be able to declare this as a resource because
> > SUGGESTION_HIGHLIGHT_COLOR is later used in an inner static class. I have
> > not seen any method to get the resources for use in a static context.
> 
> After doing a bit of googling, I found [1]. Seems fine to me, but I'm not
> sure how other people on the team might feel about this.
> 
> Richard, what do you think? ^ (You don't have to review the whole patch)
> 
> [1]: http://stackoverflow.com/a/4391811

I saw the same solution, but was unsure if it would be a good idea to implement.

> I don't think we should assume the drawManager will always be a
> `ShapedButton` - I think you can expand the CanasDelegate constructor to
> also take a `Context`.

Alright, I will do this.
I'm generally not in favor of adding context references to classes when it's not strictly necessary. It's a bad pattern and it easily goes wrong (e.g., accidentally holding a reference to an activity).

I suggest one of two ways forward:

* Ignore it.

* Lift the highlight color into SuggestionAsyncLoader, where a context is available. Pass the highlight color (same instance each time) into the Suggestion constructor on line 223. Eliminate COLOR_SPAN altogether.

That is, instead of the highlight color being an intrinsic part of the suggestion, it's an input at the point of loading. This not only makes more sense to me, but it solves this problem.
Comment on attachment 8508145 [details] [diff] [review]
Move hardcoded colors to resources.

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

::: mobile/android/base/home/TopSitesThumbnailView.java
@@ +26,5 @@
>      // 27.34% opacity filter for the dominant color.
>      private static final int COLOR_FILTER = 0x46FFFFFF;
>  
>      // Default filter color for "Add a bookmark" views.
> +    private final int DEFAULT_COLOR = getResources().getColor(R.color.top_site_default);

If this isn't static, don't name it AS_A_CONSTANT.

@@ +48,5 @@
>  
>      public TopSitesThumbnailView(Context context, AttributeSet attrs, int defStyle) {
>          super(context, attrs, defStyle);
> +
> +        // Initializing the border paint.

Initialize

::: mobile/android/base/toolbar/CanvasDelegate.java
@@ +31,5 @@
>  
>          // DST_IN masks, DST_OUT clips.
>          mMode = new PorterDuffXfermode(mode);
>  
> +        final Resources res = ((ShapedButton) drawManager).getResources();

ShapedButton is a ThemedImageButton. It's safe to assume that it wants to manage colors and themes, so why not simply have the CanvasDelegate take a `Paint` argument here?
Attachment #8508145 - Flags: feedback?(rnewman) → feedback+
I updated the patch based on feedback from both Michael and Richard.

For the search highlight color, I followed the second suggestion described by Richard. I declared the highlight color in the resource file search_colors.xml which contains other colors related to search.
Attachment #8509204 - Flags: review?(michael.l.comella)
Comment on attachment 8509204 [details] [diff] [review]
Move hardcoded colors to resources with changes based on feedback

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

Looking pretty good - just a stylistic change!

Here's a run of your patch in our Try test suite: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d120c81cfb7

::: mobile/android/base/home/TopSitesThumbnailView.java
@@ +32,5 @@
>      // Stroke width for the border.
>      private final float mStrokeWidth = getResources().getDisplayMetrics().density * 2;
>  
>      // Paint for drawing the border.
> +    private final Paint mBorderPaint;

Nice catch.

::: mobile/android/base/resources/values/search_colors.xml
@@ +27,5 @@
>  
>      <color name="network_error_link">#0092DB</color>
> +
> +    <!-- Suggestion highlight color -->
> +    <color name="suggestion_highlight">#FF999999</color>

Actually, this should be in m/a/b/resources/values/colors.xml.

Any file prepended with "search_" is actually a component of our search activity (the "Nightly Search" application installed along with the latest nightlies).

Thanks for trying to be thorough!
Attachment #8509204 - Flags: review?(michael.l.comella) → feedback+
(In reply to Michael Comella (:mcomella) from comment #10)
> ::: mobile/android/base/resources/values/search_colors.xml
> @@ +27,5 @@
> >  
> >      <color name="network_error_link">#0092DB</color>
> > +
> > +    <!-- Suggestion highlight color -->
> > +    <color name="suggestion_highlight">#FF999999</color>
> 
> Actually, this should be in m/a/b/resources/values/colors.xml.
> 
> Any file prepended with "search_" is actually a component of our search
> activity (the "Nightly Search" application installed along with the latest
> nightlies).
> 
> Thanks for trying to be thorough!

I thought the SuggestionsFragment.java file was part of the "Nightly Search" application because it is found in the mobile/android/search/ directory.
Comment on attachment 8509204 [details] [diff] [review]
Move hardcoded colors to resources with changes based on feedback

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

(In reply to Erik Edrosa (:orangeshark) from comment #11)
> I thought the SuggestionsFragment.java file was part of the "Nightly Search"
> application because it is found in the mobile/android/search/ directory.

A total oversight on my part - lgtm.

Once the try push goes green, feel free to add the checkin-needed [1] keyword to get your patch checked in! Note that all patches using "checkin-needed" need a green try push to be landed.

Thanks, Erik!

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8509204 - Flags: feedback+ → review+
The try push [1] is green. Adding the checkin-needed keyword.

[1]: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d120c81cfb7
Keywords: checkin-needed
Comment on attachment 8508145 [details] [diff] [review]
Move hardcoded colors to resources.

I'm assuming this is obsolete and shouldn't be checked-in
Attachment #8508145 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/c77cb1e0d919
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c77cb1e0d919
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.