Revise LightweightTheme API




Firefox for Android
Theme and Visual Design
3 years ago
3 years ago


(Reporter: mcomella, Unassigned)


(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)


The API is currently confusing, e.g.:
  * LightweightTheme has:
    - getDrawable(View): returns a Drawable with the bitmap
    - getColorDrawable(View): returns a Drawable with the accent color in the background where the bitmap does not fill
    - getColorDrawable(View, int): returns a Drawable with the specified color in the background where the bitmap does not fill
    - getColorDrawable(View, int, boolean): with true, filters the given color with the accent color and uses that as the background where the bitmap does not fill
  These could be better named:
    - getBitmapDrawable
    - getAccentColorDrawable
    - getColorDrawable
    - getAccentColorFilteredColorDrawable
  Or getDrawable*, to make it less grammatical but easier to figure out the API.

  * LightweightThemeDrawable is layered with a background color layer and a bitmap layer. The bitmap layer can also be set up to use a gradient alpha, but this is not specified (and probably shouldn't be initialized in onBoundsChange). Some methods act on one and some act on the other, neither is specified. It has:
    - setAlpha(int) (bitmap layer)
    - setAlpha(int, int) initializes the LinearGradient shader (bitmap layer)
    - setColor(int) (background layer)
    - setColorWithFilter(int, int) (background layer)
    - setColorFilter(ColorFilter) (bitmap layer)
  LwTD could be set up to contain each layer as a separate member and we can call methods on those, one layer could be in the Drawable and the other a member, or the names could represent which layer they work on. Additionally, the LinearGradient-enabled drawable could be a subclass.

Note also that the Drawable can cause graphic artifacts if there is no under layer and an alpha of 255 is not specified as part of the color in setColor(int).

Make this less confusing.
In my brief testing, LightweightThemeDrawable.setAlpha(int) only showed the background layer with alpha=255, while setAlpha(int, int) showed the alpha values I expected, so that sounds broken.

Also, Themed* has getColorDrawable, which is confusing given LightweightTheme.getColorDrawable.
Be careful when revising setAlpha(int) and setAlpha(int, int) - BrowserToolbarTabletBase.setButtonEnabled changes the alpha of the button to make it seem disabled.
You need to log in before you can comment on or make changes to this bug.