255255 - after searching bookmarks, the results are not editable

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Asa Dotzler [:asa]]

When you search bookmarks the results can't be edited. 

Steps:
load up bookmarks sidebar of manager
type some search string into the search field
right click on one of the results and select "properties".

Results: nothing happens.
Expected: you get a properties dialog.

Rinse and repeat for all editing functions: delete, cut, copy, paste, etc.

Tested latest branch build but this has been around forever.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 2

[chris hofmann]

does it make sense to try and get this for final?  -pr, ?1.0

renominate if that doesn't make sense.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Makes sense for final, not worth it blocking PR.  I've been poking at this since
yesterday; I've got part of it fixed already, but there's another chunk that's
going to need a bit more thinking.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 7

[Tracy Walker [:tracy]]

Several issues with managing a searched list of bookmarks:

Properties context menu item is greyed out. 
Move, Properties and Rename buttons are greyed out.
Delete is not greyed out, but doesn't work as a button or in context menu.
Cut and Paste are available but don't work.  Copy does work.
Select more than one search item and Move becomes active and works.

Comment 8

[Asa Dotzler [:asa]]

Vlad says he's near a fix for this. 

Comment 9

[Mike Connor [:mconnor]]

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

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Minusing for 1.0 -- I have a patch that's 90% of the way there, but the last 10%
are turning out to be a huge pain; I don't want to rummage too deep in the
bookmarks code for this for fear of breaking something else.  Will revisit the
patch, but this probably shouldn't block 1.0.  (Asa, re-+ if you feel extremely
strongly about this :)

Comment 11

[Justin Kerk]

Note that although the Move button works (when multiple bookmarks are selected),
Firefox won't let you drag-and-drop any of the search results into folders on
the left pane, no matter how many you select.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 16

[Fareed Rizkalla]

this is a very big problem, if someone has a large bookmarks list and he can't
edit it

Comment 17

[Wolf [:wolf]]

Only aviary peers can set blocking +/- flags.. Everybody else should nominate ?
the flag for a peer ro grant or deny. :-)

Comment 18

[Fareed Rizkalla]

has this ever worked on any nightlies

Comment 19

[Robert Parenton]

Back to the original blocking-aviary1.0-

Comment 20

[Phil Ringnalda (:philor)]

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

Comment 21

[Phil Ringnalda (:philor)]

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

Comment 22

[Asaf Romano (gone)]

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

Comment 23

[Phil Ringnalda (:philor)]

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

Comment 24

[Phil Ringnalda (:philor)]

Note also that describing Move as "works" with multiple items selected is
putting it strongly: it doesn't remove them from the search results or their
original location, it actually functions as Copy instead.

Comment 25

[dean]

Not wishing to be melodramatic. Rather, not wishing to be *excessively*
melodramatic, I want to emphasize and explain the importance I attach to this
feature. I consider the Web's 2 or 3 other cardinal (?Seminal) features, to be a
database. *THE* Database, (and its Mother, etc.) 

A database, like a reference book, cannot be read cover-to-cover. Rather, its
utility is directly proportional to its index: the quality, accessibility,
versatility, accuracy, and completeness of its Index.

We who use bookmarks (at all) use them as the Index of the Web. The ability of
FF to allow users to manipulate the Bookmark database must Not be restricted in
any way. No matter how FireFox surpasses IE in all regards, for those who view
bookmark management seriously as do I, FF will remain tragically flawed with
development arrested at build v93. -Dean Schaffer, MD "Maggot"

Comment 26

[dean]

Attachment: comment

Not wishing to be melodramatic. Rather, not wishing to be *excessively*
melodramatic, I want to emphasize and explain the importance I attach to this
feature. I consider the Web's 2 or 3 other cardinal (?Seminal) features, to be
a database. *THE* Database, (and its Mother, etc.) 

A database, like a reference book, cannot be read cover-to-cover. Rather, its
utility is directly proportional to its index: the quality, accessibility,
versatility, accuracy, and completeness of its Index.

We who use bookmarks (at all) use them as the Index of the Web. The ability of
FF to allow users to manipulate the Bookmark database must Not be restricted in
any way. No matter how FireFox surpasses IE in all regards, for those who view
bookmark management seriously as do I, FF will remain tragically flawed with
development arrested at build v93. -Dean Schaffer, MD "Maggot"

Comment 27

[theoglobal]

5-star applause ***** to docmaggot for the post. Its not melodramatic: in fact
in contrast it's concise, insightful, well-expressed. Bookmarks are not just
'cosmetic local conveniences' but a true subset of an internet index. Thanks
docmaggot and thanks to each of you working on the fix. VERY appreciated.

Comment 28

[Mike Connor [:mconnor]]

Gents, this isn't a messageboard/advocacy forum.  Comments not related to fixing
the bug aren't appropriate.  See https://bugzilla.mozilla.org/etiquette.html

