Default Image for Contacts When Image is Missing

RESOLVED FIXED in 2.1 S3 (29aug)

Status

Firefox OS
Gaia::Contacts
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Sabrina Smelko, Assigned: ivanovpavel, NeedInfo)

Tracking

unspecified
2.1 S3 (29aug)
x86
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(feature-b2g:2.1, tracking-b2g:backlog)

Details

Attachments

(12 attachments)

(Reporter)

Description

4 years ago
Default Imagery will be used wherever images are missing or not assigned. In this case, when a contact doesn't have a contact image, the default image will appear (in contact list, incoming call and outgoing call etc.)

Using CSS, we can crop the attached black and white illustration and apply a CSS overlay (colour-burn at 100%) with hex colour code #26D1C4.

This way, the image fits in to the comms style without standing out too much. And by randomizing the crop, image will always appear fresh and not repetitive.

Box link for the work/concepts: https://mozilla.app.box.com/files/0/f/2097944726/Source

Box link for the b/w image: https://mozilla.app.box.com/files/0/f/2097944726/1/f_19314621437
(Reporter)

Comment 1

4 years ago
Created attachment 8461679 [details]
black and white base image.
(Reporter)

Updated

4 years ago
Flags: needinfo?(pivanov)
(Reporter)

Updated

4 years ago
Blocks: 945225
(Reporter)

Updated

4 years ago
Blocks: 1026689
I think that we should use colored image because we can't `color-burn` the image like in PS, because of this:

Photoshop:
================================================================
Document who have 2 layers:
    Over  : The Color (#26D1C4) - Blending Mode (Color Burn)
    Under : The Imagery         - Blending Mode (Normal)


HTML & CSS:
================================================================
Element with Blending Mode (Color Burn) who have:
    Over  : The Imagery         
    Under : The Color (#26D1C4)


for Contacts App we need Imagery only for thumbnail, right?
If yes I will use for example 70x124 pixels image and I will try to show random part of if (right top,  center bottom, cender center etc.)

Are you OK with this?
Flags: needinfo?(pivanov) → needinfo?(ssmelko)
The box links are not publicly accessible. Could you make them accessible Sabrina?
Assignee: nobody → pivanov
Created attachment 8462498 [details] [review]
patch for Gaia/master
Attachment #8462498 - Flags: ui-review?(ssmelko)
(Reporter)

Comment 5

4 years ago
Created attachment 8462602 [details]
Default_Imagery_Contacts.jpg

Actually Pavel, the default image will be seen on the incoming call screen and the contact list screen and anywhere in the phone where the contact image would appear. See attachments for reference. 

The contact list icons are small, but the incoming call, for example, is the full-width size of the device.
Flags: needinfo?(ssmelko)
(Reporter)

Comment 6

4 years ago
Created attachment 8462606 [details]
Contacts Default Image

Pavel, do you mean that the overlay is too complicated? If so, I can provide a coloured image just for use in contacts. See attachment.

My only concern is that other apps also need a default image and I don't want them all to be turquoise. Can I provide a separate image for each acpp the default image is used in?
(Reporter)

Updated

4 years ago
Flags: needinfo?(pivanov)
He Sabrina,
This patch is only for contacts app ... Incomming screen and other screens in the spec are different apps :)
Are you Ok with contacts app if Yes we can go forward with the prototype if No we will gix all issues that we have :)

Thanks :)
(Reporter)

Comment 8

4 years ago
Oh okay, perfect. Thanks for clarifying. For prototype purposes, yes, we can go ahead. Thanks!
Hi,

We will need to see the impact of this on low memory devices. I'm afraid that doing this in a fugu/like device will consume a lot of resources.

We will need to be sure that we reuse the same blob for the letters that are the same, also, have we done an study of this will look on a contact list which most of the contacts don't have photo?

Comment 10

4 years ago
I agree with Francisco. 

