waterson checked in new RDF outliner tree widget ( bug 71530 ) need to convert bookmark to use the new outlier.

I'm currently working on getting a test/demo of outliner bookmarks up and running. I had a discussion with waterson this evening, and here are some of the issues we identified and some proposed solutions: Issues: - The Outliner Builder is a generic class that is not tied to any particular RDF datasource. As such, it leaves stubbed certain nsIOutlinerView methods that are of use in some RDF outliner situations, like APIs relating to inline editing, cell cycling and so on. Solutions: - The Outliner Builder is being modified to accept observers (|nsIXULOutlinerBuilderObserver|) which are notified of various events that occur. Clients can then implement these observers to handle such events. Provision has been made for the attachment of multiple observers, despite potentially tricky semantics with some APIs like IsEditable and SetCellText. A single observer claiming support for Inline Editing will be enough to produce a text field, and the changed value will then be handed to any and all observers that claim to support it. That way, a name change can be propagated not only to the local bookmark store, but perhaps to an online public bookmark repository if a third party plugin for such a system is installed. Other Issues: - Inline editing will need to be implemented in the Outliner's XBL binding. - Drag and Drop feedback needs to be provided (pinkerton) - Separators need to be provided (hyatt)

OK, with this initial patch, the outliner builder should now theoretically support n nsIXULOutlinerBuilderObservers. I've provided methods on the observer interface to the methods that clients of the outliner builder are likely to want to handle. Please let me know if there are others. To test: apply the patch in content/xul/templates and in xpfe/components/bookmarks/resources. Make chrome in xpfe/components/bookmarks/resources, load the browser and type: chrome://communicator/content/bookmarks/oTest.xul in the location field. A XUL document with a bookmarks list should display. Outliner bookmarks! If you expand or collapse a row, you should see a message printed to the console. This is a method implemented as a js object defined in the XUL file which is attached to the outliner builder as an observer. As the demo is currently pretty simple, this is the only notification you can easily see. Nevertheless, it's the first step to supporting other things!

The first patch (to nsXULOutlinerBuilder.cpp, 04/04/01 05:31) looks wonderful. Two nits: - In AddObserver(), use NS_NewISupportsArray() instead of thunking through the component manager. (We link against XPCOM, so no need to componentize here.) Also, make sure that you actually *got one back*, and return NS_ERROR_OUT_OF_MEMORY if you didn't. - In DocumentWillBeDestroyed(), just call Clear() on mObservers, instead of removing each element one-by-one. Do that, and r/sr=waterson for the first patch, whichever helps. cc'ing alecf: looks like ben is taking a first crack at doing bookmarks...

+ // Called when an item is opened or closed. + void onToggleOpenState (in long index); You should use /** * Doc comments */ here, so that they propagate to headers and can show up in doxygen some day. + // Called when selection in the outliner changes + void onSelectionChanged(); Is it worth aping (or even inheriting from) the existing selection listeners, such as passing in the current selection state? Seems like an easy optimization opportunity. Also, could these notifications not take an nsIXULOutlinerBuilder argument, so that objects could observe multiple outliners? It's not too onerous to whip up some placeholders/multiplexers in JS, but the C++ people will suffer. + void onPerformAction(in wstring action); (Our commands are Unicode? Wow.) + /** + * The builder observer. + */ + nsCOMPtr<nsISupportsArray> mObservers; Comment is singular, but variable type and naming indicates plurality. One must be wrong! +nsXULOutlinerBuilder::AddObserver(nsIXULOutlinerBuilderObserver* aObserver) +{ + nsresult rv = NS_OK; + if (!mObservers) + rv = NS_NewISupportsArray(getter_AddRefs(mObservers)); + You don't check the result, though you bother to store it. Also, there's no point in initializing rv to NS_OK, when you can never read it without setting first. + if (mObservers && mObservers->IndexOf(aObserver) == -1) + rv = mObservers->AppendElement(aObserver); ...then you check if the preceding call worked! How about propagating rv from NS_NewISupportsArray, and then losing the mObservers check there? Also, the IndexOf check indicates that this AddObserver(foo); AddObserver(foo); RemoveObserver(foo); will result in foo not being registered. That might be very surprising to one of the code paths that registered foo! At the least, document this behaviour. Other than that, very nice. You write pretty code; seems a shame to waste it in content/xul. =)

This patch checked has been checked in. Thanks, waterson & shaver! Now, to try and get some functionality up.

nav triage: we will not do the conversion to rdfliner in this release cycle. moving to future.

just a note as i'm disappointed to see this bug is moved to "future"... right now bookmarks are too slow. i hoped this wonderful "outliner" thing i saw in mail/news could benefit to the bookmarks. are you sure this is not urgent ?

Ben created a mini branch for this and plans on starting work on it soon; retargetting to 0.9.2 so this shows up on his list.

I consider this one VERY important. I cannot effectively use Mozilla with my very large bookmarks file.

nav triage: we're not going to be able to do this for mozilla0.9.2. In fact we do not want to do this for m0.9.2 because we do not wish to destabilize the existing bookmarks at this point. moving to mozilla1.0 at least.

Moving this forward to mozilla0.9.5. Ben is working on this on a branch, we hope to land this in mozilla0.9.5.

I hope this is landed soon I am getting *numerous* complaints based on this.

I guess mozilla 0.9.5 isn't realistic for this one considering that bug 75572 (which this one depends on) is targeted at mozilla 1.0? Please feel free to prove me wrong:-)

The "manage bookmarks" is my daily most problematic part of mozilla. I was just able to produce (by drag-dropping bookmarks around) something in my bookmarks which crashes mozilla while I open a bookmarks folder). Drag-and-drop often stops working, etc... Things are also slow even on fastest machines. This needs to be done ASAP so it can be debugged well before 1.0. Adding nsCatFood.

Should we push this to 0.9.6 and try to land it there? Ben, are you actively working on this?

0.9.5 is out the door. bumping TM up by one.

Paul Chen has volunteered to take this ;)

And now, because everyone gave everything to Paul, we're load balancing again. Over to blake...

bookmarksliner is landing in --> 0.9.7.

Okay, marking this fixed. There are still some issues. I know. Really. Most of them are not bugs in bookmarksliner code. Don't file bugs just yet.

This patch toasts the triple license in bookmarks.js.

Yeah, that's because Ben was working on this on a really old branch. I'll fix it tomorrow.

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31. if you think this particular bug is not fixed, please make sure of the following before reopening: a. retest with a *recent* trunk build. b. query bugzilla to see if there's an existing, open bug (new, reopened, assigned) that covers your issue. c. if this does need to be reopened, make sure there are specific steps to reproduce (unless already provided and up-to-date). thanks! [set your search string in mail to "AmbassadorKoshNaranek" to filter out these messages.]