Closed Bug 1176976 Opened 5 years ago Closed 4 years ago

[Messages][Drafts] Remove the draft saving/replacing action menu

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
FxOS-S8 (02Oct)

People

(Reporter: steveck, Assigned: azasypkin)

References

Details

(Whiteboard: [sms-sprint-FxOS-S8 p=1][sms-sprint-FxOS-S3][sms-sprint-FxOS-S4])

Attachments

(3 files)

After Bug 1144132 landed, the activity behavior will change and affect the daft behavior in message app. We might need to remove the action menu and simply save the draft directly to avoid the confusing.
Depends on: 144132
Depends on: 1144132
No longer depends on: 144132
Blocks: 1155534
Attached image sms draft v1.png
Adding spec for draft handling improvements provided by Jenny: short-term (that should be handled in this bug) and long/mid-term (once we have undo support, + it was modified a bit during discussion).
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Keywords: meta
Whiteboard: [sms-sprint-FxOS-S3]
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Hey Julien,

May I have your early feedback on this? I have left several questions at GitHub to get your opinion.

Thanks!
Attachment #8631656 - Flags: feedback?(felash)
Writing some tests at the moment to improve drafts integration tests coverage.
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

I think this looks good!
Attachment #8631656 - Flags: feedback?(felash) → feedback+
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Hey Julien,

Finally I was able to resolve issues with draft integration tests I had, fixed/add required unit tests and updated approach to show "draft saved" banner in Inbox view.

I've made some big changes and did a dozen of rebases since feedback was given, so I've squashed commits into one and added few more clarifying comments. So it should be ready for the first review round :)

Thanks!
Attachment #8631656 - Flags: review?(felash)
I started looking at the patch but won't finish today -- I started too late, sorry :(
Duplicate of this bug: 1188795
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

sorry, looks like this needs a full rebase :(
Attachment #8631656 - Flags: review?(felash)
Hey Morpheus,

In this bug we're trying to remove all draft related dialogs (save/replace/delete draft) and save/delete draft automatically whenever it makes sense, but there is one case I'm not sure about:

When user enters conversation with saved draft, selects all messages and removes them. Currently on master user is asked what to do with the draft - save or delete (though I see it's somewhat broken in master, oops :)).

So what do you think we should do here? I'd say we still have to automatically save draft in this case as user explicitly didn't ask to remove draft, but we need your expert UX advice on this :) 

Thanks!
Flags: needinfo?(mochen)
I agree with Oleg's suggestion. Since user chooses all messages to delete instead of deleting the entire thread, it means user may want to delete all messages but keep the thread. We need to automatically save the draft for any possibility user wants to edit later. 

However, I am wondering if we can provide the draft as an item on the bottom for user to select in "Delete message" view. So, when all selected, user explicitly wants to delete the thread. When all messages selected except draft, it means user wants to keep the draft. Does that make sense and is it feasible?  

If it's feasible, I can provide detailed spec for this feature. If not, we can just automatically save the draft like what Oleg suggests.

Let me know if any questions,
thanks.
Flags: needinfo?(azasypkin)
Whiteboard: [sms-sprint-FxOS-S3] → [sms-sprint-FxOS-S3][sms-sprint-FxOS-S4]
Hey Morpheus,

(In reply to mochen from comment #11)
> I agree with Oleg's suggestion. Since user chooses all messages to delete
> instead of deleting the entire thread, it means user may want to delete all
> messages but keep the thread. We need to automatically save the draft for
> any possibility user wants to edit later. 
> 
> However, I am wondering if we can provide the draft as an item on the bottom
> for user to select in "Delete message" view. So, when all selected, user
> explicitly wants to delete the thread. When all messages selected except
> draft, it means user wants to keep the draft. Does that make sense and is it
> feasible? 

> If it's feasible, I can provide detailed spec for this feature. If not, we
> can just automatically save the draft like what Oleg suggests.

Yeah, I think it's feasible, though it requires quite a lot of work right now as we don't support heterogeneous items in our selection code. So let's go with auto-saving in this bug and return to this new concept once we have free cycles. Who knows how we'll look like once NGA is done :)

Thanks!
 
> Let me know if any questions,
> thanks.
Flags: needinfo?(mochen)
Flags: needinfo?(azasypkin)
Depends on: 1197104
Blocks: 1080504
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Hey Julien,

It should be ready for the first review round.

One old issue will become more visible after this patch (see bug 1080504), namely first app instance Inbox will display draft state which it had when it was initially created by activity (second app instance), following updates won't be reflected in the first app instance until app is restarted. Previously we had this issue only if second instance is moved to background, but now we always create draft to back up activity and I've removed draft removal from ConversationView.handleDraft.

I was thinking to fix it separately in bug 1080504, but tell if you want me to fix the issue completely in this PR.

Thanks!
Attachment #8631656 - Flags: review?(felash)
Comment on attachment 8656211 [details] [review]
[gaia] julienw:1176976-remove-draft-dialogs > mozilla-b2g:master

Just a rebased version so that I can have a look at the try result.
Sorry, I started reviewing again but I couldn't finish. I added comments on the rebased pull request by mistake, sorry... The rebased version has a failing test as well.

The conflicts were in the unit tests only; take care that you don't remove some tests that were added in bug 1199593.

I'll finish the review when I come back next week !
(In reply to Julien Wajsberg [:julienw] (PTO Sept 2nd -> Sept 10th) from comment #16)
> Sorry, I started reviewing again but I couldn't finish. I added comments on
> the rebased pull request by mistake, sorry... 

I've replied on the rebased PR's comment :)

> The rebased version has a failing test as well.
> 
> The conflicts were in the unit tests only;

Just rebased original PR, fixed conflicts.

> take care that you don't remove some tests that were added in bug 1199593.

I believe I've removed some during rebasing, but I think there were related to "discard" draft/not-sent-message cases that we don't need anymore - please tell me if you think otherwise :)

