Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bookmarks #525

Merged
merged 31 commits into from Feb 6, 2019

Conversation

Projects
None yet
4 participants
@mhammond
Copy link
Member

commented Jan 9, 2019

This is intended to be a "long lived" PR to track the work in the "bookmarks" branch. This branch is where we can collaborate on bookmarks until we get to a point where we consider it "done enough" to flatten and merge into master.

Closes #511. Closes #512. Closes #514. Closes #516.

mhammond added some commits Jan 7, 2019

Move storage.rs to a directory module.
This splits history specific stuff into a history submodule, but keeps
stuff that needs to be shared between history and bookmarks in the top-level
storage module.

@thomcc thomcc self-requested a review Jan 9, 2019

@linacambridge linacambridge added this to the rustmarks/schema milestone Jan 9, 2019

@thomcc thomcc removed their request for review Jan 9, 2019

@mhammond

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

This is starting to look like it is in reasonable shape - the main thing missing is "update" and external APIs. I'm inclined to think that the changes are getting significant enough that it would be better for everyone if we review and land as things stand, and complete further work in followups.

@linacambridge and @thomcc WDYT? I'll explicitly flag for review anyway as even if we choose to not merge this now, I think another review pass makes sense regardless.

@mhammond mhammond requested review from thomcc and linacambridge Jan 14, 2019

@linacambridge
Copy link
Member

left a comment

This is excellent, @mhammond, thank you for taking this on! 🎉 👏 👷 I have a lot of style nits and small suggestions, but the general shape makes sense, and it's more than enough to iterate and build on.

I wouldn't mind seeing this PR again, but I agree that it makes sense to land next week, and save the follow-up work for later PRs. I'm sure we'll be making changes as we add merging and syncing, anyway, so it's not worth fretting too much about the details at this point.

Let's do this! 📚

Show resolved Hide resolved components/places/examples/places_utils.rs Outdated
Show resolved Hide resolved components/places/examples/places_utils.rs Outdated
Show resolved Hide resolved components/places/examples/places_utils.rs Outdated
Show resolved Hide resolved components/places/examples/places_utils.rs Outdated
BookmarkTreeNode::Folder(folder_node) => folder_node,
_ => {
println!("Imported node isn't a folder structure");
return Ok(());

This comment has been minimized.

Copy link
@linacambridge

This comment has been minimized.

Copy link
@mhammond

mhammond Jan 23, 2019

Author Member

As above, I don't care too much about that - the tool prints an error and exits. I guess a non-zero exit code makes sense, but at this stage I'm not sure it's worth it.

Show resolved Hide resolved components/places/src/storage/bookmarks.rs Outdated
Show resolved Hide resolved components/places/src/storage/bookmarks.rs
Show resolved Hide resolved components/places/src/storage/bookmarks.rs
Show resolved Hide resolved components/places/src/storage/bookmarks.rs Outdated
Show resolved Hide resolved components/places/src/storage/bookmarks.rs Outdated
@mhammond

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

Thanks for the excellent feedback @linacambridge! I've resolved all conversations where I've either made the change or no change was suggested, and pushed a new version. The only urgency to land this is when it starts blocking you or @thomcc so feel free to go another round of reviews or 2 ;)

@thomcc

thomcc approved these changes Jan 23, 2019

Copy link
Contributor

left a comment

Great work! I have a few nits but noting major.

Show resolved Hide resolved components/places/examples/places_utils.rs Outdated
Show resolved Hide resolved components/places/examples/places_utils.rs Outdated
Show resolved Hide resolved components/places/examples/places_utils.rs Outdated
Show resolved Hide resolved components/places/src/storage/bookmarks.rs Outdated

mhammond and others added some commits Jan 31, 2019

I'm dismissing lina's review so that I can re-request it. Hopefully this works.

@thomcc thomcc requested a review from linacambridge Feb 1, 2019

thomcc and others added some commits Feb 1, 2019

Fix up review nits.
* Add `Into<InsertableItem>` and `Into<BookmarkTreeNode>` type
  conversions, for convenience.
* Factor out the item type fallback pattern into
  `from_u8_with_valid_url`.
Ensure `fetch_tree` returns siblings of parents with descendants.
Given the following tree:

    A
   / \
  B   C
 /|\   \
D E F   G

`process_folder_rows` tried to recurse down the result set and
walk rows in pre-order, (A, B, D, E, F, C, G). Unfortunately, the query
in `fetch_tree` returns rows in level-order, (A, B, C, D, E, F, G).
This means we can't recurse down the result set directly, because
we'll ignore cousin nodes.

Instead, we build a temporary "pseudo-tree", mapping parents to
children, then recursively walk the pseudo-tree.

@linacambridge linacambridge force-pushed the bookmarks branch from 00df689 to ec52f27 Feb 4, 2019

@@ -214,6 +214,21 @@ impl BookmarkType {
_ => None,
}
}

pub fn from_u8_with_valid_url<F: Fn() -> bool>(v: u8, has_valid_url: F) -> Self {

This comment has been minimized.

Copy link
@linacambridge

linacambridge Feb 4, 2019

Member

I think this needs a better name, but I'm not sure what. 😅

None => return Ok(None),
};

// For all remaining rows, build a pseudo-tree that maps parent GUIDs to

This comment has been minimized.

Copy link
@linacambridge

linacambridge Feb 4, 2019

Member

@thomcc, could you double-check 👀 this, please?

This comment has been minimized.

Copy link
@thomcc

thomcc Feb 4, 2019

Contributor

HashMap<Option<SyncGuid>, Vec<BookmarkTreeNode>> huh that's a bit weird. Guess it makes some sense though.

@thomcc
Copy link
Contributor

left a comment

Looks reasonable, We should try not to log titles or URLs in log::warn! though.

Show resolved Hide resolved components/places/src/storage/bookmarks.rs Outdated
None => return Ok(None),
};

// For all remaining rows, build a pseudo-tree that maps parent GUIDs to

This comment has been minimized.

Copy link
@thomcc

thomcc Feb 4, 2019

Contributor

HashMap<Option<SyncGuid>, Vec<BookmarkTreeNode>> huh that's a bit weird. Guess it makes some sense though.

Show resolved Hide resolved components/places/src/storage/bookmarks.rs Outdated

@linacambridge linacambridge modified the milestones: rustmarks/schema, Rustmarks Feb 4, 2019

@linacambridge linacambridge merged commit adbe9e7 into master Feb 6, 2019

3 of 6 checks passed

ci/circleci: Rust tests - beta Your tests failed on CircleCI
Details
ci/circleci: Rust tests - stable Your tests failed on CircleCI
Details
Taskcluster (pull_request) TaskGroup: Pending (for pull_request.synchronize)
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details
ci/circleci: Rust benchmarks Your tests passed on CircleCI!
Details
ci/circleci: Sync integration tests Your tests passed on CircleCI!
Details

@linacambridge linacambridge deleted the bookmarks branch Feb 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.