830239 - The first time you load the gallery, it tells you you've got no photos, causing unnecessary terror

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 0. Be using a fresh gaia profile (or a profile where you've never opened the gallery before)
 1. Take some photos
 2. Open the gallery

ACTUAL RESULTS: Gallery starts out saying something like "You have no photos" when you open it. Then, after a second of terror (OMG MY PRECIOUS PICS), that message disappears and my actual photos start popping up.

EXPECTED RESULTS: Gallery starts out with a less-frightening message, like "Loading your photos...".  Once it's done whatever filesystem-crawling it needs to do, it can update that to have a "no photos" message vs. rendering-the-photos-it-finds.

Comment 1

[Casey Yee]

This too annoys me.  We should certainly fix this.

Comment 2

[Steve Chung [:steveck]]

It would be nice if we can display the message after scan complete without any media. But I think the main reason why we didn't hold the message until scan complete is because the scan might take a long time even no media to display. One scenario is all images in SDcard are over the our size limitation. It might be strange to see the loading spinner for a long time but still no image discovered in the end.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Steve Chung from comment #2)
> It might be strange to see the loading spinner for a long time
> but still no image discovered in the end.

It's even stranger to see "You have no photos" followed by "[tons of photos]".  (And more common -- IIUC, that's exactly what will happen to everyone the first time they load the gallery app.)

I think something to the effect of "Looking for photos..." would get the right point across.

Comment 4

[Casey Yee]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> 
> It's even stranger to see "You have no photos" followed by "[tons of
> photos]".  (And more common -- IIUC, that's exactly what will happen to
> everyone the first time they load the gallery app.)
> 
> I think something to the effect of "Looking for photos..." would get the
> right point across.

+1 Agreed.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch for this issue

When you start up your phone with an empty media database the phone tells you that there are no photo's yet. This is untrue. This adds another status message that tells the user that the SD card is being scanned when that is happening.

I need UX feedback on the message, and also a translation in other locales (don't know who to target for this).

Comment 6

[David Flanagan [:djf]]

Comment on attachment 720857 [details] [diff] [review]
Patch for this issue

Nice patch, thanks! But let's do wait for approval on the new language before landing.

Comment 7

[David Flanagan [:djf]]

Axel,

For patches like this that introduce new strings, is there anything else we need to do other than set the late-l10n flag?

Comment 8

[David Flanagan [:djf]]

This is a very low-risk patch, and it has tracking-b2g18 set, so we should try to get the wording and l10n issues addressed soon and see if we can uplift it.

Comment 9

[Axel Hecht [:Pike]]

No more flags necessary, but I'm concerned about the copy. "we are scanning your memory card" doesn't sound like something we want to be actually doing.

Technically, 


  $('overlay-title').textContent = navigator.mozL10n.get(id + '2-title');
  $('overlay-text').textContent = navigator.mozL10n.get(id + '2-text');

looks wrong. I'd recommend having a redirect object that allows us to modify the strings in place if needed, but starting to add string with foo2-title is kinda backwards. No idea where we started from here, though.

Comment 10

[David Flanagan [:djf]]

Jan,

Axel raises good points...

(In reply to Axel Hecht [:Pike] from comment #9)
> No more flags necessary, but I'm concerned about the copy. "we are scanning
> your memory card" doesn't sound like something we want to be actually doing.

How about we use the title "Scanning" and the text "Please wait: looking for photos and videos".  If we don't get any feedback from Casey soon, I'd be willing to land this to master with those two strings.

> Technically, 
> 
> 
>   $('overlay-title').textContent = navigator.mozL10n.get(id + '2-title');
>   $('overlay-text').textContent = navigator.mozL10n.get(id + '2-text');
> 
> looks wrong. I'd recommend having a redirect object that allows us to modify
> the strings in place if needed, but starting to add string with foo2-title
> is kinda backwards. No idea where we started from here, though.

I don't remember how showOverlay() got the 2 hardcoded into it either. But Axel is right: we shouldn't be using the same string as both an identifier for the overlay we want to display and an identifier for the strings we want to display.  Would you fix showOverlay() so that the strings for the new overlay do not need to have 2 in their ids?  Define an object that maps overlay name to l10n ids or use a switch statement or something.

I'm also setting you as the bug owner, since you've written the patch.

Comment 11

[Casey Yee]

"Looking for photos and videos" Is sufficient for this.   

Sorry for the late reply!

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

I've updated the PR to reflect the latest changes. Just need l10n for other languages than other, and then is good to go. Do I need to do anything else?

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 720857 [details] [diff] [review]
Patch for this issue

Sorry, but I'll need to r- this.

Now you're changing strings you're not actually changing. You should only be adding the scanning-* strings, and not change the others.

Also, please don't touch the gallery.*.properties files outside of en-US. The localizations inside git are just POC files, and we're updating them every now and then from the real localizations. (Those live outside, beneath http://hg.mozilla.org/gaia-l10n/ )

Given the difference between this patch and the previous, it might be a good idea to get another actual code review.

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 720857 [details] [diff] [review]
Patch for this issue

Sorry, I misread the comments. I have updated the PR as suggested, the old l10n keys have stayed the same now.

Comment 15

[Axel Hecht [:Pike]]

Comment on attachment 720857 [details] [diff] [review]
Patch for this issue

r=me, thanks.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/8b1bf134477fb8680fa265d2b0d6071af57ef3b8

Comment 17

[James Lal [:lightsofapollo]]

Backed out here: https://github.com/mozilla-b2g/gaia/commit/70a50b610db2f684eb5a537d277bd0d697a551a3

Because of linter failure.

Please wait until the travis CI build is green you will see that the PR is "good to merge" in the future.

Your r+ carries over but please rebase / and fix your lint error and re-land.

Comment 18

[James Lal [:lightsofapollo]]

Re landed here with lint error fix: https://github.com/mozilla-b2g/gaia/commit/2f2d2fde380abc91ff4136e7202070e39fde691f

Comment 19

[John Hu [:johnhu][:johu][:醬糊小弟]]

It is nice to have this scanning message in v1-train. And bug 891797 also added a message with the structure of this patch.

Comment 20

[Axel Hecht [:Pike]]

I think it's way too late in the schedule to land "nice to have".

Comment 21

[bhavana bajaj [:bajaj]]

(In reply to John Hu [:johnhu] from comment #19)
> It is nice to have this scanning message in v1-train. And bug 891797 also
> added a message with the structure of this patch.

Also a nice to have its too late for 1.1, and we have fixed this in future release :)