76525 - Tree in File Bookmark dialog should not show twisties for empty folders

Status: RESOLVED FIXED

(Core :: XUL, defect, P4)

Description

[Simon Fraser [no longer active]]

The tree in the Add Bookmarks dialog is confusing to use, for two reasons:
1. There is no 'root' folder, so I can't easily tell how to append the new
   bookmark at the end of the list.
2. Folders which contain only bookmarks, not other folders, still show twisties,
   and they should not (since opening them doesn't have any effect).

Comment 1

[timeless]

stephen: 2 should be easy, want to try?

Comment 2

[Andrew Thompson]

I almost never want to add something to the root folder, and I'd dislike having
to dig through an extra layer to get to the other folders every time, so if a
root folder is added, it should be open by default, and the top level of folders
inside that should be open as they are now (Single click to file into any of the
folders on my personal toolbar rocks).

With that much pre-expanded tree on view, I think the dialog will have to be
made larger... (um, seperate bug?)

Comment 3

[Simon Fraser [no longer active]]

The dialog is resizable, and should persist its size. I always want to add items 
to the root (because I'm lazy), and would like to see some kind of default 
selection in the tree view that shows this.

Comment 4

[[e-mail] (gone - use [e-mail] instead)]

No, I'm in "crunch time" with mail/news, I'd better stick to my regular tasks.

Comment 5

[James Paige]

I think the expandability-of-empty-folders issue is duplicate of WONTFIX bug 38728

But the need for a root-node bookmark folder is still valid (bugs really ought
not try to cover more than one different issue)

Comment 6

[Claudius Gayle]

this is not a dupe of 38728, it speaks to a more specific instance. Technically, these folders
aren't empty, they just don't contain any folders and for the purposes of this dialog, might
as well be. The twisties simply take up space and add to an alreeady confusing dialog.

Comment 7

[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 8

[Paul Chen]

Maybe fixed by outliner conversion? Marking mozilla0.9.8

Comment 9

[Paul Chen]

Assuming this gets fixed by outliner conversion, marking mozilla1.1

Comment 10

[Chris Lyon]

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

Comment 11

[Chris Lyon]

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

Comment 12

[Peter Trudelle]

mass reassign of pchen bookmark bugs to ben

Comment 13

[Chris Lyon]

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

Comment 14

[Peter Trudelle]

nsbeta1+ per Nav triage team.  Double-click shouldn't 'open' the leaf folders
either.  Should be okay to fix by displaying only the fully expanded tree, and
not allowing any collapsing.

Comment 15

[Jesse Ruderman]

I don't like trudelle's proposed solution, "displaying only the fully expanded
tree, and not allowing any collapsing".  That would be worse than this bug.

Comment 16

[Peter Trudelle]

Mass move of nsbeta1+ bugs that didn't have a valid MachV milestone to mozilla1.0.

Comment 17

[Blake Ross]

this is an rdfliner/template builder bug. -> waterson

Comment 18

[Brendan Eich [:brendan]]

And waterson's on sabbatical till April.  Who should get this, if it's to be
fixed by 1.0?

/be

Comment 19

[Blake Ross]

nsXULTemplateBuilder::IsContainerEmpty needs to check for the existence of an
iscontainer="true" rule and then ensure that any children are containers, I
think.  But nsXULTemplateBuilder is a bit too scary for me to try to delve into
right now.  Unfortunately, I don't know if anyone but waterson understands it all.

I hope waterson isn't getting too comfortable away from work, because we can't
let him quit. Ever.

Comment 20

[Chase Tingley]

Erk.

With my (severely limited) knowledge of this code, this seems like it might be
hard.  I'm still not sure if it's possible to make the change Blake suggests
without breaking another user of iscontainer that wouldn't want this behavior --
I'm not even sure if it's a distinction that can be made without extending the
syntax.   (There may be some way to do it with a non-simple rule.)

Plane tickets are cheap; maybe I'll fly to california and hang around on the
beach disguised as a surfer until Waterson shows up.

n.b. IsContainerEmpty is actually an nsXULOutlinerBuilder method.  cc'ing Jan,
who knows outliners.

Comment 21

[Robert John Churchill]

Check the XUL rules for the Mail window's folder pane. It has XUL <template>
rules  that do similiar things.  I'm recalling something like an  isEmpty="true"
 attribute.

Comment 22

[Blake Ross]

There is an isempty attribute. I thought of that but don't see how that will
help us here.

Comment 23

[Suresh Duddi (gone)]

Reassigning to rjc - the other owner of rdf. Blake take if you have this covered.

Comment 24

[Robert John Churchill]

Hmm... I believe that adding  flags="dont-build-content"  (which was added a
while ago) does the trick for <trees> now.  Marking WORKSFORME.

Comment 25

[Jan Varga [:janv]]

I'm not sure if this is really fixed, I'm still seeing this bug.
That's not complaint, just letting you know.

Comment 26

[Blake Ross]

This isn't fixed.

Comment 27

[Kyle Yuan]

Yes, the problem still exists, as well as in Address Book.

Taking and changing Component.

Comment 28

[Kyle Yuan]

Oops, forget to change the assignee.

Comment 29

[Kyle Yuan]

Attachment: proposed patch

The changes made in nsXULContentBuilder.cpp:
It will fix the problem in Address Book. (I believe that was a typo of
waterson.:)

The changes made in nsXULTreeBuilder.cpp:
It will fix other trees that have flags="dont-build-content" attribute.

Comment 30

[Kyle Yuan]

After discussion with Jan, I found I was misunderstanding the original problem. 
I'll make a new patch soon.

Comment 31

[Kyle Yuan]

Attachment: double check emptiness (raw patch, need work)

This is the safest way to solve the problem, but it may not the best. Still
looking for a better way.

My previous patch fixed such problem:
1) Open "Manage Bookmark" window;
2) Create a folder "A";
3) Create a folder "B" under "A", then "A" has a twisty now;
4) Delete "B", "A" still has a twisty.

