Closed Bug 1217710 Opened 4 years ago Closed 4 years ago

[TV Browser] Synced bookmark organization is inconsistent between bookmarks from desktop and mobile

Categories

(Firefox OS Graveyard :: Gaia::TV::Browser, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
FxOS-S10 (30Oct)

People

(Reporter: jcheng, Assigned: danhuang)

References

Details

(Whiteboard: [ft:conndevices][partner-cherry-pick])

Attachments

(2 files)

once bookmarks are synced, the way desktop bookmark and mobile bookmarks are organized is confusing. mobile seems to be contained inside the desktop bookmarks as a folder
Flags: needinfo?(tchen)
proposal from UX

once the user sign in, the structure should be like this
- desktop bookmarks (folder)
- mobile bookmarks (folder)
- local bookmark a (link)
- local bookmark b (link)
- local bookmark c (link)
...
Flags: needinfo?(yliao)
Flags: needinfo?(tchen)
Flags: needinfo?(selin)
Flags: needinfo?(selin) → needinfo?(selee)
The bookmark panel is based on the bookmark data from Firefox Sync to render, so my suggestion is that Browser has to change the way that how it's rendered.

Hey Dan,

Could you help to see the requirement?
Please notice that bug 1217463 mentioned 'places' record is not always there, and we need to handle the case as well.
Thank you!
Flags: needinfo?(selee) → needinfo?(dhuang)
This requirement can be implemented. But I think the folder name should be 'Desktop bookmarks' and 'Mobile bookmarks'. And the back button title should be 'Back to desktop bookmarks' and 'Back to mobile bookmarks'? Tori could you help to confirm this ? Thanks.
Flags: needinfo?(dhuang) → needinfo?(tchen)
See Also: → 1217463
Depends on: 1217463
(In reply to Dan Huang[:danhuang] from comment #3)
> This requirement can be implemented. But I think the folder name should be
> 'Desktop bookmarks' and 'Mobile bookmarks'. And the back button title should
> be 'Back to desktop bookmarks' and 'Back to mobile bookmarks'? Tori could
> you help to confirm this ? Thanks.

Agreed. Thanks for confirming.
Flags: needinfo?(tchen)
Blocks: TV_FxAccount
Whiteboard: [ft:conndevices]
Target Milestone: --- → FxOS-S10 (30Oct)
Assignee: nobody → dhuang
Hey Tori,
IMHO, 'Mobile bookmarks' should not be displayed when 'mobile' folder doesn't exist or has no children.
Do you agree with this?
Flags: needinfo?(tchen)
Sure, and vice versa for desktop bookmarks
Flags: needinfo?(tchen)
Duplicate of this bug: 1218687
Comment on attachment 8679860 [details] [review]
[gaia] danhuang1202:1217710_synced_bookmark_inconsistent > mozilla-b2g:master

Please help to review. This patch display synced mobile bookamrk folder item at the first layer with local bookmarks. Thanks.
Attachment #8679860 - Flags: review?(yliao)
Comment on attachment 8679860 [details] [review]
[gaia] danhuang1202:1217710_synced_bookmark_inconsistent > mozilla-b2g:master

Just a couple nits. Thanks for the effort!
Flags: needinfo?(yliao)
Attachment #8679860 - Flags: review?(yliao) → review-
Comment on attachment 8679860 [details] [review]
[gaia] danhuang1202:1217710_synced_bookmark_inconsistent > mozilla-b2g:master

Please help me to review. The second commit in this patch fix Yi-fan's comment. Thanks.
Attachment #8679860 - Flags: review- → review?(selee)
Comment on attachment 8679860 [details] [review]
[gaia] danhuang1202:1217710_synced_bookmark_inconsistent > mozilla-b2g:master

Hi Dan,

Thanks a lot for your effort!
There are comments for this patch, and I would like to refactor the current code which has some codes in too many braces.
I propose attachment 8680554 [details] [review] patch for you as an example to refactor this.

Thank you. :)
Attachment #8679860 - Flags: review?(selee) → review-
Comment on attachment 8680554 [details] [review]
[gaia] weilonge:seanlee/TVBrowser/master/Bug1217710 > mozilla-b2g:master

Hey Sean, thanks for your review and the refactor patch! I updated my code architecture reference by you patch. Please help to review again, thanks a lot!
Attachment #8680554 - Flags: review?(selee)
(In reply to Dan Huang[:danhuang] from comment #14)
> Comment on attachment 8680554 [details] [review]
> [gaia] weilonge:seanlee/TVBrowser/master/Bug1217710 > mozilla-b2g:master
> 
> Hey Sean, thanks for your review and the refactor patch! I updated my code
> architecture reference by you patch. Please help to review again, thanks a
> lot!

Sorry for the wrong path attachment, the correct one should be attachment 8679860 [details] [review].
Attachment #8680554 - Flags: review?(selee)
Hey Dan, I leave some comments at github. Please fix them and let's get one more round. :)
Comment on attachment 8679860 [details] [review]
[gaia] danhuang1202:1217710_synced_bookmark_inconsistent > mozilla-b2g:master

Sean~ Please help me to review again! This patch update _createRootFolderCache() to handle promises execution. Thanks.
Attachment #8679860 - Flags: review- → review?(selee)
Comment on attachment 8679860 [details] [review]
[gaia] danhuang1202:1217710_synced_bookmark_inconsistent > mozilla-b2g:master

Hey Dan,

Great! LGTM :)
Don't forget to modify the commit message after rebasing the patch.
Thank you!
Attachment #8679860 - Flags: review?(selee) → review+
Thanks for the review !!
Merge in master:
https://github.com/mozilla-b2g/gaia/commit/8443f983a94b83af2077a27a4852875546bb88b6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [ft:conndevices] → [ft:conndevices][partner-cherry-pick]
You need to log in before you can comment on or make changes to this bug.