73184 - can't inline rename bookmarks/folders or add bookmark folders

Status: VERIFIED DUPLICATE of bug 68537

(SeaMonkey :: Bookmarks & History, defect)

Description

[Brent Hendricks]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.14-5.0 i686; en-US; 0.8.1)
BuildID:    2001032303

After inline editing the name of a bookmark or folder and pressing enter, the
new name doesn't stick.  It goes back to the original name. 

Reproducible: Always
Steps to Reproduce:
1. Pull up the bookmarks manager
2. Select a bookmark or folder
3. Click the "Rename..." button
4. Type a new name in the inline box
5. Press Enter

Actual Results:  The bookmark or folder name goes back to its original value

Expected Results:  The new name should replace the old.

Comment 1

[Frank Jacobberger]

Not an issue in 3-23 Mozilla 0.8.1-6 - FIXED

Comment 2

[Boris Zbarsky [:bzbarsky]]

This broke after 0.8.1 branched, apparently.  I'm seeing this too, with
2001-03-23-08 build.

Specifically, when you click on a bookmark to edit it inline, the field comes up
completely empty instead of having the old text in it.  Whatever you type in the
field is not saved.

Comment 3

[Keyser Sose]

Adding keywords

Comment 4

[Claudius Gayle]

sooooo, i see this broken on a 2001032304 Win build but fixed on a 2001032310 mac build
AND on a 2001032311 Linux build.

that tells me based on the reporter's build ID a fix was checked in btw 2001032303 and ...04.
I thought that might be of interest to people.

It probably wouldn't be too hard to check the logs and see who did what during that hour...but
instead I'm just going to mark this fixed and verified.

Comment 5

[Claudius Gayle]

VERIFIED Fixed (with 2001032311 builds)

Comment 6

[zyniker]

Is it really fixed? With build 2001032321 under linux the rename of bookmarks
doesn't work. Also if I try to create a new folder I'm unable to give the folder
a name. 
If I right-click a bookmark the menu opens but nothing works.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Still broken for me in a 2001-03-24 morning CVS build on Linux.

Comment 8

[Blake Ross]

I think this is mine.

Comment 9

[Blake Ross]

Reassigning to me .

Comment 10

[Fabian Guisset]

indeed Blake, it's yours..
in xpfe/global/resources/content/treeBindings.xml, line 152 and line 134 you 
changed box.lastChild.value into box.lastChild.label.
However lastChild is a textbox, and so it had to stay .value.
Anyone care to produce a patch?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attaching patch with Fabian's changes and another change that makes the current
value appear highlighted in the textbox like it used to (so you can edit it or
overwrite it).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: patch

Comment 14

[Claudius Gayle]

note that the attached patch also fixes bug 73369 which has been marked as a dupe but is actually
a slightly more severe manifestation b/c it affects naming new folders as well.

Comment 15

[Fabian Guisset]

bz, are you sure the first change from label to value has to be done?
box.firstChild is a <text> widget, and <text> widget's use label, not value.
Or am I missing something?

Comment 16

[Boris Zbarsky [:bzbarsky]]

well....  I haven't dug into the code comletely, so I'm not sure why it works... 
but without that change the beavior is wrong.  With the change, the behavior is 
correct.  Try both.  I'll try to figure it out sometime this weekend.

Comment 17

[Fabian Guisset]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

The xul:text has the following attribute set:                  
inherits="crop,value=label,align,mode"

Would that affect things in any way?  Note: getAttribute("value") returns the
right thing.  getAttribute("label") returns "", .label returns "undefined"

Comment 19

[Fabian Guisset]

doh! you're right, my mistake..
r=fabian

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[André Dahlqvist]

With this patch applied I can rename/create folders again, but each time I
create a folder I get this error:

JavaScript error: 
chrome://global/content/treeBindings.xml#treecell-indented-folder-inline-edit-base.fieldChange()
line 7: cell.setMode is not a function

I don't know if it is related, but I thought I would report it anyway.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Well, it's in the same file.. So attaching updated patch that gets rid of that
error too.  :)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: patch that checks for existence of cell.setMode too

Comment 24

[timeless]

r=timeless

Comment 25

[Blake Ross]

timeless: Stop reassigning my goddamn bugs, this is like the 8th time you've 
done this (and the 8th time I've said to stop).  These bugs are on my list for 
a reason; so I can look over the attached patches and check them in if 
necessary. At the very least, you could have the courtesy to cc me.  I'll 
reassign it to bzbarsky when I'm ready to check it in.

Comment 26

[timeless]

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Claudius Gayle]

changing summary to capture the nature of the dupes, since the belief is that
these bugs are all the same root problem.

Comment 29

[Claudius Gayle]

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

Comment 30

[Claudius Gayle]

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

Comment 31

[Claudius Gayle]

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

Comment 32

[Blake Ross]

Cc'ing alec for sr.

Comment 33

[Alec Flett]

sr=alecf

Comment 34

[Boris Zbarsky [:bzbarsky]]

Blake, this is reviewed and all. Do you want to check it in or should we just
wait for Ben to check in his fix for this (I can't find the relevant bug right now)

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

Comment 36

[timeless]

blake: bugs should be assigned to their owners, if you need a way to track bugs 
you need to check in, use something else.

Comment 37

[timeless]

fix checked in

Comment 38

[Blake Ross]

Round and round we go!

timeless, I own this bug because I'm the dumbass that caused it.  I already 
said I'm fully willing to reassign to bz once_I_check_in_his_fix.  I appreciate 
this fix, I really do.  I hesitated to check it in for good reason, because I 
knew Ben was working on an extended fix simultaneously.

If you would like to write [e-mail] and explain to them why bz needed 
to own this bug for those few days, please go right ahead.  Don't forget to 
mention that you went and checked in this patch when even its creator was 
kindly willing to ask whether we should just wait for Ben's first.

That said, I'm reopening this bug and marking it a duplicate of Ben's bug.  Ben 
is almost certainly going to back the fix out anyways.  Boris, I'm sorry you 
had to be caught in the middle of this.  Thanks again for this patch!

Comment 39

[Blake Ross]

*** This bug has been marked as a duplicate of 68537 ***

Comment 40

[Claudius Gayle]

VERIFIED Dupe since ben's fix for that bug incorporates a fix for this bug.