Thanks!
Duplicate of this bug: 1153940
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

pffew,
I finished my review !

Please put all changes in a separate commit for an easy 2nd review pass :)
Attachment #8631656 - Flags: review?(felash)
Whiteboard: [sms-sprint-FxOS-S3][sms-sprint-FxOS-S4] → [sms-sprint-FxOS-S8 p=1][sms-sprint-FxOS-S3][sms-sprint-FxOS-S4]
See Also: → 1207578
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Should be ready for the second round of review! Hope I haven't missed anything :)

Thanks!
Attachment #8631656 - Flags: review?(felash)
Target Milestone: --- → FxOS-S8 (02Oct)
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

I left few comments in the additional commit: https://github.com/azasypkin/gaia/commit/b1b0dd4e8eaa410be860d2caf27bf300331a6642

it's r=me with the changes, but I'm fine if you want a 3rd review with a 3rd commit :)

Thanks for this huge work, I's really satisfying to see this landing.
Attachment #8631656 - Flags: review?(felash) → review+
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

Added 3rd commit, could you please take a look?

Thanks!
Attachment #8631656 - Flags: review+ → review?(felash)
Comment on attachment 8631656 [details] [review]
[gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master

ship it !

(with a green try :) )
Attachment #8631656 - Flags: review?(felash) → review+
(In reply to Julien Wajsberg [:julienw] (away Oct 1st, 2nd) from comment #23)
> Comment on attachment 8631656 [details] [review]
> [gaia] azasypkin:bug-1176976-remove-draft-dialogs > mozilla-b2g:master
> 
> ship it !
> 
> (with a green try :) )

Thanks a lot for comprehensive review!

Treeherder is green, so landed.

Master: https://github.com/mozilla-b2g/gaia/commit/08785512e40df469a3f1bd6a77b723f230b389a4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1200599
Depends on: 1213199
You need to log in before you can comment on or make changes to this bug.