Places tree behavior

[alta88[nntp]]

i'm reusing the places tree.xml and treeView.js (and other places infrastructure re sidebar) for a custom tree (snowl project).  however, i'm implementing multiselection in the tree, and there are some oddities.

1) places sends out select events (ie click on a tags rows in the Organizer) which all places trees (in organizer and in sidebar) will receive.  but i don't want the custom tree to receive such an event, how best to prevent that?  it seems in many but not all cases this isn't necessary even for Places purposes, and perhaps is better implemented via observer, ie more specifically to an interested consumer and less 'blanket'.

2) focus and select events cause trees (places doesn't have multiselection) to clear multiselection.  it seems a check is made for selectedNode, and if none then currentIndex is set to -1 etc.  but for multiselection, there will not ever be a selectedNode, though there may be several rows selected, thus incorrectly clearing the tree.  it seems it would be more correct to test for tree.selection.count or check getRangeCount.

any thoughts or advice?  the latter is close to a bug that just hasn't been hit yet as at some point perhaps Places will use multiselection in trees etc.

[Drew Willcoxon]

> 1) places sends out select events (ie click on a tags rows in the
> Organizer) which all places trees (in organizer and in sidebar) will
> receive. but i don't want the custom tree to receive such an event, how

I'm not sure what you mean here. I'm not super familiar with the code
yet, but it looks like the tree view (tree.xml) only calls
document.commandDispatcher.updateCommands("focus") on focus and select.
It sounds like you've been looking at the code, so if that's not what
you're referring to, I'd be happy to take a look at specific spots if
you could point them out.

> 2) focus and select events cause trees (places doesn't have
> multiselection) to clear multiselection. it seems a check is made for

Single-selection isn't built into the tree view. It's just that the
sidebar's use of the view is single-select. In fact the content tree in
right pane of the Organizer is multiselect. And I'm not sure how focus
and select events would cause the tree view to lose selection. Again,
if you could point out areas of the code you're running into, that'd be
great.

Drew

> _______________________________________________
> dev-apps-firefox mailing list
> dev-apps...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-apps-firefox

[alta88[nntp]]

---On 2009.Feb.10 02:47 PM, Drew Willcoxon wrote:
> > 1) places sends out select events (ie click on a tags rows in the
> > Organizer) which all places trees (in organizer and in sidebar) will
> > receive. but i don't want the custom tree to receive such an event, how
>
> I'm not sure what you mean here. I'm not super familiar with the code
> yet, but it looks like the tree view (tree.xml) only calls
> document.commandDispatcher.updateCommands("focus") on focus and
> select. It sounds like you've been looking at the code, so if that's
> not what you're referring to, I'd be happy to take a look at specific
> spots if you could point them out.

yes, the events in tree.xml aren't the cause of the problem. the thing
i can consistently reproduce (after, as you can guess, some pain trying
to figure out why multiselections were getting randomly cleared) is that
if you open Organizer, expand tags, click a tag row, i believe
_fireOnSelect() will send out a 'select' event, if you read the method
in the #listbox binding using DOMi. you can see the content tree tag
column updating with the selected/deselected tag. now, if my tree is
open in sidebar, and i have one single row selected, nothing happens.
but if >2 are selected, then clicking a tags row will deselect, as i
described. not sure the code path, but the select event triggers
something that clears multiselect, then my onSelect handler runs and
then the commandDispather.

and there of course must be something else firing an event without any
user input, which also clears the tree in the same manner, but i don't
know what triggers it.

>
> > 2) focus and select events cause trees (places doesn't have
> > multiselection) to clear multiselection. it seems a check is made for
>
> Single-selection isn't built into the tree view. It's just that the
> sidebar's use of the view is single-select. In fact the content tree
> in right pane of the Organizer is multiselect. And I'm not sure how
> focus and select events would cause the tree view to lose selection.
> Again, if you could point out areas of the code you're running into,
> that'd be great.

you will notice, if you bookmark multiple pages with the same tag, then
multiselect the bookmarks in content list, then click a tag row, the
multiselection will also clear in Organizer! i hadn't tried this
before; it's now a clear bug imo.. and i'm pretty sure it has to do
with selectedNode handling. but the question is why is this triggered
without Organizer even open etc.

[Drew Willcoxon]

> you open Organizer, expand tags, click a tag row, i believe
> _fireOnSelect() will send out a 'select' event, if you read the method
> in the #listbox binding using DOMi. you can see the content tree tag

The trees in the organizer are just that, trees, not listboxes, so I'm
not sure where you're seeing that binding and function call.

> you will notice, if you bookmark multiple pages with the same tag, then
> multiselect the bookmarks in content list, then click a tag row, the
> multiselection will also clear in Organizer! i hadn't tried this before;

Hmm, this doesn't happen to me.

The only practical reason I can think for action in one Places tree to
affect the selection in another (especially in a different window!) is
this: Places views are backed by history results. History results have
their own bookmark observers so that when something in the bookmarks
changes, the results can notify their views, which can then update their
UIs so that they show the changed data. For example, if two views show
the same bookmark, and you modify that bookmark via one view (e.g., by
renaming the bookmark in the organizer), then the other view (e.g., in a
sidebar) will be updated to display the new name. Maybe this update
causes the tree view to lose its multiselection, if there's a bug somewhere.

I don't see how simply selecting a tag container would cause any of that
to happen, though. Unless the favicons of the bookmarks that appear the
right pane are updated soon thereafter -- I don't know the code well
enough to know whether they are. In that case, if your custom view is
displaying the same bookmarks, maybe the multiselection could be lost.
Is your view displaying some of the same bookmarks that appear in a tag
container you select?

