Simplify & optimize bookmarks shortcut (keyword) logic

NEW
Unassigned

Status

Camino Graveyard
Bookmarks
12 years ago
7 years ago

People

(Reporter: Håkan Waara, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
PowerPC
Mac OS X

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
When working on bug 188178, I had to get all the bookmark keywords at some point. This wasn't easy, so I refactored the keywords code a bit.

It's really simple and shiny now.

BEFORE:

Every time someone used something looking like a keyword, we'd recursively go through *all* bookmarks to look for that keyword, and then parse it. As a bonus, all logic was in BookmarkFolder class (!)

NOW:

When we read in bookmarks (at startup), we populate a globally manage a map of keywords -> bookmarks.

We can quickly look up if something is a keyword, and it will also make it easier for anyone who wants to create UI for adding keywords to get a list of the current keywords (as well as adding new ones).

Patch upcoming.
(Reporter)

Comment 1

12 years ago
Created attachment 237857 [details] [diff] [review]
Patch

Stuart, you interested in reviewing this? Feel free to punt.

See comment 0.

(Note if you want to build: one tiny change is omitted; changing "resolveBookmarksKeyword" -> "resolveKeywordQuery" in BWC.mm. I had to many changes there already to diff it.)
Attachment #237857 - Flags: review?(stuart.morgan)

Comment 2

12 years ago
Yep, I'll review.  I wanted to do this myself when I first saw this code (I couldn't though, so I just made josh fix it so it didn't re-parse the string at every recursion (!)), so I'm all for the idea (leaving aside your apparent obsession with Britney Spears).
(Reporter)

Comment 3

12 years ago
I think the BookmarkManager will need to sign up for some kind of bookmark-removed/bookmark-changed notifications too, to update the list.

Comment 4

12 years ago
Comment on attachment 237857 [details] [diff] [review]
Patch

I'll take a look as well.
Attachment #237857 - Flags: review?(nick.kreeger)

Comment 5

12 years ago
Comment on attachment 237857 [details] [diff] [review]
Patch

> - (void) setKeyword:(NSString *)aKeyword
> {
>   if (!aKeyword)
>     return;
>+  
>+  [[BookmarkManager sharedBookmarkManager] connectBookmark:self toKeyword:aKeyword];

Bookmark items shouldn't have to know about this lookup table.  Instead, the manager should listen for changes to bookmark keywords (through the existing notification setup), and react according. (You will also need to listen for deletion, as you mentioned.)

>+- (void)connectBookmark:(BookmarkItem*)bookmark toKeyword:(NSString*)keyword;

This sounds like you can have more than one bookmark per keyword; call it setBookmark:forKeyword:

>+// flat list of all keywords as strings. may return nil if there are no keywords.
>+- (NSArray*)allBookmarkKeywords;

Don't introduce methods that aren't needed yet.

>+// converts a keyword query ("google britney spears") into one or more usable URLs.
>+-(NSArray *)resolveKeywordQuery:(NSString *)query;

Your introduction of 'query' in several places, along with the associated comments, makes it sound like it's only for bookmark search shortcuts, not for keywords on static bookmarks. Lets try to keep the language more neutral.

>+// turns a "formatted" URI (with '%s' in it) and an argument in it, to a usable URI.
>++ (NSString*)expandFormattedURL:(NSString*)url withString:(NSString*)searchString

These aren't formatted URLs (if anything they are URL formatters).  expandURL:... is fine.

>+- (void)connectBookmark:(BookmarkItem*)bookmark toKeyword:(NSString*)keyword
>+{
>+  if (!mBookmarkKeywords)
>+    mBookmarkKeywords = [[NSMutableDictionary alloc] init]

I think creating one empty dictionary when the BookmarkManager is instantiated is going to be less expensive that doing this check on every keyword change.
Attachment #237857 - Flags: review?(stuart.morgan) → review-
(Reporter)

Comment 6

12 years ago
(In reply to comment #5)

I can do all the changes, except for this one:

> (From update of attachment 237857 [details] [diff] [review] [edit])
> > - (void) setKeyword:(NSString *)aKeyword
> > {
> >   if (!aKeyword)
> >     return;
> >+  
> >+  [[BookmarkManager sharedBookmarkManager] connectBookmark:self toKeyword:aKeyword];
> 
> Bookmark items shouldn't have to know about this lookup table.  Instead, the
> manager should listen for changes to bookmark keywords (through the existing
> notification setup), and react according. (You will also need to listen for
> deletion, as you mentioned.)

The problem is that those notifications are suppressed when we first load the bookmarks.
(Reporter)

Comment 7

12 years ago
Stuart, please see comment 6.

Comment 8

12 years ago
Everyone who is interested in bookmarks instead waits for the bookmarks to finish loading, then sets up any initial state that matters to them.  An initial keyword lookup table should be built by crawling the bookmark tree once when it loads.
(Reporter)

Comment 9

12 years ago
(In reply to comment #8)
> Everyone who is interested in bookmarks instead waits for the bookmarks to
> finish loading, then sets up any initial state that matters to them.  An
> initial keyword lookup table should be built by crawling the bookmark tree once
> when it loads.

Hmm, sounds a bit wasteful to crawl the bookmarks twice to do that, when you can do both in one fell swoop? I don't really see what's better about that approach, except for the aestethic difference.

Comment 10

12 years ago
Having a well-structured object hierarchy isn't aesthetics, it's good design. That's why everything else uses notifications rather than, for example, calling into the bookmark menu and bookmark toolbar telling them to update whenever a bookmark's title changes.

Bookmark items should not know about how they are tracked.  Given everything that happens at startup, walking the in-memory bookmark tree once is going to have negligible cost (remember that this is currently being done on every single keyword use, and is still perfectly usable), which is a very small price to pay for having a better design.

Updated

12 years ago
Blocks: 357214
(Reporter)

Updated

12 years ago
Assignee: hwaara → nobody
Comment on attachment 237857 [details] [diff] [review]
Patch

Clearing this from kreeger's queue since it's already been minused and isn't the right approach.
Attachment #237857 - Flags: review?(nick.kreeger)
Summary: Simplify & optimize bookmarks keyword logic → Simplify & optimize bookmarks shortcut (keyword) logic
You need to log in before you can comment on or make changes to this bug.