73508 - Bookmarks tree widgets need to be converted to rdfliner

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P4)

Description

[Cathleen]

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

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

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)

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: initial (rough) patch to get this going.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch for xpfe/components/bookmarks, includes a demo XUL file

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

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!

Comment 5

[Chris Waterson]

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...

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: updated patch with requested changes

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: how about a patch that actually compiles.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: another patch that checks for mObservers before trying to remove an observer from it.

Comment 9

[Mike Shaver (:shaver emeritus)]

+  // 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. =)

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: once more, for the interested, addressing shaver's comments.

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 12

[Viswanath Ramachandran]

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

Comment 13

[Hervé Renault]

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 ?

Comment 14

[Blake Ross]

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.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 16

[Viswanath Ramachandran]

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. 

Comment 17

[Viswanath Ramachandran]

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

Comment 18

[Keyser Sose]

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

Comment 19

[André Dahlqvist]

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:-)

Comment 20

[Marko Macek]

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.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 22

[Asa Dotzler [:asa]]

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

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

Paul Chen has volunteered to take this ;) 

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 25

[Blake Ross]

bookmarksliner is landing in --> 0.9.7.

Comment 26

[Blake Ross]

Attachment: patch

Comment 27

[Blake Ross]

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.

Comment 28

[:deanis74]

This patch toasts the triple license in bookmarks.js.

Comment 29

[Blake Ross]

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

Comment 30

[sairuh (rarely reading bugmail)]

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.]