Drew

[alta88[nntp]]

---On 2009.Feb.11 11:58 AM, Drew Willcoxon wrote:

> you open Organizer, expand tags, click a tag row, i believe
> _fireOnSelect() will send out a 'select' event, if you read the method
> in the #listbox binding using DOMi. you can see the content tree tag

The trees in the organizer are just that, trees, not listboxes, so I'm not sure where you're seeing that binding and function call.

listbox id=editBMPanel_tagsSelector.  use the DOMi xbl bindings menuitem to see methods on that element...

> you will notice, if you bookmark multiple pages with the same tag, then
> multiselect the bookmarks in content list, then click a tag row, the
> multiselection will also clear in Organizer! i hadn't tried this before;

Hmm, this doesn't happen to me.

are you sure you have the tags listbox expanded, ie list of tags with checkboxes next to each tag?  the action is checking/unchecking the tag checkbox.

The only practical reason I can think for action in one Places tree to affect the selection in another (especially in a different window!) is this:  Places views are backed by history results.  History results have their own bookmark observers so that when something in the bookmarks changes, the results can notify their views, which can then update their UIs so that they show the changed data.  For example, if two views show the same bookmark, and you modify that bookmark via one view (e.g., by renaming the bookmark in the organizer), then the other view (e.g., in a sidebar) will be updated to display the new name.  Maybe this update causes the tree view to lose its multiselection, if there's a bug somewhere.

yes this is surely the effect, i'm interested in the specific code path that sends the event, as this update definitely causes the multiselection loss.

I don't see how simply selecting a tag container would cause any of that to happen, though.  Unless the favicons of the bookmarks that appear the right pane are updated soon thereafter -- I don't know the code well enough to know whether they are.  In that case, if your custom view is displaying the same bookmarks, maybe the multiselection could be lost. Is your view displaying some of the same bookmarks that appear in a tag container you select?

well it sure enough happens that selecting a tag checkbox is one way of clearing the multiselection in both the content list as well as my custom tree.  my tree does not display any bookmarks per se, they are of type bookmark but have their own query uri.

thanks for your help btw.

Drew

On 2/10/09 2:25 PM, alta88[nntp] wrote:

[Drew Willcoxon]

> are you sure you have the tags listbox expanded, ie list of tags with
> checkboxes next to each tag? the action is checking/unchecking the tag
> checkbox.

Oh. You were talking about the tags listbox in the right pane. OK. I
thought you meant the Tags folder in the left pane. Now this makes more
sense.

This is definitely a problem, so thanks for finding it. I filed a bug
at https://bugzilla.mozilla.org/show_bug.cgi?id=478110. The selection
restore code just doesn't work in this specific case.

As a workaround, you might be able to make up for it by adding an
onselect handler to the tree so you can remember the selection each time
it changes, registering a bookmark observer to listen for bookmark
changes, and then in the observer manually restoring the selection. Or
much better, if you're using PlacesTreeView, just implement the
selection restore code correctly yourself. And when you're done,
contribute the fix at the link above. :)

Drew

[alta88[nntp]]

---On 2009.Feb.11 04:30 PM, Drew Willcoxon wrote:
> > are you sure you have the tags listbox expanded, ie list of tags with
> > checkboxes next to each tag? the action is checking/unchecking the tag
> > checkbox.
>
> Oh. You were talking about the tags listbox in the right pane. OK.
> I thought you meant the Tags folder in the left pane. Now this makes
> more sense.
>
> This is definitely a problem, so thanks for finding it. I filed a bug
> at https://bugzilla.mozilla.org/show_bug.cgi?id=478110. The selection
> restore code just doesn't work in this specific case.

thanks.

>
> As a workaround, you might be able to make up for it by adding an
> onselect handler to the tree so you can remember the selection each
> time it changes, registering a bookmark observer to listen for
> bookmark changes, and then in the observer manually restoring the
> selection.

well, that's a bit hacky for my taste.. i persist selected itemIds
anyway, so could easily do this, but it would then force a totally
unnecessary db call. anyway, i don't think creating a 'select' event is
the right design to notify other views of a change; it casts too wide a
net. an interested consumer should register an explicit observer for a
particular event. i do realize, however, that Places was created for
its own purposes but perhaps it would be nice to consider reuse.

> Or much better, if you're using PlacesTreeView, just implement the
> selection restore code correctly yourself. And when you're done,
> contribute the fix at the link above. :)

i'm not sure what that code is trying to accomplish, unless there is an
insert/delete type row change, there's no need to mess with selections
at all. set em and forget em, why is it even running?

[Drew Willcoxon]

> well, that's a bit hacky for my taste..

Yeah, no doubt. It's a hack to get around broken code.

> unnecessary db call. anyway, i don't think creating a 'select' event is
> the right design to notify other views of a change; it casts too wide a

It's not a select event that's happening. It's not that high up, not in
the DOM. It's a consequence of the view's relationship with the
underlying data.

> i'm not sure what that code is trying to accomplish, unless there is an
> insert/delete type row change, there's no need to mess with selections
> at all. set em and forget em, why is it even running?

That's exactly what's going on. The view is being refreshed after some
general modification to the data underneath. During the refresh the
selection has to be manually remembered and restored, and the restore
code is broken in this particular path.

Maybe it won't assuage your concerns, but you might read up on the view
system if you haven't already:

https://developer.mozilla.org/en/Querying_Places
https://developer.mozilla.org/en/Displaying_Places_information_using_views

If you have specific ideas about this bug, please lodge them at the
bugzilla link. Of course you're welcome to keep asking here if you have
more general questions.

Drew