Closed Bug 1312114 Opened 8 years ago Closed 8 years ago

Improve VectorDrawable support to allow using them in app menu and tabs tray

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: ahunt, Assigned: ahunt)

References

Details

(Whiteboard: [MobileAS])

Attachments

(6 files)

Bug 1312114 - Pre: Move getDrawable() from (geckoview) BitmapUtils into (fennec) MenuBitmapUtils to allow support library access
58 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1312114 - Support VectorDrawable's in ResourceDrawableUtils
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1312114 - Add VectorDrawable support to GeckoMenu
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1312114 - Post: make catch more specific to not hide exceptions
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1312114 - Post: cleanup imports
58 bytes, text/x-review-board-request
ahunt
: review+
Details
Bug 1312114 - Post: centralise AppCompatDrawableManager calls to allow easy replacement
58 bytes, text/x-review-board-request
sebastian
: review+
Details 
Bug 1300144 adds a bunch of VectorDrawable's that will be used in the new ActivityStream context menu.

Some of these are duplicates of existing png resources - we should try and replace those if possible.

Note: the existing icons aren't 1:1 copies, moreover VectorDrawable's can't be used everywhere on Android 4 devices, hence we need to be careful about testing every replacement. E.g. updating the share icon in the main and context menus is requiring changes to icon loading code.
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [MobileAS]
Depends on: 1300144
Iteration: --- → 1.7
Comment on attachment 8803544 [details]
Bug 1312114 - Pre: Move getDrawable() from (geckoview) BitmapUtils into (fennec) MenuBitmapUtils to allow support library access

https://reviewboard.mozilla.org/r/87778/#review86812

stamp!  FYSA, we don't want to force GV consumers to be appcompat-v7 consumers.  So this is great!

FYI, I'm on leave until January.  mcomella and sebastian are good reviewers for build stuff, jchen and snorp for build and GV stuff.
Attachment #8803544 - Flags: review?(nalexander) → review+
Did you talk to Bryan and/or Antlam about this? Afaik they have been meeting in Toronto last week and also talked about design/icon consistency.
Comment on attachment 8803545 [details]
Bug 1312114 - Support VectorDrawable's in ResourceDrawableUtils

https://reviewboard.mozilla.org/r/87780/#review86962

::: mobile/android/base/java/org/mozilla/gecko/util/ResourceDrawableUtils.java:118
(Diff revision 1)
>          }
>  
>          if (data.startsWith("drawable://")) {
>              final Uri imageUri = Uri.parse(data);
>              final int id = getResource(context, imageUri);
> -            final Drawable d = context.getResources().getDrawable(id);
> +            final Drawable d = AppCompatDrawableManager.get().getDrawable(context, id);

It seems like this class is hidden in later releases of the support library.

Hopefully there's a different approach we can use later?

https://android.googlesource.com/platform/frameworks/support/+/master/v7/appcompat/src/android/support/v7/widget/AppCompatDrawableManager.java#64
Attachment #8803545 - Flags: review?(s.kaspari) → review+
Comment on attachment 8803546 [details]
Bug 1312114 - Add VectorDrawable support to GeckoMenu

https://reviewboard.mozilla.org/r/87782/#review86964
Attachment #8803546 - Flags: review?(s.kaspari) → review+
Comment on attachment 8803547 [details]
Bug 1312114 - Post: make catch more specific to not hide exceptions

https://reviewboard.mozilla.org/r/87784/#review86966

r+++++
Attachment #8803547 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Comment on attachment 8803545 [details]
> Bug 1312114 - Support VectorDrawable's in ResourceDrawableUtils
[...]
> > +            final Drawable d = AppCompatDrawableManager.get().getDrawable(context, id);
> 
> It seems like this class is hidden in later releases of the support library.
> 
> Hopefully there's a different approach we can use later?
> 
> https://android.googlesource.com/platform/frameworks/support/+/master/v7/
> appcompat/src/android/support/v7/widget/AppCompatDrawableManager.java#64

I've been spending some time digging into this, but am not really sure what we want to do here:

- Support library 24.2.0 introduces AppCompatResources.getDrawable():
https://developer.android.com/topic/libraries/support-library/revisions.html#24-2-0-api-updates
API docs:
https://developer.android.com/reference/android/support/v7/content/res/AppCompatResources.html

That means we at least have a solution once we're on >= 24.2. (Note: I haven't actually tested this, so maybe there are unknown issues lurking there...)


