321384 - New views need to support multiple selection ('Select All' is broken)

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

The new views only support having one event selected at a time.  Since the unifinder allows for several to be selected, the views need to be able to handle this as well.  Also, until this is fixed 'Select All' is broken.

Comment 1

[Joey Minta]

Attachment: proposed interface/manager

This is a proposed interface and manager.  I have NOT tested it to see if it is actually compiles/runs, so there's quite likely minor bugs in it.  I'll do that when I start implementing the observers after everyone seems reasonably happy with this model.  The general idea is that this model is intended to be much more intelligent (and requires its observers to be more intelligent) than the current Sunbird model, which simply calls onSelectionChanged for any type of change.  For tree/list views, this new model will greatly improve performance.  For the views, I think there will be little net gain or loss.  For comparison, see http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendarSelection.js

Feedback welcome/encouraged.

Comment 2

[Joey Minta]

Some thoughts from discussing this on IRC (put here so I don't forget):
-the selection manager needs to observe onModifyItem, so it doesn't hold stale references to items
-break up addItem to multiple functions with clearer meanings
-use arrays more often instead of batch-mode

Comment 3

[gekacheka]

(In reply to comment #1)
> The general idea is that this model is intended to be much
> more intelligent (and requires its observers to be more intelligent) than the
> current Sunbird model, which simply calls onSelectionChanged for any type of
> change.
Can you be more specific?  'intelligent' is vague.

> For tree/list views, this new model will greatly improve performance. 
Can you be more specific, and explain which poorly performing mechanisms the new design improves?

I suggest retaining the convenience methods, though they could be renamed
- clearSelection()
- isSelected(item)         
- addItems(array)
- selectItem(item)
- selectItems(array)

(current the two branches of addItem don't share any code, so there doesn't
seem to be savings in merging them with the obscure 'push' parameter; two separate methods addItem and selectItem will be clearer to read.)


I see some potential performance issues with the implementation:

It's an O(n^2) operation to add a set of items to the selection.
The problem is it is iterating through the list to add each item, and
iterating through the already selected items to check for duplicates.
So for example selectAll of 1000 items requires n*(n-1) comparisons or 999000 comparisons.   In the selectAll case it looks like it is unnecessary; we already know all the items are distinct.  Maybe:
 * retain a method like 
   - selectDistinctItems(array)
 * implement isSelectedItem more efficiently, such as with hashing or sorting.
   (at least insert a comment that isSelected(item) needs to be reimplemented
    for efficiency if you don't want to do it for the first version.)

Extending a selection seems more expensive than necessary, since it unselects each item and then reselects it.  While in theory each observer could merge the removes and adds into the minimal set of changes for a batch (is that what 'intelligent' refers to?), it seems easier to implement the merge once rather than duplicate it in each observer. 


Have you thought about how recurring items fit with this?  Are the selected items individual items or the master recurring item?

Comment 4

[Joey Minta]

(In reply to comment #3)
> Can you be more specific?  'intelligent' is vague.
The idea here is to avoid continually passing the complete list of selected items.  'Intelligent' means that the observers need to be able to handle the fact that in many cases they will only be sent a list of the items whose selection changed, rather than a complete list of selected items.

> 
> > For tree/list views, this new model will greatly improve performance. 
> Can you be more specific, and explain which poorly performing mechanisms the
> new design improves?
Looking at the unifinder, we see that onSelectionChanged calls selectSelectedEventsInTree.  For each event in the selection, we have to find its row at unifinder.js#117, which requires iterating through the entire tree.  (The net result is also an n^2 operation.)  The advantage here is in cases where we want to add/subtract to that selection, usually via Ctrl+click. These additional operations under the old view are still n^2, because the entire selection is rebuilt, but under the new code, they are constant (c=1).

> I see some potential performance issues with the implementation:
> 
> It's an O(n^2) operation to add a set of items to the selection.
> The problem is it is iterating through the list to add each item, and
> iterating through the already selected items to check for duplicates.
> So for example selectAll of 1000 items requires n*(n-1) comparisons or 999000
> comparisons.   In the selectAll case it looks like it is unnecessary; we
> already know all the items are distinct.  
This is exactly the intended use of the aPush argument (although I agree with everyone that it should be broken out into a separate function.)  I'm struggling to find a standard-user action that would result in multiple items being sent with aPush == true.  SelectAll and shift+click seem to invariably be cases where aPush would be false, and hence would be dealt with by the other branch, which is linear.  All of the cases of aPush == true that I can imagine only would have 1 item as an argument.

> Maybe:
>  * retain a method like 
>    - selectDistinctItems(array)
>  * implement isSelectedItem more efficiently, such as with hashing or sorting.
>    (at least insert a comment that isSelected(item) needs to be reimplemented
>     for efficiency if you don't want to do it for the first version.)
In one attempt that I made (never submitted) to improve unifinder performance, I did indeed use hashing of event-ids to allow for use of the indexOf operator, which could improve performance somewhat.  I do think that true performance optimization ought to be left to later, as long as we have usable performance now, and the ability to optimize without a complete re-write.

> 
> Extending a selection seems more expensive than necessary, since it unselects
> each item and then reselects it.
Extending a selection does not do this (or at least it shouldn't).  That is the purpose of aPush.  That branch of code merely calls onItemAdded.  Only replacing the selection unselects all old items and then selects all new items.

> 
> Have you thought about how recurring items fit with this?  Are the selected
> items individual items or the master recurring item?
> 
That is why my comparisons also examine the recurrence id.  The goal is to allow individual occurrences to be added to the selection so that the 'delete' and 'edit' can be able to respond to the new functionality of modifying occurrences that is being worked on.

Comment 5

[Michael Büttner (no reviews TFN)]

first of all i truly believe that this approach will suffer from the overall architecture, where clients need to clone() items in order to modify them. in that case clients need to be aware of the existance of the selection manager and manually delete/reinsert the modifed item. this problem would be solved if we decide to implement the proposed new architecture, see http://wiki.mozilla.org/Calendar:Architecture for further details on this subject.

the second issue that i see worth noting is the fact, that this is the classical decision between an array which keeps references to the selected items and a ringbuffer where the items keep references to their siblings. assume you have a new interface (IPickable) that is injected into the calIItemBase-hierarchy (through inheritance, aggregation or containment) which keeps the references (next/prev) to its immediate sibling elements, and a selection manager (the picklist) which only contains the references to head/tail of the selection. this way insert/remove/query would be an O(1) operation. items simply know their state (next/prev is null -> not selected, not null -> selected). of course you need a sentinel entry to make this work, but that's an implementation detail. such an approach would be a) faster b) more object oriented.

and i would be interested in the answer to the question why so much code in the calender needs to be implemented in javascript. i can't see any advantages to writing so much stuff in a scripting language, you lose everything a compiler has to offer and sacrifice speed. of course i could understand if the existing codebase has evolved that way, but why is it necessary to follow that path for new stuff? wouldn't it make more sense to write much more in C++?

one last thing worth nothing is the question: "is it really necessary to being able to pick more than one item at a time?". let's say you have more than one recurring event, pick several occurrences from different series and drag'n'drop them to another location or edit them in any other way. what should happen? should we really pop up several dialogs each of them asking "all or this one?" where the user doesn't know to which item each of those dialogs belongs to?

that's it from me...

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #5)
> first of all i truly believe that this approach will suffer from the overall
> architecture, where clients need to clone() items in order to modify them.

I agree that this is getting messy here. But on the other side, we can't keep rewriting the core arch. At some point, we just have to live what we have. (except for small, gradual improvements ofcourse)


About IPickable: This mixes the backend with the frontend. being selected is really a front-end only thing. The backend should care about that. Speed improvements can be made in other places. For example, do we really need to check for one item being selected twice? What will go wrong if one item is selected twice, and how likely is it to happen?

About using JS: This discussion shouldn't be done here in a bug about something else.
In short, the reason is that developing in JS is easier in a lot of places and thus faster. You can do more in less lines of code (no more worry about memory allocation etc.) If there are performance problems, critical parts of cade can be switched to C++ later. If you want to discuss this more, the newsgroup mozilla.dev.apps.calendar sounds like a perfect place.

Comment 7

[Michael Büttner (no reviews TFN)]

(In reply to comment #6)

> I agree that this is getting messy here. But on the other side, we can't keep
> rewriting the core arch. At some point, we just have to live what we have.
> (except for small, gradual improvements ofcourse)

since there are so many features missing i don't believe it's too late to think about the existing system. we're still far from having a fully functional product and in order to get there in the minimum amount of time and effort we should carefully think if it's worth keeping a flakey backend. it's not only getting messy, but it will be a real pain in the ass to keep the clone()ing.

> About IPickable: This mixes the backend with the frontend. being selected is
> really a front-end only thing. The backend should care about that. Speed
> improvements can be made in other places. For example, do we really need to
> check for one item being selected twice? What will go wrong if one item is
> selected twice, and how likely is it to happen?

why is it mixing backend and frontend? we don't need to inherit calIItemBase from IPickable but can use aggregation, thus eliminating the need to force each different type of item to implement the pickable functionality. and to be honest, 'to be pickable' is an aspect of an object, thus it's naturally to being able to QueryInterface() for this functionality. an object needs to know about its state, otherwise other higher-level systems needs to worry about specific functionailities (such as items being pickable) and violating object oriented contraints.

Comment 8

[Joey Minta]

Attachment: proposed interface/manager v2

This patch is updated to include some of the comments from the prevoius discussion, which I very much appreciate.

Changes include:
-Removal of batch mode.  Instead, methods are now provided for replacing the selection with an arbitrary array of items, and observers are now notified with arrays, rather than individual items.
-Introduction of observers to cover the case where items within the array can change.
-Pushing the burden for ensuring that an item is not added twice on to the consumers.  This ought to reduce all of the functions to linear complexity.
-Introduction of an isSelected function.  This will help consumers ensure that no duplicates are added, as well as being generally useful.

About iPickable: As I understand this solution, it would inherently depend on having the Calendar:Architecture changes in place.  Since there are different objects corresponding to the same event, I don't see a way to make them all aware of their next/prev elements in the ring.  Given that we're trying to get 0.3a2 out the door asap after lightning 0.1, I'm inclined to not want to wait on something that substantial (both in discussion and hacking time).

Comment 9

[gekacheka]

o typos: 
  * "duplicate entries may result in expected behavior" ->
    "duplicate entries may result in unexpected behavior" 
  * "return this.mSelectionItems" ->
    "return this.mSelectedItems" 

o onModifyItem
  * Does onDeleteItem handle cases where the untilDate or repeatCount
    were reduced?  Maybe onModifyItem should clear the selection.
    (the consumer should clone the array before starting modifications.)
  * (Is selecting all occurrences and dragging them, expected to modify 
    the start date, or just the occurrences?  It seems like
    the occurrence dates might be modified twice, once because of the
    startDate change and once because of the drag.)

o Needs composite onCalendarAdded/onCalendarRemoved listener:
  Looks like it handles deleting a calendar (via
  onCalendarRegistered/onCalendarUnregistered
  events from calendar manager) but not hiding a calendar
  (onCalendarAdded/onCalendarRemoved events from calComponsiteCalendar).
  I think unchecking the checkbox to hide the calendar
  should also remove any of its items from the selection, so
  deleting or otherwise operating on the selection set never
  operates on any hidden items.

o Suggest splitting removeItem into two methods, removeItem(item) and
  removeItemAndOccurrences(item) [they can share a private implementation].  

  removeItem currently has a boolean parameter aRemoveOccurrences.
  It cannot be omitted when called from non-javascript callers, can it?
  (that's why this is a service, right?)

  It seems like removeItem without occurrences will be used exclusively
  in UI selection code, and removeItemAndOccurrences will only
  be used for special case internal code such as listening for deletions.
  This code's own delete listener uses a separate method and does not call
  removeItem.

  Are there other scenarios where aRemoveOccurrences is useful?  The
  UI selection code should only be able to remove what it was able to
  add.  But there is no corresponding "addOccurrences" parameter to
  addItem and replaceSelection, so it can't add occurrences by adding
  a master.
 
  If not split, need to document how the following methods should
  behave with regard to occurrences (given master, do they add
  occurences or not):
    addItem
    replaceSelection
    isSelected

o Suggest removeItemWithOccurrences may return a boolean saying whether
  it was found, rather than throwing an error.  (Otherwise have to call
  isSelected first, which doubles the time, or catch errors.  This
  code's own delete listener does not throw an error.)

o Request selectItem(item), clearSelection():
  The interface is a little hard to figure out how to use because there 
  are no methods for the most common operations: clicking a single item, 
  and clearing the selection.  Would like to see convenience methods for
  these common operations; these will also make callers' code easier
  to read.
  ...= function selectItem(anItem) { this.replaceSelection([anItem], 1); }
  ...= function clearSelection()   { this.replaceSelection([], 0);       }

o As I understand, the expected method calls for common operations
  might be something like below.  Is this as you expected?

  var selMan = .../*selection manager*/
  var allMyItems = .../*array of items in order*/
  var prevFocusItem = null;

  function click(anItem) {
    if (anItem) { 
      selMan.selectItem(anItem);    /*see above*/
    } else {
      selMan.clearSelection();    /* see above */
    } 
    prevFocusItem = anItem; /* used for shift-click */
  }

  function controlClick(anItem) {
    if (anItem) { 
      if (selMan.isSelected(anItem)) {
	selMan.removeItem(anItem, false); /* ignore occurences parameter */
      } else {
	selMan.addItem(anItem);
      }
      previousFocusItem = anItem; /* used for shift-click */
    }
  }

  function shiftClick(anItem) { 
    if (anItem) { 
      var prevFocusIndex = Math.max(0, allMyItems.indexOf(prevFocusItem));
      var clickIndex = allMyItems.indexOf(anItem);
      if (clickIndex >= 0) {
	var allSelectedItems = [];
	var loIndex = Math.min(prevFocusIndex, clickIndex);
	var hiIndex = Math.max(prevFocusIndex, clickIndex);
	for (var i = loIndex; i <= hiIndex; i++) {
	  allSelectedItems.push(allMyItems[i]);
	}
	// removes all previously selected items, then adds back current set.
	selMan.replaceSelection(allSelectedItems, allSelectedItems.length);
      }
    }
  }


[o (minor) suggest addItems, removeItems:
  The above shiftClick method may sometimes be inefficient,
  since a shift-click that adjusts the size of the selection removes
  all items then adds them all back again.  (For example if you select
  999 items, then adjust selection to select 1000 items, it removes
  all 999 items, then adds them back again plus the one additional
  item.  This could cause many unnecessary redraws in a view with
  many events.)

  Since the design "push[es] the burden ... on the consumers",
  a client could try to work around this by implementing the
  set difference, so it only removes items that are no longer
  selected, and adds items that are selected.  

  However, there are no methods to pass an array of items.
  If removeItem was called for each item, then each of the listeners
  will be called for each item, rather than once for the whole
  array of removed items.   Similarly for addItem. 

  Suggest adding addItems(array) and removeItems(array) methods.

  removeItems(array) could be useful when a calendar is removed or hidden.

  They might be used something like the following
  (shiftClick that only removes/adds items whose selection changed):

  function shiftClick2(anItem) { 
    if (anItem) { 
      var prevFocusIndex = Math.max(0, allMyItems.indexOf(prevFocusItem));
      var clickIndex = allMyItems.indexOf(anItem);
      if (clickIndex >= 0) {
	var allSelectedItems = [];
	var loIndex = Math.min(prevFocusIndex, clickIndex);
	var hiIndex = Math.max(prevFocusIndex, clickIndex);
	for (var i = loIndex; i <= hiIndex; i++) {
	  allSelectedItems.push(allMyItems[i]);
	}
	// remove previously selected items not in current set;
	// Use hashsets to implement set difference.
        // Must use sets because there may be items previously selected via
	// control-click inside or outside the new range.
	function makeKey(item) {
	  var id = item.id;
	  if (item.recurrenceId) { id = id + "/" + recurrenceId.nativeTime; }
          return id;
        }

	var oldArray = selMan.selectedItems;
        var oldSet = {};
        for each (var item in oldArray) {
	  oldSet[makeKey(item)] = true;
        }
	var additions = [], removals = [];
        var newSet = {};
        for each (var item in allSelectedItems) {
	  var key = makeKey(item);
	  newSet[key] = true;
	  if (!(key in oldSet)) {
	    additions.push(item);
          }
        }
	for each (var item in oldArray) {
	  var key = makeKey(item);
	  if (!(key in newSet)) {
            removals.push(item);
          }
        }
	selMan.removeItems(removeArray);
	selMan.addItems(addArray);
      }
    }
  }
]  

Comment 10

[Joey Minta]

Attachment: implement selectionManager

This patch sets up the necessary backend for a full-fledged selection manager.  It also does the basics of wiring it into the views.  In order to avoid conflicts, I've ripped out the old selection manager that sunbird was using and also replaced those calls with their appropriate calISelectionManager equivalents.

This patch therefore fixes the following:
-Sunbird can end up editing stale (old copies) of events, because the selection manager does not purge them during onmodifyitem
-Lightning does not preserve event selection when switching views

In order to try to keep the size of this patch to a minimum, I've decided not to actually implement multiple selection in the views at this time.  Follow-up patches will be submitted that fix this, as well as:
-Wiring selection manager into agenda-tree
-Wiring selection manager into task-lists
-Improving selection efficiency in the unifinder

Note about this patch: While updating the various calls to the old selection system, I found that focusFirstItemIfNoSelection is never called.  This is confirmed by the fact that it had glaring bugs written into the function that would have thrown an error immediately if it ever had been called.  Accordingly, I've removed it.

Comment 11

[Joey Minta]

Comment on attachment 214181 [details] [diff] [review]
implement selectionManager

bah... bugzilla patch viewer makes it much easier to see errors in code.  Updated version coming soon.

Comment 12

[Joey Minta]

Attachment: updated patch

Patch updated to fix some minor bugs discovered after further testing.  Same description as before applies.

A small note on one of the changes in unifinder.js:
-   /* This needs to be in a setTimeout */
-   setTimeout( "resetAllowSelection()", 1 );
+   resetAllowSelection();

With the new selection manager, this needs to NOT be in a timeout, because the selection manager is too fast.  Changes made via onItemsAdded will not be picked up because the timeout from onItemsRemoved hasn't finished.  There was no indication in either of the bugs cited by CVS blame as to why it needed to be in a timeout, but testing seemed to indicate no undesirable behavior with the change.

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 216015 [details] [diff] [review]
updated patch

>Index: calendar/base/content/calendar-month-view.xml
>@@ -588,24 +601,26 @@
>           if (this.mSelectedItem) {
>-              var newboxes = this.findBoxesForItem(this.mSelectedItem);
>               for each (newbox in newboxes) {
>                   newbox.box.selectItem(this.mSelectedItem);
>               }

Ehm, i don't think that will work. You need to assign newboxes before using it.

>Index: calendar/base/content/calendar-multiday-view.xml
>@@ -1493,16 +1494,20 @@
>         window.addEventListener("resize", this.mResizeHandler, true);
> 	this.reorient();
Bonus points for fixing that indent :)

>Index: calendar/base/public/Makefile.in
> 		  calIRecurrenceRule.idl \
>+          calISelectionManager.idl \
And that indent. (makefiles tend to use tabs)


Now for the bigger problems:
With this patch, I still can't ctrl-click to select multiple items. Or isn't that in this patch yet?

And I've been wondering if we really need this to be an xpcom interface. Are we ever going to call this from c++ code? Maybe a normal JS file would be enough to give the needed functionality.

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 216015 [details] [diff] [review]
updated patch

>-            <property name="selectedItem">
>-                <getter><![CDATA[
>-                    return document.getAnonymousElementByAttribute(this, "anonid", "view-element").selectedItem;
>-                ]]></getter>
>-            </property>
>-

I was going to ask:

  Do we want a selectedItems property here, which hides the calselmgr machinations?

but it's been replaced by a bigger question, see below.

>+               //XXX we currently only support selecting one item
>+               this.calView.mSelectedItem = aItems[0];
>                var newboxes = this.calView.findBoxesForItem(this.calView.mSelectedItem);
>                for each (newbox in newboxes) {
>                    newbox.box.selectItem(this.calView.mSelectedItem);
>                }
>+          },

Prefer FIXME with bug numbers to XXX.

>         window.addEventListener("resize", this.mResizeHandler, true);
> 	this.reorient();
>+
>+        var calSelMgr = Components.classes["@mozilla.org/calendar/selection-manager;1"]
>+                                  .getService(Components.interfaces.calISelectionManager);
>+        calSelMgr.addObserver(this.mSelectionObserver);

Reindent that .reorient call while you're in there?

>+/**
>+ * calISelectionManager is a service meant for storing a globally accessible
>+ * list of the selected items.  This enables all visual representations of 
>+ * calendar items to remain in sync with their selection.  Global commands,
>+ * such as those in toolbars or menubars, can likewise access this list to act
>+ * on items in a consistent manner.
>+ */

What does this mean for having multiple calendars open/visible at a time?  Does the
selection manager's state reflect the selected items for the selected calendar, or the
union of the selected items in all displayed calendars, or?  Some of the comments and fields in calISelectionManager seem to imply that there's context to the selection, but it's not
really clear to me what it's supposed to be.

Having different calendar views in tb2 tabs, or even top-level windows in Sb and Ltn alike seems like something that we don't want to fence off here.

Fear the ghost of gCalendarWindow!

>+    var calSelMgr = Components.classes["@mozilla.org/calendar/selection-manager;1"]
>+                              .getService(Components.interfaces.calISelectionManager);
>     // if( !calendarEventArray)
>-    var calendarEventArray = gCalendarWindow.EventSelection.selectedEvents;
>-
>+    var calendarEventArray = calSelMgr.getSelectedItems({});

See, this feels to me a lot like it should be

var calendarEventArray = someCalendar.selectedItems;

or perhaps 

var calendarEventArray = someView.selectedItems;

I dunno if this is useful so long after the discussion seems to have died down, but I'm going to minus on the basis of mvl's newbox catch at least.

Comment 15

[Joey Minta]

Attachment: no xpcom (work in progress)

This is a work in progress that I'm putting here, since I almost lost this patch once already.  It has all of the architecture changes needed to support mulitple selection in the views.

-The views become the authoritative source of selected items.
-People interested in syncing selection should listen for the "itemselect" select event.
-The views are responsible for ensuring that their selected items are up to date (to avoid stale references).

TODO:
-actually sync selection when switching views.
-Ctrl+click adds to selection.
-Clean-up

Comment 16

[Joey Minta]

Attachment: multiple selection

This patch gets our selection into decent shape.  As I said before, the views become the authoritative source of selected items.  Therefore, as long as all of the places that are interested in the selection have access to content, we don't need the xpcom service I previously attempted.

Comment 17

[Joey Minta]

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

Comment 18

[Joey Minta]

Comment on attachment 233799 [details] [diff] [review]
multiple selection

Incorporate the architecture description in comment #15 into the IDL.  Also note that it's possible for there to be no selected events.  A note about what aSuppressEvent is for would also help.

+    var oldSelection = [];
This is probably better named "currentSelection"

File a follow-up for ensuring that clicking in a blank space in a view unselects all selected events.

+          this.mSelectedItems = aItems || [];
Include a comment about what's going on here.

         // still select it (since we'll stopPropagation())
-        evbox.calendarView.selectedItem = evbox.mOccurrence;
+        evbox.calendarView.setSelectedItems(1, [this.mOccurrence]);
This looks like it's going to ignore ctrl+click

       <!-- methods -->
+      <field name="mCurrentSelection">[]</field>
Let's call this mSelectedChunks

-  attribute calIItemBase selectedItem;
+  readonly attribute calIItemBase selectedItems;
+
That's cheating.  While returning an array to js will work ok, in c++ it won't.  We need a getSelectedItems() method.

r2=dmose with that

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 233799 [details] [diff] [review]
multiple selection

I won't have time to review this any time soon.
I took a very quick look at the patch, and didn't saw anything obviously wrong. So i'll just trust dmose's review.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: multi-select patch, v2

New version of the patch merged to the CVS trunk, with most, but not all of the review comments addressed.  I'll finishing addressing and testing in the morning before landing.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: multi-select patch, v3

Patch with all comments incorporated.  Unfortunately, it still has the issue that somehow getSelectedItems seems to get called without a parameter, as though XPConnect were somehow eating the parameter out of the arg list.  The calls in question are in calendar.js and unifinder.js.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: multi-select patch, v4

BL woes have been worked around.  Still one possible semantic kink with the unifinder to be worked out.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Checked in a version with a few more cleanups.  Filed a spin-off bug 351747 related to a workaround that I had to add.  Also filed bug 351760 about the unifinder selection semantics being crazy.  However, the semantics apparently match the 0.2 semantics, and 0.2 parity is what this bug was all about.

Comment 24

[Sebastian Schwieger]

(In reply to comment #23)
> Checked in a version with a few more cleanups.  Filed a spin-off bug 351747
> related to a workaround that I had to add.
This is a typo: bug 351757 is the correct one.

Comment 25

[Sebastian Schwieger]

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

Comment 26

[Damian Szczepanik]

Litmus testcase 2602 created

Comment 27

[Damian Szczepanik]

Litmus testcase 2603 created

Comment 28

[Miguel]

Verified.