81893 - Cannot delete bookmarks from Search Results window

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect, P2)

Description

[Chris Lyon]

Using build 2001051920 on Win2K

Steps to Reproduce:
1. Open Manage Bookmarks Window
2. Search for a bookmark (Ctrl+F)
3. Delete one of the Search Results from the Search Results window

Result:
Bookmark does not get deleted and the following error occurs:

Error: [Exception... "Component returned failure code: 0x80004005 
(NS_ERROR_FAILURE) [nsIRDFContainer.Init]"  nsresult: "0x80004005 
(NS_ERROR_FAILURE)"  location: "JS frame :: 
chrome://communicator/content/bookmarks/bookmarksOverlay.js :: anonymous :: 
line 588"  data: no]
Source File: chrome://communicator/content/bookmarks/bookmarksOverlay.js
Line: 588

Reproducible:
Always

Expected Results:
Bookmark gets deleted from both Search Results and bookmark list

Comment 1

[Chris Lyon]

The same error occurs when cutting a bookmark from the Search Results Window.

Comment 2

[Chris Lyon]

Fixed typo in summary.  Sorry for the spam.

Comment 3

[Asa Dotzler [:asa]]

confirmed with win32 mozilla build 062615. javascript error and failure to
delete or cut.

Comment 4

[Chris Lyon]

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

Comment 5

[Chris Lyon]

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

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Paul Chen is now taking Bookmarks bugs. For your convenience, you can filter 
email notifications caused by this by searching for 'ilikegoats'.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

nominating for nsbeta1. simple fix - need to catch exceptions when
initialization of seq fails. don't want to clutter *user's* js console with app
js errors. 

Comment 8

[Peter Trudelle]

nsbeta1- per Nav triage team

Comment 9

[Chris Lyon]

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

Comment 10

[Pierre Chanial]

taking, js error fixed by disabling the delete operation for found items for now.

Comment 11

[Chris Lyon]

Javascript error is now gone, but bookmark is not deleted.
20030210 on WinXP.

Comment 12

[Chris Lyon]

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

Comment 13

[José Jeria]

Just clicking on an bookmars that you searched for gives a javascript error

Comment 14

[Jesus Cea]

Anybody still alive?. This bug is OLD, SERIOUS and should be "obvious".

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this even a bug?  Is it clear to the user that deleting a bookmark from the
search results list should delete the bookmark from the bookmarks in addition to
from the search results list?  Or is the reverse clear -- that it would only
delete the bookmark from the search results lists?  I doubt it, which makes me
think that delete in the search results list shouldn't do anything.

In any case, it looks like a feature request and it's not going to block 1.4.x
or 1.5a.

Comment 16

[Al Savage]

David:
> Is this even a bug?  Is it clear to the user that deleting a bookmark from the
> search results list should delete the bookmark from the bookmarks in addition to
> from the search results list?

If my list of Bookmarks is several hundreds deep, and I want to delete one of
them but need to find it first, how do I accomplish this?

<Ctrl-F>,[enter criteria], yields a Search Results window in which Edit->Delete
has been disabled (from the RMB menu as well), so no deleting can occur here.  I
can now see the bookmark I want to delete, but can't do it in the Search Results
window, and I can't find it in the Bookmark Manager window.

Al S.

Comment 17

[Paul Sharpe]

The use case in Al's previous comment is also my experience of this bug.  So I
think that's empirical evidence that delete means "delete the bookmark" and
should be functional.

paul

Comment 18

[Vedran Miletic]

retargeting

Comment 19

[Steve]

This is one of the most serious useability problems in this area, the bookmark
handling, search, etc was much more useable in Netscape 4.7.  Mozilla's
functionality is terrible. 

I think Mozilla should model bookmark functionality along the lines of Netscape
4.7. 

1. Bookmark search took place in the current bookmark ctrl-b window. 

2. F3 searched more.

3. Bookmark duplicates could be easily found and deleted, or organized into
logical folders. None of this can be done now.  Sure you can find duplicates but
you can't find out where they are, in which folder.  You can't delete or move
them.  It's clunky. Worthless.  The most annoying feature of Mozilla. 

4. Another very powerful feature of Netscape 4.7 is the bookmark window
behaviour. With 4.7 you can open a folder in your bookmark tree, then go over to
the browser window, and drag the urlbar icon to the place you want it in the
bookmark window. The bookmark window stayed on top even though you had clicked
the icon in the browser window.  This was inegenius!  

With Mozilla when you click the browser window it pops in front of the bookmark
window. You must have the bookmark window situated off the side of the browser
window in order to drag.  Very crippled by comparison. 

Comment 20

[idgreenwald]

#19 makes a lot of good points that would be better expressed in a separate RFE.
 I'd hate to see the fix for the deletion problem held up by long arguments on
the merits of Netscape bookmarks vs Mozilla bookmarks.

I find the inability to delete the results of searching most distressing.

Comment 21

[black-hole]

would be a very helpful feature

Comment 22

[Rory Parle]

OS is listed as Windows 2000. I get this bug on Windows XP and bug 219656 seems
to be the same bug under MacOSX. Should this be OS->All?

Comment 23

[black-hole]

of course it should. I'm using W98se and GNU/Linux and it's everywhere the same,
it's a general bug not system related. I think the oriinating user just picked
his OS from the list...

Comment 24

[OstGote!]

(In reply to comment #22)
> and bug 219656 seems to be the same bug under MacOSX. Should this be OS->All?
(In reply to comment #23)
> of course it should. I'm using W98se and GNU/Linux and it's everywhere the same,
> it's a general bug not system related. 

Please change Hardware/OS to All/All. IMO Bug 219656 could be duplicated.  

Comment 25

[José Jeria]

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

Comment 26

[Mitchell Mebane]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040707 Firefox/0.9.2

I also cannot delete bookmarks from a bookmark search. (As in comment 11). I
agree with #16, this is a serious bug.

Comment 27

[black-hole]

Anyone still on it? Winning a browser war is not only done in security issues.
It's ALSO about having nice, mighty & userfriendly tools to work with. Of course
one could try to workaround - but 1st: that not the way it is supposed to be and
2nd: Mozilla is aiming for the masses (hey, it's not some text res. lynx like),
so it HAS to be friendly for INEXPERIENCED users cause the mass consists of
INEXPERIENCED users that want EASY HANDLING (and not finding out about
workarounds, howtos, doing 7 steps to accomplish something that could be done
with one button). Also more experienced people and profs. would surely be fond
of having their every day work done fast and without too much artistical jumps
and rolls. 
So if someone could please just add this feature. 

Comment 28

[Chouser]

Attachment: proposed patch

Comment 29

[Chouser]

I'm sorry for the spam and the fact that I obviously don't know how to properly
submit a patch.  Anyway, this patch allows the deletion of bookmarks while a
search filter is applied.  It doesn't work perfectly, however -- I think bug
123679 is still biting.

--Chouser

Comment 30

[Pierre Chanial]

chouser: bug 123679 is the reason why this bug has not been fixed.

Comment 31

[OstGote!]

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

Comment 32

[OstGote!]

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

Comment 33

[Ray Booysen]

Reassigning as per Bug #32644

Comment 34

[Jens Hatlak (:InvisibleSmiley)]

This should be fixed when bug 123679 is fixed, setting dependency.

Comment 35

[Jens Hatlak (:InvisibleSmiley)]

Update: Neil chose to fix bug 123679 differently so this one is not fixed by that one. My patch on that bug should still work, though.

Comment 36

[Jens Hatlak (:InvisibleSmiley)]

Attachment: stripped down patch from bug 123679

To be honest I "just" ported the patch from bug 255255 originally without checking whether all of the changes were needed. I'm still not sure. Anyway, the patch I'm attaching here is much smaller. Some explanations:

-      if (!isImmutable && aSelection.parent[i])
+      if (!isImmutable)

That change enables the Delete and Cut commands (cf. isCommandEnabled, cmd_bm_cut and cmd_bm_delete), i.e. the context menu entries and the keys.

+      // try to put back aSelection.parent[i] if it's null, so we can delete after searching
+      if (aSelection.parent[i] == null)
+        aSelection.parent[i] = BookmarksUtils.getParentOfContainer(aSelection.item[i]);

This makes removeSelection() not beep. ;-) More seriously, that function really requires the parent to be present (for RDFC.Init(), I checked that deleting doesn't work without the parent).

The rest is basically making sure that the search view is updated whenever needed.

Just play with it: Delete, Cut, Undo, Paste, all should work in search and normal mode.

Comment 37

[[e-mail]]

Comment on attachment 393181 [details] [diff] [review]
stripped down patch from bug 123679

>+    BookmarksUtils.refreshSearch();
I don't think this is ideal; it only refreshes the search if that's the window from which you deleted the bookmark. Unfortunately there's no really correct way to do this, since the local search service has no way to know which search windows are still open. Maybe notify everybody through nsIObserverService?

>       else if (parent) {
>         var parentProtocol = parent.Value.split(":")[0];
>         if (parentProtocol == "find" || parentProtocol == "file")
>           aSelection.parent[i] = null;
>       }
>-      if (!isImmutable && aSelection.parent[i])
>+      if (!isImmutable)
>         aSelection.containsMutable = true;
This deliberately stops you from deleting resources with find or file parents. The find parent is because it's not a true parent, but you don't want to try to delete resources with file parents! If the protocol here is "find" you could look up the original parent now.

>+      // try to put back aSelection.parent[i] if it's null, so we can delete after searching
And not here, where you don't actually know that you were doing a find.

>+      bmTree.setAttribute("ref", "");
>+      bmTree.setAttribute("ref", aRef);
This should probably just be a Rebuild().

Comment 38

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

(In reply to comment #37)
> (From update of attachment 393181 [details] [diff] [review])
> >+    BookmarksUtils.refreshSearch();
> I don't think this is ideal; it only refreshes the search if that's the window
> from which you deleted the bookmark. Unfortunately there's no really correct
> way to do this, since the local search service has no way to know which search
> windows are still open. Maybe notify everybody through nsIObserverService?

Good idea. I'm not sure I got it right 100% (especially the addObserver location, also note that I didn't know where to put removeObserver).

> >+      bmTree.setAttribute("ref", "");
> >+      bmTree.setAttribute("ref", aRef);
> This should probably just be a Rebuild().

Neither Rebuild() nor bmTree.Rebuild() worked and I couldn't find Rebuild() anywhere. Or did you mean creating a function of that name?

Comment 39

[[e-mail]]

(In reply to comment #38)
> (In reply to comment #37)
> > >+      bmTree.setAttribute("ref", "");
> > >+      bmTree.setAttribute("ref", aRef);
> > This should probably just be a Rebuild().
> Neither Rebuild() nor bmTree.Rebuild() worked and I couldn't find Rebuild()
> anywhere. Or did you mean creating a function of that name?
No, I was probably thinking of bmTree.builder.rebuild();

Comment 40

[[e-mail]]

Comment on attachment 393357 [details] [diff] [review]
patch v2

> function initBMService()
> {
>   kBMSVCIID = Components.interfaces.nsIBookmarksService;
>   BMDS  = RDF.GetDataSource("rdf:bookmarks");
>   BMSVC = BMDS.QueryInterface(kBMSVCIID);
>   BookmarkTransaction.prototype.RDFC = RDFC;
>   BookmarkTransaction.prototype.BMDS = BMDS;
>+
>+  Components.classes["@mozilla.org/observer-service;1"]
>+            .getService(Components.interfaces.nsIObserverService)
>+            .addObserver(bookmarksChangedObserver, "bookmarks-changed", false);
This is the wrong place to add an observer; we call this from all sorts of unusual places! Also you never remove the observer. Since it's only the tree that needs to rebuild, the best place to add/remove the observer would be in bookmarks.xml (I guess we should copy the style of the existing listeners).

Comment 41

[[e-mail]]

I wonder whether there's a way of notifying the deleted bookmark and only rebuilding if that bookmark is actually visible.

Comment 42

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3

(In reply to comment #41)
> I wonder whether there's a way of notifying the deleted bookmark and only
> rebuilding if that bookmark is actually visible.

I fear I cannot answer that question. If you care about it I suggest filing a follow-up bug.

Comment 43

[[e-mail]]

Comment on attachment 393828 [details] [diff] [review]
patch v3

>         BMSVC.transactionManager.RemoveListener(this.transactionListener);
> 
>         this.tree.builder.removeListener(this.builderListener);
> 
>         this.treeBuilder.removeObserver(this.builderObserver);
> 
>         this.tree.controllers.removeController(this.controller);
> 
>+        Components.classes["@mozilla.org/observer-service;1"]
>+                  .getService(Components.interfaces.nsIObserverService)
>+                  .removeObserver(bookmarksChangedObserver, "bookmarks-changed");
When I said it should use the same style, I meant that the observer should be a field in the XBL. (This also makes it easier to get a reference to the tree.)

Comment 44

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v4

Comment 45

[Stefan [:stefanh]]

http://hg.mozilla.org/comm-central/rev/05871ae7b38f