982054 - [Camera] Setting change text Feed back or Notifications

Status: RESOLVED INVALID

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

Description

[singhashish1887]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140310030201

Steps to reproduce:

Go to Camera Application->click setting icon->change any setting


Actual results:

It don't show any notification of changes settings


Expected results:

It should show notification for changed settings

Comment 1

[singhashish1887]

Attachment: Pull Request(Camera-new-feature)

Comment 2

[singhashish1887]

Attachment: Pull Request(Camera-new-feature) With UT

Comment 3

[David Flanagan [:djf]]

Comment on attachment 8389123 [details] [review]
Pull Request(Camera-new-feature)

The code for the notification controller doesn't make much sense to me.  There appears to be an attempt to support multiple notifications at the same time, but that code is broken.

Settings notifications need to be localized and can't be done as they are here.

Also, this feature is too tightly coupled to battery state. Write a generic notification view class that can handle a single persistent notification and a single transient notification.  Then, when you implement battery status in a separate bug, define a battery status controller class to display the appropriate indicators and notifications.  Similarly, someone can implement a settings notification controller to display notifications when settings have changed.

And what's the deal with full screen notifications? Do you really need those?  Is that actually part of the UX design?

Comment 4

[singhashish1887]

sorry I Have changed the status by mistake.It will make changes and update the patch

Comment 5

[singhashish1887]

We have updated the code as suggested by David now we made generic notification which will listen Notification event and receive object with minimum one parameter {message: "message"}. 
message from settings are localized now.
 Full Screen is required for showing critical battery message.It is part of UX .That is why did not changed in Full screen notification.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8389123 [details] [review]
Pull Request(Camera-new-feature)

This is better, but there is still work to be done before it can land.  See the various comments on github.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 8389123 [details] [review]
Pull Request(Camera-new-feature)

Ashish,

This is getting much better. I still think that your notification view JS and CSS can be simplified and clarified, and there are a few other minor changes (some of them just nits) that I'd like you to fix as well.  All of my comments are on github.  (I put a couple directly on the new commits but then repeated them in the PR itself.)

Thank you for fixing the file permission bits on your changed files!

Comment 8

[David Flanagan [:djf]]

Comment on attachment 8389123 [details] [review]
Pull Request(Camera-new-feature)

There are minor problems in the notifications view: showMessage() and clearMessage() should be prefixed with _ to mark them as private.  Or clearMessage() needs to handle redisplaying the persistent notification. Otherwise other modules could call clearMessage and get the wrong behavior.

There are problems with the icons.css file.  Ideally this PR would not change that file at all.  But I think you have merge conflict issues in there now.

Note that js/main.js and style/main.css have their file modes changed to 755.  If you can get those back to 644 before landing, I'd appreciate it.

These last few changes are mostly nits and I would have given r+ at this point, except that your test is completely out of date and can't even run anymore, I think. That will need to be updated before we can land.  If it helps, consider merging this back in with the battery status work if it is easier to write tests that way.

Finally, it is time to get ux review on this.  Please attach screenshots showing notifications displayed in different phone orientations.  Again, if it is easier, you could merge with the battery status patch and get ux review for the notifications there, since those will show icons as well.

Comment 9

[David Flanagan [:djf]]

Another review comment: the notifications seem to appear on top of the settings menu.  If I set the self timer and then quickly open the menu again, I see the timer notification on top of the menu. This is more noticeable when the camera is in landscape orientation.

I think you'll need to use z-index to ensure that the settings menu is on top of the notifications. Or you'll need to hide notifications (like we do with the grid) when the menu is displayed.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8389123 [details] [review]
Pull Request(Camera-new-feature)

This patch no longer has a test, so it can't land as it is.

And you keep including this patch in your patch for bug 948263.

I've asked you repeatedly whether you want to just merge this patch into the patch for bug 948263 and you have not responded.  So I'll make the decision.

Let's stop work on this bug and handle the notifications are part of bug 948263
I'm going to clear the review flag here.  When 948263 lands we can close this bug too.

But lets do all the notification and battery work together in that other bug.

Comment 11

[Hema Koka [:hema]]

I am assuming that the work for this is part of bug 948263 which landed in master today. Please confirm and close if it is done.

Comment 12

[singhashish1887]

Hema , this is resolved with the patch for bug 948263 .