352250 - Simplify & optimize bookmarks shortcut (keyword) logic

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Bookmarks, defect)

Description

[Håkan Waara]

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.

Comment 1

[Håkan Waara]

Attachment: 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.)

Comment 2

[Stuart Morgan]

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).

Comment 3

[Håkan Waara]

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

[Nick Kreeger]

Comment on attachment 237857 [details] [diff] [review]
Patch

I'll take a look as well.

Comment 5

[Stuart Morgan]

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.

Comment 6

[Håkan Waara]

(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.

Comment 7

[Håkan Waara]

Stuart, please see comment 6.

Comment 8

[Stuart Morgan]

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.

Comment 9

[Håkan Waara]

(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

[Stuart Morgan]

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.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.