Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Pull-down bookmarks/history lists to trigger sync #2165

Closed
sv-ohorvath opened this issue Apr 30, 2019 · 8 comments
Closed

Pull-down bookmarks/history lists to trigger sync #2165

sv-ohorvath opened this issue Apr 30, 2019 · 8 comments
Assignees
Labels

Comments

@sv-ohorvath
Copy link
Contributor

sv-ohorvath commented Apr 30, 2019

Why/User Benefit/User Problem

It would be a simple, fast way to bring new synced items, without the need to navigate back & forth between account settings and library.

What/Requirements

Pull-down to refresh and sync bookmarks & history.
Or, add a button in Bookmarks & history menus for that.

Acceptance Criteria (how do I know when I’m done?)

┆Issue is synchronized with this Jira Task

@sv-ohorvath sv-ohorvath added the feature request 🌟 New functionality and improvements label Apr 30, 2019
@sv-ohorvath sv-ohorvath mentioned this issue Apr 30, 2019
18 tasks
@vesta0 vesta0 added the P2 Upcoming release label May 3, 2019
@vesta0 vesta0 added this to the Post-MVP Backlog milestone May 3, 2019
@vesta0 vesta0 removed P2 Upcoming release labels Jul 2, 2019
@vesta0 vesta0 modified the milestone: Feature Backlog Jul 26, 2019
@ayushi1998
Copy link

Hey, I am an outreachy applicant. Can I work on this? How to go about the same? New to open source

@person808 person808 self-assigned this Jun 5, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 8, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 9, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 10, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 11, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 11, 2020
@person808
Copy link
Contributor

person808 commented Jun 11, 2020

Gonna write down a couple of potential edge cases for bookmarks that I think I need UX feedback on.

Given this bookmark structure:

Mobile Bookmarks
|__A
   |__B

Edge cases:

  1. If I'm in folder B, delete folder B on another device, sync that device, then trigger sync while in folder B, we end up viewing a folder that no longer exists, causing a crash.
  2. If I'm in folder A, delete folder B on another device, sync that device, trigger a sync while in folder A, and navigate to folder B, then we are viewing a folder that no longer exists. I haven't tested yet, but this probably causes a crash.
  3. If we try to edit a folder or bookmark, or we try to create a new folder while syncing, we might be making changes to a part of the bookmarks tree that will be invalid after the sync is finished.

Some potential solutions for 1:

  1. Only allow the pull-to-refresh gesture when viewing folders that we know can't be deleted (ie Desktop Bookmarks, Booksmarks Toolbar, etc.) This is easy to implement but is a bit inconsistent.
  2. If we sync and end up viewing an invalid folder, return to the top level bookmarks folder (Mobile Bookmarks). Would take more time to implement than 1, but I think still very doable. Probably would need to show some kind of message indicating that the folder was removed?
  3. Always go back to the top level bookmarks folder after syncing. Similar difficulty to implement as 2. Consistent behavior, but what if a user just wants to see a new bookmark from another device in a folder? Seems like having to navigate back to the folder would be annoying.

Some potential solutions for 2:

  1. Cancel sync when opening a folder.
  2. Implement one of solutions 2 or 3 from above.

Some potential solutions for 3:

  1. Disable any actions that would edit the bookmarks tree (folder creation, folder editing, etc.) while syncing and show a message explaining why they are disabled. I haven't implemented this yet.

Currently I have solution 1 implemented for the first two problems as those are the easiest to implement but I believe the other solutions wouldn't be too much trouble. The only problem with the other solutions is the changes needed break both the back and up buttons so they would immediately go back to the previous screen instead of navigating up the bookmark tree. It looks like the back button is easy to fix, but I'm not sure about the up button.

@person808 person808 added the needs:UX-feedback Needs UX Feedback label Jun 11, 2020
@person808
Copy link
Contributor

person808 commented Jun 11, 2020

Now that I think about it, canceling sync when opening a folder could be tricky to get right because syncing and updating the bookmarks displayed are separate operations so we need to make sure we can't trigger sync, have the sync finish, and navigate before updating the bookmarks view.

@topotropic
Copy link

@apbitner can you have a look? Thanks!

@apbitner
Copy link

@person808 I would recommend solution #2. If the user tries to sync a folder that has been deleted elsewhere, we should return them to the parent (not necessarily top level) folder, where the deleted child folder should no longer be visible.

If I'm in folder A, delete folder B on another device, sync that device, trigger a sync while in folder A, and navigate to folder B, then we are viewing a folder that no longer exists.

For this edge case, after a sync has been triggered I would expect folder B would no longer be visible, so I'm not sure how the user could view it?

If we try to edit a folder or bookmark, or we try to create a new folder while syncing, we might be making changes to a part of the bookmarks tree that will be invalid after the sync is finished.

I would assume the window in which a user could make changes while sync is occurring is so small that I would be okay if the user lost their changes, since they will be minor anyways.

person808 added a commit to person808/fenix that referenced this issue Jun 19, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 22, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 22, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 22, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 23, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 23, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 23, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 25, 2020
person808 added a commit to person808/fenix that referenced this issue Jun 25, 2020
@person808
Copy link
Contributor

Both gestures are merged! Just need qa to verify

@person808 person808 added the eng:qa:needed QA Needed label Jun 25, 2020
@liuche liuche mentioned this issue Jun 27, 2020
12 tasks
@sflorean
Copy link
Contributor

Verified as fixed on Nightly 6/30 and Beta 78.0.1-beta.2 with Samsung Galaxy Note 10 (Android 10) and LG G7 FIT (Android 8).

@sflorean sflorean added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Jun 30, 2020
@data-sync-user data-sync-user changed the title Pull-down bookmarks/history lists to trigger sync FNX2-17723 ⁃ Pull-down bookmarks/history lists to trigger sync Aug 5, 2020
@data-sync-user data-sync-user changed the title FNX2-17723 ⁃ Pull-down bookmarks/history lists to trigger sync FNX3-16111 ⁃ Pull-down bookmarks/history lists to trigger sync Aug 11, 2020
@data-sync-user data-sync-user changed the title FNX3-16111 ⁃ Pull-down bookmarks/history lists to trigger sync FNX-82 ⁃ Pull-down bookmarks/history lists to trigger sync Aug 11, 2020
@data-sync-user data-sync-user changed the title FNX-82 ⁃ Pull-down bookmarks/history lists to trigger sync Pull-down bookmarks/history lists to trigger sync May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

9 participants