Cannot delete bookmarks from Search Results window

RESOLVED FIXED in seamonkey2.0b2

Status

SeaMonkey
Bookmarks & History
P2
normal
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: Chris Lyon, Assigned: InvisibleSmiley)

Tracking

Trunk
seamonkey2.0b2
Dependency tree / graph
Bug Flags:
blocking1.4.1 -
blocking1.5a -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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
(Reporter)

Comment 1

16 years ago
The same error occurs when cutting a bookmark from the Search Results Window.
(Reporter)

Comment 2

16 years ago
Fixed typo in summary.  Sorry for the spam.
Summary: JavaScript Error when deleting a boomark from Search Results window → JavaScript Error when deleting a bookmark from Search Results window

Comment 3

16 years ago
confirmed with win32 mozilla build 062615. javascript error and failure to
delete or cut.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

16 years ago
*** Bug 93370 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 5

16 years ago
*** Bug 100006 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1
Paul Chen is now taking Bookmarks bugs. For your convenience, you can filter 
email notifications caused by this by searching for 'ilikegoats'.

Assignee: ben → pchen
Status: ASSIGNED → NEW
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. 
Assignee: pchen → ben
Keywords: nsbeta1
Priority: -- → P2
Target Milestone: mozilla1.1 → mozilla0.9.9
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.9 → mozilla1.1

Comment 8

16 years ago
nsbeta1- per Nav triage team
Keywords: nsbeta1 → nsbeta1-
(Reporter)

Comment 9

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

Comment 10

15 years ago
taking, js error fixed by disabling the delete operation for found items for now.
Assignee: ben → chanial
Status: ASSIGNED → NEW
Depends on: 160019
(Reporter)

Comment 11

15 years ago
Javascript error is now gone, but bookmark is not deleted.
20030210 on WinXP.
Summary: JavaScript Error when deleting a bookmark from Search Results window → Cannot delete bookmarks from Search Results window
(Reporter)

Comment 12

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

Comment 13

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

Comment 14

14 years ago
Anybody still alive?. This bug is OLD, SERIOUS and should be "obvious".
Flags: blocking1.5a?
Flags: blocking1.4.x?
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.
Flags: blocking1.5a?
Flags: blocking1.5a-
Flags: blocking1.4.x?
Flags: blocking1.4.x-

Comment 16

14 years ago
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

14 years ago
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

14 years ago
retargeting
Target Milestone: mozilla1.1alpha → Future

Comment 19

14 years ago
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

14 years ago
#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

13 years ago
would be a very helpful feature

Comment 22

13 years ago
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

13 years ago
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

13 years ago
(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

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

Comment 26

13 years ago
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

13 years ago
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

13 years ago
Created attachment 158263 [details] [diff] [review]
proposed patch

Updated

13 years ago
Attachment #158263 - Attachment filename: bookmarks.js → mozilla/xpfe/components/bookmarks/resources/bookmarks.js

Comment 29

13 years ago
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

13 years ago
chouser: bug 123679 is the reason why this bug has not been fixed.
Product: Browser → Seamonkey

Comment 31

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

Comment 32

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

Comment 33

11 years ago
Reassigning as per Bug #32644
Assignee: p_ch → nobody
QA Contact: claudius → bookmarks
(Assignee)

Comment 34

8 years ago
This should be fixed when bug 123679 is fixed, setting dependency.
Depends on: 123679
(Assignee)

Updated

8 years ago
Blocks: 375925
(Assignee)

Comment 35

8 years ago
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.
No longer blocks: 375925
(Assignee)

Comment 36

8 years ago
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.
Assignee: nobody → jh
Attachment #158263 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #393181 - Flags: review?(neil)
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().
Attachment #393181 - Flags: review?(neil) → review-
(Assignee)

Comment 38

8 years ago
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?
Attachment #393181 - Attachment is obsolete: true
Attachment #393357 - Flags: review?(neil)
(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 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).
Attachment #393357 - Flags: review?(neil) → review-
I wonder whether there's a way of notifying the deleted bookmark and only rebuilding if that bookmark is actually visible.
(Assignee)

Comment 42

8 years ago
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.
Attachment #393357 - Attachment is obsolete: true
Attachment #393828 - Flags: review?(neil)
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.)
(Assignee)

Comment 44

8 years ago
Created attachment 393885 [details] [diff] [review]
patch v4
Attachment #393828 - Attachment is obsolete: true
Attachment #393828 - Flags: review?(neil)
Attachment #393885 - Flags: review?(neil)

Updated

8 years ago
Attachment #393885 - Flags: review?(neil) → review+
(Assignee)

Updated

8 years ago
Attachment #393885 - Flags: superreview?(neil)

Updated

8 years ago
Attachment #393885 - Flags: superreview?(neil) → superreview+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
OS: Windows 2000 → All
Hardware: x86 → All
Target Milestone: Future → seamonkey2.0b2

Comment 45

8 years ago
http://hg.mozilla.org/comm-central/rev/05871ae7b38f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Duplicate of this bug: 164711
(Assignee)

Updated

7 years ago
Duplicate of this bug: 375925
You need to log in before you can comment on or make changes to this bug.