Which leaves 23.4.0: I can't actually find older versions of AppCompatDrawableManager, and it's entirely possible that it's hidden even on 23.4.0. (I.e. on androidxref, I can only find AppCompatDrawableManager when searching in 7.0, but not in 6.0 or earlier - I'm going to see if I can just find the upstream support library repo.)

However it does work without issues on the devices I've tested, and it's also what the various AppCompat widgets are using (e.g. AppCompatImageView). So if we stick to the same API, then we *might* still be ok with using it for 23.4.0. I don't feel particularly comfortable doing that though, so I'm going to try and do some more research and testing...
I wonder if loading the drawables using VectorDrawableCompat.create() would be a better idea for now. That would however require checking the type of drawable before inflation and/or trying normal inflation, and falling back if normal inflation throws an Exception because of a <vector>.

(Which is essentially what AppCompatDrawableManager does: it iterates over various load-delegates, and finally tries the default Resoureces.getDrawable() if no other method succeeds.)
(In reply to Andrzej Hunt :ahunt from comment #12)
> That means we at least have a solution once we're on >= 24.2. (Note: I
> haven't actually tested this, so maybe there are unknown issues lurking
> there...)

Sounds good! As soon as we are building with SDK 24/25 we can switch to the latest support library.

> Which leaves 23.4.0: I can't actually find older versions of
> AppCompatDrawableManager, and it's entirely possible that it's hidden even
> on 23.4.0. (I.e. on androidxref, I can only find AppCompatDrawableManager
> when searching in 7.0, but not in 6.0 or earlier - I'm going to see if I can
> just find the upstream support library repo.)
> 
> However it does work without issues on the devices I've tested, and it's
> also what the various AppCompat widgets are using (e.g. AppCompatImageView).
> So if we stick to the same API, then we *might* still be ok with using it
> for 23.4.0. I don't feel particularly comfortable doing that though, so I'm
> going to try and do some more research and testing...

23.4 is what we are using. If it compiles then it shouldn't be hidden and we can just use it for now. I was just wondering if there's something we can switch to later (24.x / 25.x). :)
Iteration: 1.7 → 1.8
Summary: Investigate replacing relevant drawables with the new ActivityStream VectorDrawables → Improve VectorDrawable support to allow using them in app menu and tabs tray
I've filed Bug 1314902 to do the actual drawable replacement, I'm going to focus on landing the code changes needed for VectorDrawable support in this bug.
Comment on attachment 8803549 [details]
Bug 1312114 - Post: centralise AppCompatDrawableManager calls to allow easy replacement

https://reviewboard.mozilla.org/r/87788/#review90066
Attachment #8803549 - Flags: review?(s.kaspari) → review+
Comment on attachment 8803548 [details]
Bug 1312114 - Post: cleanup imports

https://reviewboard.mozilla.org/r/87786/#review90146
Attachment #8803548 - Flags: review+
Pushed by ahunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9cf52476e2aa
Pre: Move getDrawable() from (geckoview) BitmapUtils into (fennec) MenuBitmapUtils to allow support library access r=nalexander
https://hg.mozilla.org/integration/autoland/rev/23668d4083a4
Support VectorDrawable's in ResourceDrawableUtils r=sebastian
https://hg.mozilla.org/integration/autoland/rev/4c5989da040c
Add VectorDrawable support to GeckoMenu r=sebastian
https://hg.mozilla.org/integration/autoland/rev/93f705ff5404
Post: make catch more specific to not hide exceptions r=sebastian
https://hg.mozilla.org/integration/autoland/rev/07ebcbebe013
Post: centralise AppCompatDrawableManager calls to allow easy replacement r=sebastian
https://hg.mozilla.org/integration/autoland/rev/4be064a14b66
Post: cleanup imports r=ahunt
See Also: → 1310143
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: