Bookmarks tree widgets need to be converted to rdfliner

VERIFIED FIXED in mozilla0.9.7

Status

SeaMonkey
Bookmarks & History
P4
normal
VERIFIED FIXED
18 years ago
14 years ago

People

(Reporter: Cathleen, Assigned: Blake Ross)

Tracking

({perf})

Trunk
mozilla0.9.7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [br][nav+perf])

Attachments

(7 attachments)

(Reporter)

Description

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

Updated

18 years ago
Blocks: 73948
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)



Created attachment 29693 [details] [diff] [review]
initial (rough) patch to get this going.
Created attachment 29694 [details] [diff] [review]
patch for xpfe/components/bookmarks, includes a demo XUL file
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

18 years ago
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...
Created attachment 29782 [details] [diff] [review]
updated patch with requested changes
Created attachment 29783 [details] [diff] [review]
how about a patch that actually compiles.
Created attachment 29784 [details] [diff] [review]
another patch that checks for mObservers before trying to remove an observer from it.
+  // 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. =)


Created attachment 29791 [details] [diff] [review]
once more, for the interested, addressing shaver's comments.
This patch checked has been checked in. Thanks, waterson & shaver! Now, to try 
and get some functionality up. 

Status: NEW → ASSIGNED
(Reporter)

Updated

18 years ago
Depends on: 75404
(Reporter)

Updated

17 years ago
Depends on: 75572
Keywords: nsbeta1
Summary: bookmark needs to use new RDF outliner → Bookmarks tree widgets need to be converted to rdfliner
Target Milestone: --- → mozilla0.9.2

Updated

17 years ago
Keywords: perf
nav triage: we will not do the conversion to rdfliner in this release cycle. 
moving to future. 
Keywords: nsbeta1 → nsbeta1-
Target Milestone: mozilla0.9.2 → Future
(Assignee)

Updated

17 years ago
Whiteboard: [br]

Comment 13

17 years ago
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 ?
(Assignee)

Comment 14

17 years ago
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.
Target Milestone: Future → mozilla0.9.2
(Reporter)

Updated

17 years ago
Blocks: 52144
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. 
Priority: -- → P4
Target Milestone: mozilla0.9.2 → mozilla1.0
(Assignee)

Updated

17 years ago
Whiteboard: [br] → [br][nav+perf]
(Reporter)

Updated

17 years ago
Blocks: 91351
Moving this forward to mozilla0.9.5. Ben is working on this on a branch, we hope 
to land this in mozilla0.9.5. 
Target Milestone: mozilla1.0 → mozilla0.9.5

Updated

17 years ago
Blocks: 92860
(Reporter)

Updated

17 years ago
Blocks: 52149

Updated

17 years ago
No longer blocks: 92860

Comment 18

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

Comment 19

17 years ago
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

17 years ago
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.
Keywords: nsCatFood
Should we push this to 0.9.6 and try to land it there?  Ben, are you actively
working on this?

Updated

17 years ago
Keywords: mozilla0.9.6

Comment 22

17 years ago
0.9.5 is out the door. bumping TM up by one.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Paul Chen has volunteered to take this ;) 
Assignee: ben → pchen
Status: ASSIGNED → NEW
And now, because everyone gave everything to Paul, we're load balancing again. 
Over to blake...
Assignee: pchen → blakeross
(Assignee)

Updated

17 years ago
Blocks: 65957
(Assignee)

Updated

17 years ago
Blocks: 70347
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

17 years ago
Blocks: 83901
(Assignee)

Updated

17 years ago
Blocks: 90829
(Assignee)

Comment 25

17 years ago
bookmarksliner is landing in --> 0.9.7.
Target Milestone: mozilla0.9.6 → mozilla0.9.7

Updated

17 years ago
Blocks: 93693

Updated

17 years ago
Blocks: 93920

Updated

17 years ago
Blocks: 77496

Updated

17 years ago
Blocks: 52298

Updated

17 years ago
Blocks: 107591
(Assignee)

Comment 26

17 years ago
Created attachment 60383 [details] [diff] [review]
patch
(Assignee)

Updated

17 years ago
No longer blocks: 107591
(Assignee)

Comment 27

17 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 28

17 years ago
This patch toasts the triple license in bookmarks.js.
(Assignee)

Comment 29

17 years ago
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.]
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.