apart from that, a default image was proposed long time ago and it was decided not to be used. I don't know why we are reopening this old issue.
I'm not sure ... maybe Sabrina can give use more light?
Flags: needinfo?(pivanov) → needinfo?(ssmelko)
Francisco: Why would it consume resources and why are you talking about blobs? Can't we write the letters in the DOM?
(In reply to Anthony Ricaud (:rik) from comment #12)
> Francisco: Why would it consume resources and why are you talking about
> blobs? Can't we write the letters in the DOM?

:) ... definitely, I apologize since my mind was just thinking on a image problem that we had.

We can do this as Anthony said writing new elements on the dom, which I don't like for the list performance.

With this patch applied I see (not my eye ;) but the fps indicator) a drop between 5-10 fps while scrolling the list fast.

Pavel will you add the letter to your patch?

Thanks a lot!
Hey Francisco and Anthony,

Sorry about this but ... I'm not sure that I understand ... which letters?
Flags: needinfo?(francisco)
Flags: needinfo?(anthony)
Hei Pavel!

In this attachment:

https://bugzilla.mozilla.org/attachment.cgi?id=8462602

I guess we are putting the group letter for the contact isnt?
Flags: needinfo?(francisco)
Oh ... sorry I focused over the image ... and totally miss the letter ... :D like I see them for the first time.

I will add them soon :) Thanks :)
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #13)
> We can do this as Anthony said writing new elements on the dom, which I
> don't like for the list performance.
Why don't you like it for list performance?

> With this patch applied I see (not my eye ;) but the fps indicator) a drop
> between 5-10 fps while scrolling the list fast.
Could it be because we images on every item? Or because of border-radius? Maybe a look at https://wiki.mozilla.org/FirefoxOS/Performance/App_Performance_Validation would give some clues.
Flags: needinfo?(anthony)
(In reply to Anthony Ricaud (:rik) from comment #17)
> > With this patch applied I see (not my eye ;) but the fps indicator) a drop
> > between 5-10 fps while scrolling the list fast.
> Could it be because we images on every item? Or because of border-radius?
> Maybe a look at
> https://wiki.mozilla.org/FirefoxOS/Performance/App_Performance_Validation
> would give some clues.

The border radius, I would like to have a patch to measure it and check. But to be honest, I really like how it looks like :)
Pavel, do you think we can have this for 2.1 (1st September)? I'll give you a hand with anything related to the list, performance, reviews and coding if needed ;)
Flags: needinfo?(pivanov)
Hey Francisco,

I wish that too :) thanks :)
Today I will continue with this one and I will add the letters and r? you to help me with the performance and if you have time we can try to hack other apps (I need help with the JS part)?
Flags: needinfo?(pivanov) → needinfo?(francisco)
Happy to help here Pavel!
Flags: needinfo?(francisco)
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

here we go :)
Attachment #8462498 - Flags: review?(francisco)
(Reporter)

Comment 23

4 years ago
Hey Pavel and Francisco! Thanks for your enthusiasm and help on this project. At the risk of sounding silly, it the document in GitHib something I can fork and upload to my Flame to see the default imagery "in action"? Or is it simply a matter of flashing my device and it will be included in the flash? 

If you could you walk me through how to get this patch on my device, it would be greatly appreciated!
Thanks again.
Flags: needinfo?(ssmelko)
Hi Sabrina,

You can apply the patch that Pivanov provided, or flash the branch bug-1043496 in Pivanov's repo.

For the first option try to do the following:

- Clone gaia's repo:
git clone https://github.com/mozilla-b2ggaia.git
cd gaia

- Download Pivanov patch and apply it
wget https://github.com/mozilla-b2g/gaia/pull/22160.patch
patch -p1 < 22160.patch

- Flash gaia into your phone (this will delete all data)
make reset-gaia

If you have trouble, are you in an office where an engineer can give you a hand?
setting feature-b2g=2.1 as this is the feature committed with partner.
Pavel, it will be great if you can tag the target milestone to either speint2 or sprint3.
Thanks.
blocking-b2g: --- → backlog
feature-b2g: --- → 2.1
Hey Sabrina,
are you OK with this one?
Flags: needinfo?(ssmelko)
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

Pretty good job, we are getting close, please see my comments on github.

Basicaly we should split the function for adding image in the one that already is there and a new one (the code you created) for adding default image in case that the contact doesn't have one.

Thanks for the job Pavel!
Attachment #8462498 - Flags: review?(francisco) → review-
Flags: in-moztrap?(jlorenzo)
Hei Pavel,

this is one of the features for 2.1 targeted by the end of this sprint, do you think you'll have time to finish it?
Flags: needinfo?(pivanov)
Target Milestone: --- → 2.1 S3 (29aug)
Hey Francisco,

yeah I think we are very close (did you see my email about the latest version of the PR in github?)
Flags: needinfo?(ssmelko)
Flags: needinfo?(pivanov)
Flags: needinfo?(francisco)
Hei Pavel!

Unfortunately didn't get any email, could you put here in bugzilla or resend?

Thanks!
Flags: needinfo?(francisco) → needinfo?(pivanov)
Hey Francisco,

I have success with your proposal (I hope so) but there is one small thing:

when we have a contact with photo we have the group letter over it ... maybe because of the cloneNode but I'm not sure.

I need help
Thanks in advance :)
Flags: needinfo?(pivanov) → needinfo?(francisco)
Hey Pavel,

could you rebase your patch, wanted to download it to check it but it's not mergeable in master.

Thanks!
Flags: needinfo?(francisco) → needinfo?(pivanov)
Pavel, I found another problem,when you scroll up and down, you can see how more images are being added.

Will try to provide a patch over your branch.
Done ;)
Flags: needinfo?(pivanov) → needinfo?(francisco)
Created attachment 8475974 [details] [diff] [review]
pavel_patch.diff

Pavel, I don't know why I cannot send you a PR to your branch so I'm attaching a patch to your last PR, you can add it to your code by:

git apply pavel_patch.diff

It solved the problem of the repeated letters, also I think I manage to improve the performance of the original patch

Could you add this patch to your PR?
Flags: needinfo?(francisco) → needinfo?(pivanov)
Hey Francisco :)

looks nice on my device :)) (patch is updated)
Flags: needinfo?(pivanov)
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

Hei folks, could you review this?

I'll review as well, but would like to know Cristian's opinion about performance and the use of images on the list.

Sergi your opinion is also important.

I just helped Pavel to make it work quickly, but I'm sure that we still can improve even more when we show the default image or when we  dont.

For FB images, as FB data is loaded asynchronously, we will see first default image and then we will change to the FB image.
Attachment #8462498 - Flags: review?(sergi.mansilla)
Attachment #8462498 - Flags: review?(crdlc)
Attachment #8462498 - Flags: review-
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #37)
> Comment on attachment 8462498 [details] [review]
> patch for Gaia/master
> 
> Hei folks, could you review this?
> 
> I'll review as well, but would like to know Cristian's opinion about
> performance and the use of images on the list.

I want to move this question to Vivien because it is more for an APZ's expert. Although I guess that we could modify a bit the image loader component for changing the display property ("none" - "block") when the rows are on the viewport or not like we do with images loading or releasing them right now. Of course we should do it in a follow-up.

> 
> Sergi your opinion is also important.
> 
> I just helped Pavel to make it work quickly, but I'm sure that we still can
> improve even more when we show the default image or when we  dont.
> 
> For FB images, as FB data is loaded asynchronously, we will see first
> default image and then we will change to the FB image.

I noticed this too, is it expected?
Flags: needinfo?(21)
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

I wrote some comments on github
Attachment #8462498 - Flags: review?(crdlc)
You have to take a look at contacts under "#" group when we have only the number. The photo shows "und" as title.
And also for favorites contacts under star group. The photo shows "favo" as title
And the last thing, please take a look to the search list, all photos by default show "unde" as title
Pavel, feel free to take this patch [1] that implements:

1. Show images by default when they are on screen
2. Hide images by default when they aren't on screen
3. The image by default is hidden for FB contacts while they are loading and also the same happens when contacts have a local image (this is less perceptible but happens).

[1] https://github.com/crdlc/gaia/commit/78c298c3a97c2e2ad9c1ae9cb83cce36290f7ddd

Fran, could you check this patch in order to know if you're comfortable with this behavior?

Many thanks
Flags: needinfo?(pivanov)
Flags: needinfo?(francisco)
https://moztrap.mozilla.org/manage/cases/?filter-tag=2992
Flags: in-moztrap?(jlorenzo) → in-moztrap+
(In reply to Cristian Rodriguez (:crdlc) from comment #43)
> Pavel, feel free to take this patch [1] that implements:
> 
> 1. Show images by default when they are on screen
> 2. Hide images by default when they aren't on screen
> 3. The image by default is hidden for FB contacts while they are loading and
> also the same happens when contacts have a local image (this is less
> perceptible but happens).
> 
> [1]
> https://github.com/crdlc/gaia/commit/78c298c3a97c2e2ad9c1ae9cb83cce36290f7ddd
> 
> Fran, could you check this patch in order to know if you're comfortable with
> this behavior?
> 
> Many thanks

Hei Cristian, thanks for the patch, it definitely helps with the preload of the default image. An impressive improve.

Pavel could you add Cristian's patch to your PR?

I'll give you a hand with the favourites and the 'und' groups
Flags: needinfo?(francisco)
Hey Francisco,

I update my patch based on Cristian patch and I also fix some comments on github. I will ping you back when I'm on IRC to see what's next :) Thanks guys :)
Flags: needinfo?(pivanov)
Created attachment 8477325 [details] [diff] [review]
patch_groups.diff

Hei Pavel,

please find attached another patch that will solve the problems of the groups that Cristian commented, for the contacts under the group '#' now we will displaying '#' in the image, for the favorites one we will displaying the original group.

With this I think the only thing we are missing is unit tests.

Do you agree with the result?
Flags: needinfo?(pivanov)
One more thing, Cristian left a couple of tiny comments on the original PR that are pretty easy to add, we should introduced them as well here :)
Created attachment 8478990 [details] [diff] [review]
config_activation.diff

Hei Pavel,

here is the patch for making this feature available via build time option.

By default we setup to true, so we enable the default images. But we give the chance to other builds to decide if the want to enable it or not.
I just test it :) and I think it looks nice :) PR updated
Who can I ask for r?
Flags: needinfo?(pivanov) → needinfo?(francisco)
Let me send a last patch with unit tests and I think we can ask Cristian for r?
Flags: needinfo?(francisco)
Created attachment 8479034 [details] [diff] [review]
unit_test.diff

Hei Pavel, last JS part, unit tests to check when we render the default photo and that the favorite and # groups have the correct data.

With this, I think we can ask Cristian for review.
Hey Francisco,

thanks :) I fix some lint errors and everything looks OK :)
Attachment #8462498 - Flags: review?(crdlc)
(Reporter)

Comment 54

4 years ago
Would anyone be kind enough to send me a screen shot? I've been having trouble flashing my phone to see the latest build with this implemented. Or is it not in the latest build? I'd like to see how it's looking. Thanks!
Created attachment 8479305 [details]
2014-08-26-21-02-58.png

Hi Sabrina, this is a screenshot, showing contact with default image, also in the favorites group and the #
Attachment #8479305 - Flags: ui-review?(ssmelko)
Attachment #8462498 - Flags: ui-review?(ssmelko)
Created attachment 8479307 [details] [diff] [review]
enable_by_default.diff

Hi Pavel,

In my last patch I disabled by default this feature, i'm sending the patch to enable it by default.

Sorry for the inconvenience
Flags: needinfo?(pivanov)
Sure :) Updated!
Flags: needinfo?(pivanov)
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

Hi, the code looks good although before the positive review I would like you to answer some questions and fix some nits in github. Thanks a lot for your work
Attachment #8462498 - Flags: review?(crdlc)
Obviously, after answering/addressing comments, please request again to me the last review. Thanks Pavel
Created attachment 8479811 [details] [diff] [review]
patch_favorites.diff

Pavel could you update the patch?
Flags: needinfo?(pivanov)
Attachment #8462498 - Flags: review?(sergi.mansilla) → review?(crdlc)
Flags: needinfo?(pivanov)
it's updated now
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

