175748 - Need mechanism for creating new, blank bookmarks

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Bookmarks, enhancement)

Description

[Albert Kammerer]

There should to be some way to create a blank bookmark that you can type a name
and URL into and then save it in your bookmarks. To do this now you have to
create a bookmark of whatever page is open. You can then open the bookmark and
replace the information with what you want and save it. Cumbersome at best.

Comment 1

[Gilles Durys]

*** Bug 175749 has been marked as a duplicate of this bug. ***

Comment 2

[Gilles Durys]

*** Bug 175750 has been marked as a duplicate of this bug. ***

Comment 3

[Gilles Durys]

*** Bug 175752 has been marked as a duplicate of this bug. ***

Comment 4

[kaldari]

Why would anyone ever want to create a bookmark manually?

Comment 5

[kaldari]

*** Bug 181098 has been marked as a duplicate of this bug. ***

Comment 6

[Stephane Moureau]

Manually? To create bookmarklets.
To be able to manage, clean, copy/paste the bookmarks offline.
To add some of them in short format apple,hp,canon instead of http://www.apple.com.

Comment 7

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

It seems to me--particularly once the BM ends up in a tab, bug 215235--that the
Add Bookmark item in the "plus" widget at the bottom of the BM needs to do what
this bug asks (currently it brings up the Add Bookmark sheet with the underlying
page's info filled in).  Cmd-D/Add Page to Bookmarks menu item should either do
the same (or be disabled; one doesn't need to bookmark the Bookmarks Manager,
does one?).

Comment 8

[Samuel Sidler (old account; do not CC)]

I agree 100% with Smokey. This is exactly what the add bookmark option in the
menu should do. This doesn't seem too hard. I'm adding to the 1.0 list.

Comment 9

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

Also, in about:bookmarks, using the action menu to create a new bookmark overwrites about:bookmarks in the location bar with the location of the previous page.

The sheet is also annoying here because if I'm in another collection (searches, or the Bookmarks Bar) and invoke the action menu item, Camino still tries to put the bookmark in the Bookmarks Menu (bug 287708) unless I scroll the sheet to select the (other) collection that *I'm currently viewing*....  

See also bug 159230, which is tangentially related in the non-Manager case.

Comment 10

[Håkan Waara]

Attachment: Proposed fix

Here's a patch that will make the action menu's "Add bookmark" item (in about:bookmark) create a new bookmark and pop up the edit bookmark dialog for it.

Comment 11

[Håkan Waara]

Comment on attachment 237482 [details] [diff] [review]
Proposed fix

There's a nib change too, to make the context-menu item use the new IBAction.

Comment 12

[Nick Kreeger]

Comment on attachment 237482 [details] [diff] [review]
Proposed fix

Taking a look ...

Comment 13

[Nick Kreeger]

Comment on attachment 237482 [details] [diff] [review]
Proposed fix

+  BookmarkFolder *currentFolder = [self selectedContainerFolder];
+  Bookmark *newBookmark = [currentFolder addBookmark];

A little nit here, could you put the * on the left hand side?

+  BookmarkFolder *currentFolder = [self selectedContainerFolder];
+  Bookmark *newBookmark = [currentFolder addBookmark];

Since |selectedContainerFolder:| and |addBookmark:| may return nil, they should be nil checked.

if (currentFolder) {
	Bookmark* newBookmark = [currentFolder addBookmark];
	...
}

+  [bic setBookmark:newBookmark];

Your nil check for |newBookmark| should be here

if (newBookmark) {
	[bic setBookmark:newBookmark];
	...
}

Looks fine other than that.

Comment 14

[Mike Pinkerton (not reading bugmail)]

for the record, i think popping up the info sheet is a little weird, especially since it's non-modal. I'd expect to be able to edit the bookmark inline, sorta like when you create a new folder.

Comment 15

[Stuart Morgan]

(In reply to comment #14)
> for the record, i think popping up the info sheet is a little weird, especially
> since it's non-modal. I'd expect to be able to edit the bookmark inline, sorta
> like when you create a new folder.

I'm in that camp as well.  The only problem I see with inline editing is that tab isn't going from the title to the URL, but that should be fixable.

Comment 16

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

(In reply to comment #14)
> for the record, i think popping up the info sheet is a little weird, especially
> since it's non-modal. I'd expect to be able to edit the bookmark inline, sorta
> like when you create a new folder.

We pop up the sheet when creating a new folder (which is really weird imo).  I think inline would be the way to go on both cases, too, though.

Comment 17

[Håkan Waara]

Ok, I'll make both of these things inline.

Comment 18

[Stuart Morgan]

Comment on attachment 237482 [details] [diff] [review]
Proposed fix

Clearing review flag per comment 17

Comment 19

[Chris Lawson (gone)]

(In reply to comment #15)

> I'm in that camp as well.  The only problem I see with inline editing is that
> tab isn't going from the title to the URL, but that should be fixable.

I'm going to file a new bug for that, as it seems to be unrelated to fixing *this* bug, and I spent about two hours tonight digging around trying to find out why it doesn't work.

Patch for this bug coming soon, by the way.

Comment 20

[Chris Lawson (gone)]

Attachment: code fixes

Needs a nib, coming next.

Comment 21

[Chris Lawson (gone)]

Attachment: nib fix

Here's the nib with hookups for the new IBActions.

Comment 22

[Chris Lawson (gone)]

Targeting for 1.1 since we have a fix.

Comment 23

[Sean Murphy]

(In reply to comment #20)
> Created an attachment (id=247777) [edit]

Sorry Nick, I don't mean to jump in and steal the review; just looking over some patches for experience.

+- (IBAction)newBookmarkFolder:(id)aSender
+{
+  BookmarkFolder* currentFolder = [self selectedContainerFolder];
+  if (currentFolder) {
+    Bookmark* newFolder = [currentFolder addBookmarkFolder];

Should newFolder be declared as a BookmarkFolder*, since it's being initialized as such?  This results in a build error, so you might already have noticed it.

Also, right clicking on an empty row in the bookmark manager properly displays the new menu item without the ellipsis character and sheet.  The context menu for a BookmarkFolder or BookmarkItem, however, still uses a sheet instead of inline creation/editing.  I'm not sure if we want this behavior or not, so bringing it to attention just in case.

Comment 24

[froodian (Ian Leue)]

Comment on attachment 247777 [details] [diff] [review]
code fixes

If a patch doesn't build, I think it's safe to - it.

Comment 25

[Chris Lawson (gone)]

Attachment: fixed

Murph, you mind taking a look at this?

I do have one question about the current patch, too.

+  int index = [mBookmarksOutlineView selectedRow];
+  BookmarkFolder* currentFolder = [self selectedContainerFolder];
+  if (currentFolder) {
+    // add blank bookmark after current selection
+    Bookmark* newBookmark = [currentFolder addBookmark:@"" url:@"" inPosition:(index + 1) isSeparator:NO];
+    if (newBookmark) {
+      [self revealItem:newBookmark scrollIntoView:YES selecting:YES byExtendingSelection:NO];
+      int newBookmarkIndex = [mBookmarksOutlineView rowForItem:newBookmark];
+      [mBookmarksOutlineView editColumn:0 row:newBookmarkIndex withEvent:nil select:YES];

Would it be legit to replace the last two lines with

+      [mBookmarksOutlineView editColumn:0 row:(index + 1) withEvent:nil select:YES];

and avoid re-determining the index of the new bookmark (or folder), since we know it already?

cl

Comment 26

[Chris Lawson (gone)]

Oh, BTW, I'm going to wait on posting the nib fixes until I get at least a "yeah, this looks right" type of review, although I'm betting the previous nib fixes will still work (unless that nib has bitrotted since 07 December).

cl

Comment 27

[Håkan Waara]

Are these two new methods ensured to never be called when there is no selection? (I.e., are the menuitems enabled/disabled appropriately.)

I noted that selectedContainerFolder doesn't deal with the selected row index being -1 (no row selected), and I think calling objectAtIndex:-1 will throw an exception.

Comment 28

[Håkan Waara]

Comment on attachment 251503 [details] [diff] [review]
fixed

Two other comments:

>+- (IBAction)newBookmark:(id)aSender
>+{
>+  int index = [mBookmarksOutlineView selectedRow];
>+  BookmarkFolder* currentFolder = [self selectedContainerFolder];
>+  if (currentFolder) {
>+    // add blank bookmark after current selection
>+    Bookmark* newBookmark = [currentFolder addBookmark:@"" url:@"" inPosition:(index + 1) isSeparator:NO];
>+    if (newBookmark) {
>+      [self revealItem:newBookmark scrollIntoView:YES selecting:YES byExtendingSelection:NO];

1) According to the documentation for editColumn:row:withEvent:select:, it already reveals the item.

>+      int newBookmarkIndex = [mBookmarksOutlineView rowForItem:newBookmark];
>+      [mBookmarksOutlineView editColumn:0 row:newBookmarkIndex withEvent:nil select:YES];

2) I don't like the hardcoding of the column here. Will it work even if the name column is not the first one? Use columnWithIdentifier: to get the column index instead.

Comment 29

[Sean Murphy]

Comment on attachment 251503 [details] [diff] [review]
fixed

(In reply to comment #27)

> I noted that selectedContainerFolder doesn't deal with the selected row index
> being -1 (no row selected), and I think calling objectAtIndex:-1 will throw an
> exception.

selectedContainerFolder returns the selection from mContainersTableView, which is the source-list type "Collections" TableView in the bookmark editor.  We have the collections table configured in Interface Builder to disallow an empty selection, so [mContainersTableView selectedRow] should never return -1 and the subsequent objectAtIndex: will supply a valid unsigned number.

(For the latest patch):

Functionally, it works great and I like this behavior for the "Add Bookmark" action menu item much better than adding a bookmark for the bookmark manager itself.  There are a few code changes which might be worth considering, though.

+  int index = [mBookmarksOutlineView selectedRow];
+  BookmarkFolder* currentFolder = [self selectedContainerFolder];
+  if (currentFolder) {
+    // add blank bookmark after current selection
+    Bookmark* newBookmark = [currentFolder addBookmark:@"" url:@"" inPosition:(index + 1) isSeparator:NO];

The declaration and initialization of index could be moved down closer to where it's actually used, since this is probably what caused Håkan (and myself at first) to wonder about calling selectedContainerFolder with nothing currently selected in the OutlineView.  Additionally, renaming index to something less generic (such as selectedBookmarkRow) makes the inPosition:(selectedBookmarkRow + 1) parameter even more apprehensible.

+      [self revealItem:newBookmark scrollIntoView:YES selecting:YES byExtendingSelection:NO];

As Håkan mentioned, we don't need to explicitly scroll the item into view since editColumn:row:withEvent:select: will do that.  Therefore, the only functionality we're really using revealItem:… for is its selecting capability.  I'd go ahead and remove the method and replace it with one that just selects the new item.

As for actually selecting the new bookmark, revealItem:scrollIntoView:selecting:byExtendingSelection: sets the outline view's selected row by calling selectRow:byExtendingSelection:.  This NSTableView method was depreciated in Mac OS X 10.3 and should be modified to use the newer NSIndexSet technique instead (selectRowIndexes:byExtendingSelection:). That doesn't directly concern this patch anymore, however, but just make sure you set the selection using an index set.  I'll look into replacing the older selection methods, since Apple's docs explain: "A method identified as deprecated has been superseded and may become unsupported in the future."

> Would it be legit to replace the last two lines with
> 
> +      [mBookmarksOutlineView editColumn:0 row:(index + 1) withEvent:nil
> select:YES];
> 
> and avoid re-determining the index of the new bookmark (or folder), since we
> know it already?

I'd just ask the bookmarkOutlineView again for the item's index.  A degradation in performance doing this is negligible and therefore a non-issue, and more importantly, retrieving the index again makes sure the code isn't fragile and will work with any future changes to the lines above it.

+    // add blank bookmark after current selection

It could be helpful to indicate in this comment that if there's no current selection the bookmark will be inserted at the top/beginning.

Finally, a style consideration worth considering is to remove the nesting caused by error checking and make it obvious what the normal flow (or most likely path) of these methods are.  Unless I'm writing a particularly long method and I need to constantly look for errors I usually go against the SESE philosophy and throw in explicit returns right after a problem is detected.  The rest of BookmarkViewController.mm seems to follow that same convention.  This is just a style opinion though, and whether it's actually better is fairly subjective :)

This is newBookmark: with an alternative error checking style and setting the selection using an index set:

BookmarkFolder* currentFolder = [self selectedContainerFolder];
if (!currentFolder)
	return;

// add blank bookmark after current selection
int selectedBookmarkRow = [mBookmarksOutlineView selectedRow];
Bookmark* newBookmark = [currentFolder addBookmark:@"" url:@"" inPosition:(selectedBookmarkRow + 1) isSeparator:NO];
if (!newBookmark)
	return;

int newBookmarkRow = [mBookmarksOutlineView rowForItem:newBookmark];
if (newBookmarkRow == -1) 
  return;

[mBookmarksOutlineView selectRowIndexes:[NSIndexSet indexSetWithIndex:newBookmarkRow] byExtendingSelection:NO];
[mBookmarksOutlineView editColumn:0 row:newBookmarkRow withEvent:nil select:YES];

Review- for now; Just addressing the functional issues from Håkan and myself would be enough for a r+.  The style suggestions are somewhat optional, but I would recommend considering them.

Comment 30

[Chris Lawson (gone)]

Attachment: applies murph and hwaara's suggestions

Thanks, guys. Here's an updated patch that addresses the issues.

cl

Comment 31

[Håkan Waara]

Comment on attachment 251556 [details] [diff] [review]
applies murph and hwaara's suggestions

Looks great to me!

Comment 32

[Chris Lawson (gone)]

Comment on attachment 251556 [details] [diff] [review]
applies murph and hwaara's suggestions

sr? smorgan per pink on AIM.

Murph, feel free to review or not at your leisure, since hwaara already did.

Comment 33

[Sean Murphy]

Comment on attachment 251556 [details] [diff] [review]
applies murph and hwaara's suggestions

Looks great to me as well!

I did want to mention though the only other thing that came to my mind when looking this over is how the two IBAction methods each share the same code for about 10 of their 11 lines.  I don't think that's really a problem though - most of the lines are only error checking and splitting the common code into another method would probably just unnecessarily complicate matters.

Comment 34

[Stuart Morgan]

Comment on attachment 251556 [details] [diff] [review]
applies murph and hwaara's suggestions

>+- (IBAction)newBookmark:(id)aSender;

Should this maybe be newBlankBookmark: or newInlineBookmark:, to make its function clear?

>+- (IBAction)newBookmarkFolder:(id)aSender;
...
> - (IBAction)addBookmarkFolder:(id)aSender;

We can't have both newBookmarkFolder: and addBookmarkFolder:. If we really need two different methods, one or (more likely) both need to be renamed.

>+  // add blank bookmark after current selection, or at top if nothing is selected (which should be impossible)

It's not only possible, but easy, and adding at the top instead of the bottom feels really wrong.


I thought I would like this behavior, but when I tried it my very first thought when confronted with the completely blank bookmark was "How many people would actually know how to fill in a blank bookmark?". So while this is better than the sheet behavior, I'm actually finding myself leaning a bit toward comment 4.

Comment 35

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

I use it a lot when adding bookmarklets, sometimes when adding new shortcut searches, and other times for reasons I can't recall atm.  Right now I have to bookmark about:bookmarks or duplicate some random bookmark and make changes, which is not the end of the world, but it's an annoyance every time I go to do it.

Comment 36

[Chris Lawson (gone)]

I personally really enjoy this behaviour, and my general feeling is that

a) when in the bookmark manager, adding a bookmark for the underlying page (or about:bookmarks) seems rather unexpected *too* (at least not any less weird than having to fill in a blank bookmark)
b) the more closely we can imitate standard Finder behaviour in a view that is very similar to a Finder list view, the better
c) very few people would actually run into a situation where they're using this by accident, and the people who experience this use case often would know what they're doing and why they want it to be this way (see comment 35, for instance)

cl

Comment 37

[Stuart Morgan]

a) Yes, I specifically said I thought it was better than the sheet.
b) Finder doesn't have a "create a new file" action, so I'm not sure how that relates.
c) Even assuming that's true, it's also true for all kinds of advanced prefs and weird fringe functionality. It doesn't make it good UI.

To be clear, I sr-'d it for the comments I made about the code; if those are addressed I'll approve it.

I'm just saying that I think we are band-aiding a bad behavior by replacing it with a marginally less bad behavior. Beyond 1.1 I think we should figure out what circumstances most users would currently need the ability to make a blank bookmark, address those, and then remove the blank bookmark UI. Search shortcuts are a good example in that providing users a way to hand-create them with a blank bookmark is about the worst UI we could possibly have if we want more than a tiny fraction of users to be able to take advantage of them.

Comment 38

[Chris Lawson (gone)]

b) No, but Finder has UI for making new folders, which is conceptually very similar (and which this bug also covers).

I see your point about blank bookmarks (not folders) being a bit of a band-aid as far as making bookmarklets or search shortcuts, though.

cl

Comment 39

[Stuart Morgan]

Folders I agree completely should be created with inline edit. No technical knowledge is needed to pick a name for a folder ;) I'm talking just about blank bookmarks.

Comment 40

[Chris Lawson (gone)]

Attachment: Fixed

TIA for the nib love (the nib already here should serve as a fine jumping-off point).

Comment 41

[Stuart Morgan]

Comment on attachment 254900 [details] [diff] [review]
Fixed

This patch has a serious problem that I didn't notice until I sat down to redo the nib:

>+  BookmarkFolder* currentFolder = [self selectedContainerFolder];
>+  if (!currentFolder)
>+    return;
>+  int selectedBookmarkIndex = [mBookmarksOutlineView selectedRow];
>+  // add after current selection, or at bottom if nothing is selected
>+  Bookmark* newBookmark = [currentFolder addBookmark:@""
>+                                                 url:@""
>+                                          inPosition:(selectedBookmarkIndex ? (selectedBookmarkIndex + 1) : [mBookmarksOutlineView numberOfRows])
>+                                         isSeparator:NO];

This selection math totally ignores the possibility of subfolders. It not only wouldn't put the bookmark in the right place if you have a bookmark in a subfolder selected, it would be really easy to get index out of bounds exceptions.

Also, as a note to whoever picks this up:
>+- (IBAction)newInlineBookmark:(id)aSender;
>+- (IBAction)newBookmarkFolder:(id)aSender;
I wasn't thinking about 'new' being bad to use from a naming convention standpoint, and the Inline part clashes with all the other things that are inline but don't say so, so these should be addBlankBookmark: and addBookmarkFolder:


Unless someone reworks this tomorrow, we should just bump it from 1.1. It would be nice to have, but nothing is broken if we don't take it. Pushing it out would also let us spend the time to reconcile with the menu (where I think Add Bookmark Folder... should probably lose the '...' and use this behavior when the manager is selected)

Comment 42

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

Bumping.

Comment 43

[Chris Lawson (gone)]

I can clean this up for 1.6, pending discussion of the issues raised in the last paragraph of comment 41.

cl

Comment 44

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

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 45

[Chris Lawson (gone)]

Kicking this back to nobody until we decide what we want to do about it.

Comment 46

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

I ended up filing bug 576718 on folder frustrations without realizing that cl's patch for this bug basically did what I wanted.  We should perhaps rescue the folder behavior and move it over there (and address comment 41).