Last Comment Bug 81893 - Cannot delete bookmarks from Search Results window
: Cannot delete bookmarks from Search Results window
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: P2 normal with 21 votes (vote)
: seamonkey2.0b2
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
: 93370 100006 138504 164711 195208 219656 273489 301393 375925 (view as bug list)
Depends on: 123679 160019
Blocks:
  Show dependency treegraph
 
Reported: 2001-05-20 19:31 PDT by Chris Lyon
Modified: 2010-08-17 15:05 PDT (History)
22 users (show)
dbaron: blocking1.4.1-
dbaron: blocking1.5a-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch (54.17 KB, patch)
2004-09-08 19:37 PDT, Chouser
no flags Details | Diff | Splinter Review
stripped down patch from bug 123679 (4.86 KB, patch)
2009-08-07 07:28 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review-
Details | Diff | Splinter Review
patch v2 (6.08 KB, patch)
2009-08-08 04:39 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review-
Details | Diff | Splinter Review
patch v3 (7.15 KB, patch)
2009-08-11 11:56 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v4 (7.56 KB, patch)
2009-08-11 14:54 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description Chris Lyon 2001-05-20 19:31:46 PDT
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 2001-05-20 19:42:09 PDT
The same error occurs when cutting a bookmark from the Search Results Window.
Comment 2 Chris Lyon 2001-05-21 18:01:32 PDT
Fixed typo in summary.  Sorry for the spam.
Comment 3 Asa Dotzler [:asa] 2001-06-26 23:54:02 PDT
confirmed with win32 mozilla build 062615. javascript error and failure to
delete or cut.
Comment 4 Chris Lyon 2001-08-03 00:23:13 PDT
*** Bug 93370 has been marked as a duplicate of this bug. ***
Comment 5 Chris Lyon 2001-09-20 05:45:36 PDT
*** Bug 100006 has been marked as a duplicate of this bug. ***
Comment 6 Ben Goodger (use ben at mozilla dot org for email) 2001-10-19 17:28:01 PDT
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) 2001-12-12 18:15:52 PST
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 2002-02-18 17:29:32 PST
nsbeta1- per Nav triage team
Comment 9 Chris Lyon 2002-04-23 08:19:06 PDT
*** Bug 138504 has been marked as a duplicate of this bug. ***
Comment 10 Pierre Chanial 2002-08-18 07:58:22 PDT
taking, js error fixed by disabling the delete operation for found items for now.
Comment 11 Chris Lyon 2003-02-27 06:32:24 PST
Javascript error is now gone, but bookmark is not deleted.
20030210 on WinXP.
Comment 12 Chris Lyon 2003-02-27 06:32:33 PST
*** Bug 195208 has been marked as a duplicate of this bug. ***
Comment 13 José Jeria 2003-05-26 04:23:50 PDT
Just clicking on an bookmars that you searched for gives a javascript error
Comment 14 Jesus Cea 2003-07-03 08:25:01 PDT
Anybody still alive?. This bug is OLD, SERIOUS and should be "obvious".
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-07-08 12:46:12 PDT
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 2003-08-17 19:55:04 PDT
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 2003-09-01 18:56:42 PDT
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 2003-10-05 08:12:16 PDT
retargeting
Comment 19 Steve 2004-01-05 09:52:06 PST
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 2004-01-24 09:40:37 PST
#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 2004-04-18 13:51:11 PDT
would be a very helpful feature
Comment 22 Rory Parle 2004-05-14 08:16:50 PDT
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 2004-05-14 10:26:51 PDT
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! 2004-06-04 02:18:59 PDT
(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 2004-06-04 02:27:12 PDT
*** Bug 219656 has been marked as a duplicate of this bug. ***
Comment 26 Mitchell Mebane 2004-07-12 12:31:47 PDT
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 2004-09-05 02:41:05 PDT
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 2004-09-08 19:37:13 PDT
Created attachment 158263 [details] [diff] [review]
proposed patch
Comment 29 Chouser 2004-09-08 19:45:17 PDT
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 2004-09-08 20:08:19 PDT
chouser: bug 123679 is the reason why this bug has not been fixed.
Comment 31 OstGote! 2004-12-07 03:00:38 PST
*** Bug 273489 has been marked as a duplicate of this bug. ***
Comment 32 OstGote! 2005-07-20 11:39:05 PDT
*** Bug 301393 has been marked as a duplicate of this bug. ***
Comment 33 Ray Booysen 2006-02-13 07:44:52 PST
Reassigning as per Bug #32644
Comment 34 Jens Hatlak (:InvisibleSmiley) 2009-08-01 04:25:45 PDT
This should be fixed when bug 123679 is fixed, setting dependency.
Comment 35 Jens Hatlak (:InvisibleSmiley) 2009-08-06 09:38:39 PDT
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) 2009-08-07 07:28:59 PDT
Created attachment 393181 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-08-07 16:09:11 PDT
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) 2009-08-08 04:39:26 PDT
Created attachment 393357 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-08-10 06:17:52 PDT
(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 neil@parkwaycc.co.uk 2009-08-10 06:30:27 PDT
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 neil@parkwaycc.co.uk 2009-08-10 06:36:14 PDT
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) 2009-08-11 11:56:33 PDT
Created attachment 393828 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-08-11 14:04:29 PDT
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) 2009-08-11 14:54:53 PDT
Created attachment 393885 [details] [diff] [review]
patch v4
Comment 45 Stefan [:stefanh] 2009-08-15 08:13:19 PDT
http://hg.mozilla.org/comm-central/rev/05871ae7b38f
Comment 46 Jens Hatlak (:InvisibleSmiley) 2010-08-17 15:03:02 PDT
*** Bug 164711 has been marked as a duplicate of this bug. ***
Comment 47 Jens Hatlak (:InvisibleSmiley) 2010-08-17 15:05:05 PDT
*** Bug 375925 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.