Comment 32

[Kyle Yuan]

Attachment: Proposed patch: moved the checking work to nsXULTreeBuilder

Comment 33

[Kyle Yuan]

really want waterson to take a look.

Comment 34

[Chase Tingley]

This patch looks sort of like it's performing surgery with a sledge hammer.
If we have to re-run rule propagation every time we CheckContainer()
just to weed out false positives, it seems we're just masking whatever
is causing CheckContainer() to be untrustworthy -- like an update to
the datasource being missed somehow.

But then, I may just be misinterpreting what's going on here.  I'd
have to go through this with a debugger to know for sure.

Comment 35

[Kyle Yuan]

Chase, you are partly right, but I'm still trying to explain how can I do that.

Firstly, the <rule> of the <tree> in "File Bookmark" has an iscontainer="true" 
attribute. nsXULTemplateBuilder::CompileSimpleRule() is the only place that 
take care of this attribute, and it constructs a nsRDFConInstanceTestNode 
object that honored "iscontainer" and "isempty". 

Then, those two attributes are only used by 
nsRDFConInstanceTestNode::FilterInstantiations(). There are two functions 
called FilterInstantiations() -- TestNode::Propagate() and TestNode::Constrain
(). I have no idea about what Constrain() does, but maybe Propagate() is the 
right function we are looking for.

Therefore, in current code structure, as far as I know, RootNode::Propagate() 
is the only entrance to let iscontainer="true" attribute take effect.

Changing the current rule network algorithm goes beyond my knowledge of such 
code.

Comment 36

[Chase Tingley]

I looked at this some last night, and I think the problem may be that
nsRDFConInstanceTestNode::Retract() is stubbed out.  Unfortunately,
I haven't yet figured out how to write it correctly -- there's no
assertion of container emptiness in RDF that the XUL code can sniff;
instead it has to be inferred from other things.  I wonder if it's
possible to do this by catching a specific change to rdf:nextVal
in the CanPropogate() code, but that would also catch the case of
container initialization...  I'm going to try to hack together a
proof-of-concept of this approach tonight to see if I'm just deluded.

Comment 37

[Chase Tingley]

Update: I have code that will trap the emptying of a container in
Retract() code, but I'm doing something bad to the conflict set. I'm
going to keep investigating.

I'd be interested to hear what waterson had to say about the patch.

Comment 38

[Kyle Yuan]

=> Chase Tingley

Comment 39

[Chris Lyon]

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

Comment 40

[Chris Lyon]

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

Comment 41

[mmc]

what is the status of this bug, with the new bookmark implementation?

Comment 42

[Jan Varga [:janv]]

This is not bookmarks specific, although it is only visible in file bookmark
dialog at the moment.
It is general RDF tree builder problem.

Comment 43

[Steffen Wilberg]

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

Comment 44

[Asa Dotzler [:asa]]

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

Comment 45

[Bill Mason]

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

Comment 46

[Raanan Barzel]

Aside from identifying numerous duplicates - which indicates that the bug is
causing concern to many users - is there any plan to resolve this before 1.0? 

The current behavior of the Bookmarks tree may be considered as acceptable for
some, but it really gives the impression of an unfinished feature, certainly
inadequate for a 1.0 release.

Comment 47

[Ryan Polk (Quark)]

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

Comment 48

[Bill Mason]

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

Comment 49

[Jesiah S]

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

Comment 50

[Jan Varga [:janv]]

I think the last patch is correct, I'm using it and it works fine.
The additional code in the patch is actually copied from OpenSubtreeOf() method that creates children for a container. CheckContainer() (this method is currently used to test if a container is empty) only checks containment properties and if an *RDF* container is empty. The problem is that there may be rules that filter out all children of this RDF container.
The performance shouldn't be affected much, because the final result is cached in mContainerFill.

Neil, could you take a look?

Comment 51

[Jens Bannmann]

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

Comment 52

[Przemyslaw Bialik]

This bug looks fixed on trunk :)

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007081205 Minefield/3.0a8pre

Comment 53

[Steffen Wilberg]

Indeed, and I think bug 285631 fixed it.

Comment 54

[Steffen Wilberg]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/templates/src/nsXULTreeBuilder.cpp&rev=1.107&mark=563-570#553