Comment 29

[José Jeria]

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

Comment 30

[:Gavin Sharp [email: [e-mail]]]

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

Comment 31

[denis]

Mozilla version of this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=164711

Comment 32

[PikeUK]

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

Comment 33

[:Gijs (he/him)]

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

Comment 34

[Steven Boothe]

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

Comment 35

[Steven Boothe]

As of build: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.6)
Gecko/20050317 Firefox/1.0.2

1. Menu Item: Manage Bookmarks
2. Type string into search field.
3. Select one of the matching results
4. The "Delete" button lights up indicating selected bookmark from search can be
deleted.
5. Pressing on the "Delete" button accomplishes nothing.
6. Right clicking on selected bookmark displays: cut, copy, paste, delete as
available options.
7. Selecting delete from the context menu accomplishes nothing.

Comment 36

[Darkside]

So vlad, how's that last 10% coming?
Thanks for working on this!

Comment 37

[MW]

Temporary Workaround:

1. (optional step)  Install  "Enhanced Bookmarks Manager" extension from
http://forums.mozillazine.org/viewtopic.php?p=1425196 which enables you to
search on various other fiels such as URL, keyword etc.

2. Install  "Locate in Bookmark Folders" extension from
http://www.extensionsmirror.nl/index.php?showtopic=3264

3. After search in complete highlight the bookmark and click the "locate"
toolbarbutton

4. Once the Folder in which the Bookmark resides is located the you can
edit/cut/copy/move/paste the bookmark to your hearts content.....

Comment 38

[dean]

Hey Vlad,
Would you offer your own comment on the workaround which was just so graciously
offered by MW?

You guys are *both*  heroes to keep plugging away at this tough nut- but over 60
of us are intensely interested about this particular failure and so very
grateful for your efforts.... particularly when they bear fruit!

Comment 39

[tabmix.onemen]

when you delete bookmark and then perform search for the same bookmark nmae, the
deleted bookmark is showing in the search result? is it related to this bug?

Comment 40

[tabmix.onemen]

Attachment: this patch fix the bug

Comment 41

[Asaf Romano (gone)]

Comment on attachment 184417 [details]
this patch fix the bug

this is a modified version of bookmarks.js, not a patch.

Comment 42

[tabmix.onemen]

sorry for the wrong term. 

Comment 43

[Radek 'sysKin' Czyz]