The search view shows contacts with photo by default like "unde" and the feature to make contacts as favorites is broken for contacts with photo by default. At least I can reproduce it.
Attachment #8462498 - Flags: review?(crdlc)
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

I think it's OK now :)
Attachment #8462498 - Flags: review?(crdlc)
The search should be fixed, I cannot reproduce the error on making a contact with photo a favorite.
with photo by default?

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #64)
> The search should be fixed, I cannot reproduce the error on making a contact
> with photo a favorite.
I still see the problem with favorites

STR:

1) Add as favorite a contact already in my list
2) It works fine
3) Create a new contact (name: 'manolo')
4) Add as favorite 'manolo'
5) It fails, 'manolo' hides the contact by default and it does not appear under 'Favorite' group
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

Please ask me again when it is fixed (see comment 66). Thanks a lot
Attachment #8462498 - Flags: review?(crdlc)
Created attachment 8479916 [details] [diff] [review]
fix_tests.diff

Last patch to fix the unit tests and get ride of the |setDefaultImage|

Hopefuly is the last one ;D
Flags: needinfo?(pivanov)
When it is merged I gonna review and test, thanks a lot mates
Status: NEW → ASSIGNED
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

Updated :)
Attachment #8462498 - Flags: review?(crdlc)
Flags: needinfo?(pivanov)
Comment on attachment 8462498 [details] [review]
patch for Gaia/master

It works like a charm, good job, thanks for this patch. Before merging please take a look at this comment https://github.com/mozilla-b2g/gaia/pull/22160/files#r16759797

Thanks guys
Attachment #8462498 - Flags: review?(crdlc) → review+
@ssmelko could we have that UX review?
(Reporter)

Comment 73

4 years ago
I can't actually get the patch to load on my device. Pavel tried to help me, but he's not sure the issue either. Please ask Patryk's opinion for the final say. In the meantime, I'll try to trouble-shoot.
Flags: needinfo?(padamczyk)
Pavel it functions wells. However there are 3 things I would polish, perhaps best make a separate bug for them if needed.

1. https://bug1043496.bugzilla.mozilla.org/attachment.cgi?id=8461679 not sure why the BW background image is fuzzy or really soft. Why can’t we use a crisp image?

2. https://bug1043496.bugzilla.mozilla.org/attachment.cgi?id=8479305, it would be good to neutralize the tones more. Some of letters appear too bright (3rd G and #) and some appear too dark (1st G, 2nd G, 5th G). Basically only the 4th G looks to have good brightness / contrast. This should be addressed in the large BW image.

3. Do we need the grey outline stroke around the pictures? I’d remove it. This could be addressed in v.2.2 if its too late for v.2.1
Flags: needinfo?(padamczyk) → needinfo?(pivanov)
Hey Patryk,

for #1 and #2 maybe Carol will help us. #3 is already done :)

Thanks :)
Flags: needinfo?(pivanov) → needinfo?(chuang)
Hi folks,

can we land this, as it's a 2.1 feature and do some follow ups in 2.1 or leave it for 2.2?

Cheers!
Hey Francisco,
I'm waiting for final images for this one (with better quality) and I will land it after that (hope till few hours) are you OK?
Flags: needinfo?(francisco)
That sounds like a good plan to me \o/!
Flags: needinfo?(francisco)
Hi Pavel,
As our offline discussion, i believe Sabrina will take care of this. thanks!
Flags: needinfo?(chuang) → needinfo?(ssmelko)
(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #76)
> Hi folks,
> 
> can we land this, as it's a 2.1 feature and do some follow ups in 2.1 or
> leave it for 2.2?
> 
> Cheers!

UX is review is way too late and slow. Go with what we have and create a follow-up.
Thanks all :)

Landed to master:
https://github.com/mozilla-b2g/gaia/commit/001d1761e4685c2a08d3e96bb362f031d47f4033
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Relanded:

https://github.com/mozilla-b2g/gaia/commit/561f053a8eb8da05d6c8432f35e01db36b55d1fd
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Reporter)

Updated

4 years ago
Flags: needinfo?(ssmelko)

Updated

4 years ago
Depends on: 1070900
Depends on: 1114559
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.