1042319 - [gallery] Update gallery edit UI and Visuals

Status: RESOLVED FIXED

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

Comment 1

[Tiffanie Shakespeare [:tif]]

Should we close this Bug 1021067 and just keep track of the visual change to the crop handles in this one while we do the other edit mode updates?

Comment 3

[Punam Dahiya [:pdahiya]]

(In reply to Tiffanie Shakespeare from comment #1)
> Should we close this Bug 1021067 and just keep track of the visual change to
> the crop handles in this one while we do the other edit mode updates?

Sure, marked 1021067 as resolved duplicate of this bug

Comment 4

[Punam Dahiya [:pdahiya]]

Attachment: WIP Patch for Bug 1042319

Hi David
I am attaching WIP PR for editUI and visuals updates to get your feedback on this initial version. Thanks

Comment 5

[David Flanagan [:djf]]

Trying out this patch, the new UX feels so much nicer and more intuitive than the old one, and I'm really glad that Stephany suggested this.  A could of UX nits:

1) The image preview still seems to be the same size as it used to be, so there is extra blank space on the bottom. The image should be centered on the screen, I think.

2) If I go into an edit mode and then cancel or apply, when I come back to the main edit screen, the edit tool I just used is highlighted.  That does not seem right. We don't need a highlighted state for the exposer, crop and color buttons anymore.  And we for the auto-enhance we only need to show blue or with stars to indicate whether it is on or not.

3) Do we really want the button to say "Done" instead of "Save"?  Is that a change?

I'll look at the code next, but overall, I'm really liking the new UX.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8468448 [details] [review]
WIP Patch for Bug 1042319

I like where this patch is going. I've left comments on github with a number of suggestions.  There is still some old code that is not needed anymore and should be removed. And I'd like you to think about improving the existing code by having one single state object that holds all the details you need like the currently selected crop mode and currently selected effect.  Rather than resetting the UI if the user cancels from an edit mode, just re-initialize each edit mode when you enter it so that the UI is always in sync with your master state object.  (Hopefully this suggestion is clearer in the context of my github comments.)

Comment 7

[Punam Dahiya [:pdahiya]]

Attachment: Patch for Bug1042319

Hi David
Attaching patch updated with feedback in (comment #5, comment #6) and rebased with latest master. Setting feedback flag as patch has new editUI flow implemented but still missing tests and new visuals. 

Thanks
Punam

Comment 8

[Amy Lee [:amylee] UX]

Hi Punam, 

I've attached the VsD spec and icons to Meta Bug 1024258. Let me know if you need anything else!

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8482819 [details] [review]
Patch for Bug1042319

Punam,

I left lots of minor comments on github, but overall this looks good.

Comment 10

[Punam Dahiya [:pdahiya]]

Created bug 1064969 for edit flow visual updates as per VsD specs attached to bug 1024258

Comment 11

[Punam Dahiya [:pdahiya]]

Comment on attachment 8482819 [details] [review]
Patch for Bug1042319

Hi David
I have addressed comments on github and updated PR with latest master. Please review. Thanks!

Comment 12

[David Flanagan [:djf]]

Comment on attachment 8482819 [details] [review]
Patch for Bug1042319

Punam,

I've noted a few nits in the code, but if you address them and have tested this carefully, please go ahead and get it landed.  (Feel free to flag me for re-review if you'd like.)  The existing code is such a mess and I have to keep reminding myself that I can't ask you to fix it all in this one patch.

One query on github that I'd like you not to miss is the question about whether the enableSaveAndApply() function gets called when we use the ImageEditor to crop an image during the image pick activity. It is sort of ugly if we do, but it is okay with me as long as you have verified that it is benign and isn't reporting errors in the logcat or anything.

Comment 13

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #12)
> Comment on attachment 8482819 [details] [review]
> Patch for Bug1042319
> 
> Punam,
> 
> I've noted a few nits in the code, but if you address them and have tested
> this carefully, please go ahead and get it landed.  (Feel free to flag me
> for re-review if you'd like.)  The existing code is such a mess and I have
> to keep reminding myself that I can't ask you to fix it all in this one
> patch.
> 
> One query on github that I'd like you not to miss is the question about
> whether the enableSaveAndApply() function gets called when we use the
> ImageEditor to crop an image during the image pick activity. It is sort of
> ugly if we do, but it is okay with me as long as you have verified that it
> is benign and isn't reporting errors in the logcat or anything.

Thanks David for review. I have updated patch with your feedback and tested edit flow. Verified pick activity flow with the updated patch, enableSaveAndApply button does get called but its benign and not reporting errors in logcat.

Comment 14

[Punam Dahiya [:pdahiya]]

Comment on attachment 8482819 [details] [review]
Patch for Bug1042319

Hi Katie
Attached patch implements edit UI flow as per the specs provided by you. Please review. Thanks!

Comment 15

[Punam Dahiya [:pdahiya]]

Comment on attachment 8482819 [details] [review]
Patch for Bug1042319

Hi Zac
Attached PR updates edit flow which resulted in a failed edit python tests.
I have updated edit_photo.py with the fix and Gip is back to green now. 
Setting feedback flag for you if update in python test looks good. Thanks!

Comment 16

[katieC]

Comment on attachment 8482819 [details] [review]
Patch for Bug1042319

ui+ for 2 step Edit Mode updates

Comment 17

[Zac C (:zac)]

Comment on attachment 8482819 [details] [review]
Patch for Bug1042319

r- because we just need to split tap_apply and tap_save into separate functions. All the methods in there represent an action on a page.

Thanks :)

Comment 18

[Punam Dahiya [:pdahiya]]

(In reply to Zac C (:zac) from comment #17)
> Comment on attachment 8482819 [details] [review]
> Patch for Bug1042319
> 
> r- because we just need to split tap_apply and tap_save into separate
> functions. All the methods in there represent an action on a page.
> 
> Thanks :)

Makes sense, I have updated patch with your feedback. Setting feedback flag again. Thanks!

Comment 19

[Zac C (:zac)]

Comment on attachment 8482819 [details] [review]
Patch for Bug1042319

f+, thanks

Comment 20

[Punam Dahiya [:pdahiya]]

Thanks Zac. Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/aa4da463a628b41b065fde7412b0e4a8fec62bc7

Comment 21

[No-Jun Park [:njpark]]

Hi Punam, I just have a quick question about this line:

https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/gallery/regions/edit_photo.py#L19

I couldn't find the element with the ID edit-tool-apply-button in WebIDE when I opened the Gallery app with its debugger.  (I could find other ids though)  Is this something that's supposed to be hidden from the debugger? (or should I look at a different place?)  Is there a way to locate this id?

Thanks for your help!

Comment 22

[Punam Dahiya [:pdahiya]]

(In reply to No-Jun Park [:njpark] from comment #21)
> Hi Punam, I just have a quick question about this line:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/
> gaiatest/apps/gallery/regions/edit_photo.py#L19
> 
> I couldn't find the element with the ID edit-tool-apply-button in WebIDE
> when I opened the Gallery app with its debugger.  (I could find other ids
> though)  Is this something that's supposed to be hidden from the debugger?
> (or should I look at a different place?)  Is there a way to locate this id?
> 
> Thanks for your help!

Hi No-Jun
edit-tool-apply-button is hidden on main edit screen and shows when the user enters edit mode tool (crop, exposure, filters). These icons are at the bottom on the main edit screen. Thanks!

https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/index.html#L153