(In reply to comment #40)
> Created an attachment (id=184417) [edit]
> this patch fix the bug
> 

I tried to check your file but the differences between your file and current
bookmarks.js are huge. Did you, by any chance, use some outdated bookmarks.js?
If so, could you repeat your changes using the bookmark.js from Deer Park Alpha
(either release candidate available now or real Deer Park Alpha if it's out when
you're reading this).

I'm sure someone will create a patch for you, if you can't.

Comment 44

[tabmix.onemen]

(In reply to comment #43)
> (In reply to comment #40)
> > Created an attachment (id=184417) [edit] [edit]
> > this patch fix the bug
> > 
> 
> I tried to check your file but the differences between your file and current
> bookmarks.js are huge. Did you, by any chance, use some outdated bookmarks.js?
> If so, could you repeat your changes using the bookmark.js from Deer Park Alpha
> (either release candidate available now or real Deer Park Alpha if it's out when
> you're reading this).
> 
> I'm sure someone will create a patch for you, if you can't.
i did use the last Rev 1.99 of bookmark.js.
can you or anybody elese check if this file work as expected....
just replace ths file bookmark.js with this file.
if you and other can confirm it's working then maybe someone will create a patch

Comment 45

[Thomas Bertels]

Attachment: Diff between bookmarks.js 1.99 and "this patch fix the bug"

(In reply to comment #44)
It works under Deer Park Alpha 1, but are you sure you made the changes from
the 1.99 version ?
Could you edit this patch to remove everything you didn't change ?

Comment 46

[tabmix.onemen]

i made all the changes to the bookmarks.js file that was in the last build.
it's seems to me that the differences, are in the line that start with:
//@line xx
"/cygdrive/c/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/browser/components/bookmarks/content/bookmarks.js"

and all the code that say :
#ifdef ..... #else ...... #endif is somehow missing from the file in tha trunk
build that i use (i'm use winXP)
other than that all the other changes are mine.

Comment 47

[Thomas Bertels]

Attachment: Patch without comments

Here is the patch without what you don't seem to have modified. Check if it's
right. Use the bookmarks.js modified version 1 to correct anything I missed.
Please add some comments at least for each function and attach the new version
of the resulting file to this bug.

Comment 48

[Thomas Bertels]

Attachment: bookmarks.js modified version 1

Comment 49

[tabmix.onemen]

Attachment: bookmarks.js modified version 2 with comments

the fix for enable edit bookmarks after searching is simple.
only need to fix case "cmd_bm_rename" in isCommandEnabled function, and put
back aSelection.parent[i] if it's null in removeSelection.

all the other new code is to prevent nsLocalSearchService to return deleted
bookmarks in search result

Comment 50

[Thomas Bertels]

Attachment: Patch with comments

Tell me if it's ok so I can request a review.

Comment 51

[tabmix.onemen]

(In reply to comment #50)
> Created an attachment (id=184817) [edit]
> Patch with comments
> 
> Tell me if it's ok so I can request a review.

it's look ok to me. thank you fo your help

Comment 52

[Thomas Bertels]

Comment on attachment 184817 [details] [diff] [review]
Patch with comments

You can set "this patch fix the bug" as obsolete.

Comment 53

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 184817 [details] [diff] [review]
Patch with comments

This completely breaks Live Bookmarks, in that you're able to edit and do all
sorts of things with Live Bookmark entires that you aren't/shouldn't be allowed
to do.	(Also, live bookmarks themselves don't show up when you search, though
I think that's an old bug (unrelated to this).)  Some more specific comments
below:

>   kIOContractID     = "@mozilla.org/network/io-service;1";
>   kIOIID            = Components.interfaces.nsIIOService;
>   IOSVC             = Components.classes[kIOContractID].getService(kIOIID);
>+  
>+  gProperties       = [RDF.GetResource(gNC_NS+"Name"),
>+                       RDF.GetResource(gNC_NS+"URL"),
>+                       RDF.GetResource(gNC_NS+"ShortcutURL"),
>+                       RDF.GetResource(gNC_NS+"Description")];

What about the other properties that may be on a bookmark?  There already is a
gProperties in bookmarksProperties.js (used for the right-click Properties
command).

>@@ -458,9 +467,32 @@
>   
>     var sBookmarkItem = ""; var sTextUnicode = ""; var sTextHTML = "";
>     for (var i = 0; i < aSelection.length; ++i) {
>+      sBookmarkItem += aSelection.item[i].Value + "\n";
>+      ////////////////////////////////////////////////////////////////////////////////

Please don't have the long /////.. comments in; those should really be used for
major section
divisions in the code, not applicable inside a function.  A blank line should
be fine.

> [...]
>+      sBookmarkItem = childCount +  "\n" + sBookmarkItem + "]-[";

What happens if I put in ]-[ in my bookmark title?


>@@ -996,7 +1044,8 @@
>       var flavourArray = Components.classes[kSuppArrayContractID].createInstance(kSuppArrayIID);
>       const kSuppStringContractID = "@mozilla.org/supports-cstring;1";
>       const kSuppStringIID = Components.interfaces.nsISupportsCString;
>-    
>+
>+      // i think that "text/x-moz-url" here causing problem

What problem?

>       var flavours = ["moz/bookmarkclipboarditem", "text/x-moz-url"];
>       for (i = 0; i < flavours.length; ++i) {
>         const kSuppString = Components.classes[kSuppStringContractID].createInstance(kSuppStringIID);

Comment 54

[tabmix.onemen]

Attachment: revise patch after vladimir comments

Comment 55

[Mike Connor [:mconnor]]

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

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Vladimir, I think you also missed this request for review. Can you take a look
at the current patch?

Comment 57

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 186750 [details] [diff] [review]
revise patch after vladimir comments

r=vladimir, looks good.. thanks for fixing the previous issues!

I'll upload a patch that applies cleanly to the trunk right now (some misc
things needed to be merged) and get it checked in.

Comment 58

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch updated for trunk

Comment 59

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in; could use some QA coverage.

Comment 60

[Asa Dotzler [:asa]]

Vlad, we'll get right on this in the morning. 

Comment 61

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #60)
> Vlad, we'll get right on this in the morning. 

Great, thanks!

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Works fine for me with my three minutes old CVS build Mozilla/5.0 (X11; U; Linux
i686; en-US; rv:1.8b4) Gecko/20050719 Firefox/1.0+. Thanks

Comment 63

[Tracy Walker [:tracy]]

Works fine on Deep Park builds from 07/19 on Windows, Linux and Mac

Comment 64

[Wayne Mery (:wsmwk)]

WFM XP Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050719
Firefox/1.0+

Comment 65

[Steven Boothe]

WFM win2k pro
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050719
Firefox/1.0+

(Fantastic!!!)

Comment 66

[lucano]

Worked as soon as I applied the 7/19 update. Good work!

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050719
Firefox/1.0+

Comment 67

[Gary Elshaw]

OS X 10.4.2

Appears fixed in the 1.1 nightly here. DP Alpha2 and Firefox 106 exhibit the problem, but you knew that 
anyway

Comment 68

[Neil Paris]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050719 Firefox/1.0+

Everything looks great

Comment 69

[O. Atsushi (Torisugari)]

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

Comment 70

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change