1122776 - Remove whitespace from menu drawable pngs

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

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

The menu (hamburger button) png drawables (.../m/a/b/resources/drawable-*/ic_menu*.png, e.g. [1]) contain whitespace - remove the whitespace without distorting the image content (I recommend using image magick) and then add padding to the appropriate views to compensate for the padding removed from the image.

Double-check to make sure these drawables are not used outside of the action bar as well because this change will mess with those too!

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable-xhdpi/ic_menu_bookmark_add.png

Comment 1

[Anish]

I used convert -trim ic_menu_bookmark_add.png crop.png to trim the spaces is that enough?

Comment 2

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

(In reply to Anish from comment #1)
> I used convert -trim ic_menu_bookmark_add.png crop.png to trim the spaces is
> that enough?

According to the documentation, looks appropriate! However, it's best to use your judgment! Open the asset file: is the whitespace border missing from the asset? If so, great! If not, try something else!

By the way, you can use the "Need more information" field at the bottom of the bug to make a more noticeable message to either Martyn, myself, or anyone else that you may need info from!

Comment 3

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

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html

Comment 4

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

Unassigning due to inactivity - let me know if you're still working on this, Anish.

Comment 5

[Anirudh S [:anirudh24seven]]

Should we really add padding to the appropriate Views? I am not sure if that will scale well to changes. The View would then become tightly-coupled to the image size. 

Ideally, the View should not be worried about drawable size. If someone is changing the images later, the Views might not behave in an expected manner.

Comment 6

[Anirudh S [:anirudh24seven]]

How about creating xml drawable files that point to the png drawable files and provide padding inside those xml drawables, instead? I think that might work long-term. When someone makes changes to the png drawables, they'll have to ensure that they change padding in the xml drawables as well.

But I don't know if this approach is more performant, compared to the existing scenario. If it isn't, we would have to think of a different solution.

Comment 7

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

(In reply to Anirudh S [:anirudh24seven] from comment #5)
> Should we really add padding to the appropriate Views? I am not sure if that
> will scale well to changes. The View would then become tightly-coupled to
> the image size.

(In reply to Anirudh S [:anirudh24seven] from comment #6)
> How about creating xml drawable files that point to the png drawable files
> and provide padding inside those xml drawables, instead? I think that might
> work long-term. When someone makes changes to the png drawables, they'll
> have to ensure that they change padding in the xml drawables as well.

These sound like the same solution to me except the padding is better encapsulated to a drawable rather than directly on the View in your solution. In theory, this sounds good to me but in practice, I think it's messy. Android creates a lot of resource files and references that require you to open multiple files in order to figure out what a View is doing - it's much easier to edit if it's all in the same place, directly where the View is declared. This also prevents the clutter of extra files in resource directories and of references in resource files. I feel the best use for using these named references (e.g. a style, a drawable, etc.) is when it can't be done any other way (e.g. shape drawable) and when it is used in multiple places.

Do you disagree, Anirudh?

Comment 8

[Anirudh S [:anirudh24seven]]

> In theory, this sounds good to me but in practice, I think it's messy. 

I agree. My suggestion was not a clean solution either.

How about just ensuring that cropped images are centered in their Views and not worry about padding? This solution will also not cause maintenance headaches when the images are changed in the future.

Comment 9

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

Sorry for the delay.

(In reply to Anirudh S [:anirudh24seven] from comment #8)
> How about just ensuring that cropped images are centered in their Views and
> not worry about padding? This solution will also not cause maintenance
> headaches when the images are changed in the future.

Sounds ideal, provided the containing View is sized correctly to properly center the content (or have the View be centered in the parent).

Just be sure the url bar does not actually change size.

Comment 10

[Anirudh S [:anirudh24seven]]

I have made a few observations which I feel are important:


1. I am positive that if the images are set as source/icon/background for a View using only Android APIs, they are centered by default. I checked the Fennec source code and this is how the ic_menu_* images are set within the app (No other custom/unexpected way).


2. Some images have different padding values on each side of the image. If we use auto-cropping tools like ImageMagick's trim feature, we will end up with images that won't look appropriate if we use them directly, as they won't 'appear' centered. [eg. ic_menu_addons.png]

Therefore, I am using Gimp's 'Change Canvas Size' feature to manually remove padding and still ensure that images are centered after removing as much padding as possible. This will result in a little padding on some sides of the image though, as I want the image to 'appear' centered.


3. While changing the Canvas size, I will remove equal padding on all sides. This means, I will have to remove even number of pixels (1px on left + 1px on right) so that one side doesn't have an extra pixel.

For example, if I want to change canvas size of a 63x63 image, I will have to change it to 57x57 instead of 56x56. 63-57 = 8 (an even number). If I use 56x56, the left side or the right side will have an extra pixel.



I hope I'm not over-complicating things here.

Comment 11

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

These will be removed in due time when necessary.