Last Comment Bug 296040 - Cell-based selection in trees
: Cell-based selection in trees
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 All
: -- normal with 2 votes (vote)
: ---
Assigned To: Jan Varga [:janv]
:
Mentors:
Depends on: 340811 340867 343636 393711
Blocks:
  Show dependency treegraph
 
Reported: 2005-05-31 02:35 PDT by Brian King [:kinger]
Modified: 2008-07-31 03:18 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial patch (60.42 KB, patch)
2005-09-28 19:20 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
test case (4.16 KB, application/vnd.mozilla.xul+xml)
2005-09-28 19:21 PDT, Jan Varga [:janv]
no flags Details
patch for review (85.05 KB, patch)
2005-09-30 14:49 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
better patch (82.70 KB, patch)
2005-10-03 09:11 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v0.1 (93.84 KB, patch)
2005-10-11 11:32 PDT, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v0.2 (95.83 KB, patch)
2005-10-26 23:45 PDT, Jan Varga [:janv]
neil: review+
Details | Diff | Splinter Review
new approach (92.46 KB, patch)
2006-01-09 14:16 PST, Jan Varga [:janv]
no flags Details | Diff | Splinter Review
patch v0.3 (144.82 KB, patch)
2006-06-06 03:02 PDT, Jan Varga [:janv]
neil: superreview+
Details | Diff | Splinter Review
patch v0.4 (146.67 KB, patch)
2006-06-06 10:01 PDT, Jan Varga [:janv]
enndeakin: review+
jvarga: superreview+
Details | Diff | Splinter Review
Rough guide to selection styles (1.38 KB, image/gif)
2007-08-22 15:53 PDT, neil@parkwaycc.co.uk
no flags Details

Description Brian King [:kinger] 2005-05-31 02:35:47 PDT
One of our clients has requested the following, and we want to develop it on the
trunk and get it checked in. We feel it would be a useful addition to the tree
widget.

1) The ability to have a cell based selector in a tree, allowing us to navigate
by keyboard across a single row to each adjacent column.
2) The ability to attach an event to the tree (or a cell) that would allow us to
know which cell is currently selected when a key (such as enter) is pressed,
based on its row and column.
Comment 1 Jan Varga [:janv] 2005-09-28 19:20:39 PDT
Created attachment 197793 [details] [diff] [review]
Initial patch

Neil, could you take a look at this patch?
Thanks
Comment 2 Jan Varga [:janv] 2005-09-28 19:21:31 PDT
Created attachment 197794 [details]
test case
Comment 3 neil@parkwaycc.co.uk 2005-09-29 06:42:01 PDT
I had a quick look at the patch, and it seems to me that you made tree cells
keyboard selectable by default which you don't want in the folder pane.

I wonder whether it's worth optimizing invalidateSelection for cell mode.
Comment 4 Jan Varga [:janv] 2005-09-29 07:07:43 PDT
(In reply to comment #3)
> I had a quick look at the patch, and it seems to me that you made tree cells
> keyboard selectable by default which you don't want in the folder pane.
> 

yeah, good catch
we need a new attribute to enable/disable it, any ideas?

> I wonder whether it's worth optimizing invalidateSelection for cell mode.

I don't think it's worth at this point.

Thanks for initial review.
Comment 5 Neil Deakin (away until Oct 3) 2005-09-29 07:38:15 PDT
Jan, could you please specify the possible values for seltype and selstyle? Are
they meant to be combined into one? There are places
(classic/global/win/tree.css) where both are used and other places where only
one is used

Also <property name="cellmode"> should be readonly, and I think should be cellMode
Comment 6 Jan Varga [:janv] 2005-09-29 09:59:18 PDT
(In reply to comment #5)
> Jan, could you please specify the possible values for seltype and selstyle? Are
> they meant to be combined into one? There are places
> (classic/global/win/tree.css) where both are used and other places where only
> one is used

This is probably a cut and paste problem. I meant to remove selstyle completely.
seltype can have these values: single, cell or text.
I'll fix that.

> 
> Also <property name="cellmode"> should be readonly, and I think should be cellMode
> 

This is just a helper for internal code, we could rename it to _cellMode for now
Comment 7 Jan Varga [:janv] 2005-09-29 12:24:49 PDT
(In reply to comment #3)
> I had a quick look at the patch, and it seems to me that you made tree cells
> keyboard selectable by default which you don't want in the folder pane.
> 

There is selstyle="primary" in mailWidgets.xml (without seltype="single"), do
you think it's safe to convert it to seltype="text"?
Comment 8 neil@parkwaycc.co.uk 2005-09-29 13:39:50 PDT
(In reply to comment #7)
>There is selstyle="primary" in mailWidgets.xml (without seltype="single"), do
>you think it's safe to convert it to seltype="text"?
Yeah, the seltype="single" is enforced by the mail code
(the tree never gets focus, so its code does not apply).
Comment 9 Jan Varga [:janv] 2005-09-30 14:49:39 PDT
Created attachment 198052 [details] [diff] [review]
patch for review
Comment 10 Jan Varga [:janv] 2005-10-03 09:11:08 PDT
Created attachment 198320 [details] [diff] [review]
better patch

changes:
- removed nsITreeColumn.active and nsITreeColumns.getActiveColumn()
- added nsITreeSelection.currentColumn
Comment 11 neil@parkwaycc.co.uk 2005-10-07 12:52:46 PDT
Comment on attachment 198320 [details] [diff] [review]
better patch

>+NS_IMETHODIMP nsTreeSelection::SetCurrentColumn(nsITreeColumn* aCurrentColumn)
>+{
>+  if (mCurrentColumn == aCurrentColumn) {
>+    return NS_OK;
>+  }
>+  if (mCurrentIndex != -1 && mCurrentColumn)
>+    mTree->InvalidateCell(mCurrentIndex, mCurrentColumn);
>+  
>+  mCurrentColumn = aCurrentColumn;
>+  
>+  if (mCurrentIndex != -1 && mCurrentColumn)
>+    mTree->InvalidateCell(mCurrentIndex, mCurrentColumn);
>+
>+  return NS_OK;
>+}
Just skimming this, but note that we don't guarantee that the current column is
the selected column, for example when right-clicking a folder in the folder
pane.
Comment 12 neil@parkwaycc.co.uk 2005-10-07 12:55:51 PDT
(In reply to comment #11)
>Just skimming this, but note that we don't guarantee that the current column is
>the selected column, for example when right-clicking a folder in the folder pane.
Doh! I meant row, not column :-[
Comment 13 Jan Varga [:janv] 2005-10-07 13:08:44 PDT
yeah, I should call invalidateSelection() or an optimized version as you suggested
Comment 14 neil@parkwaycc.co.uk 2005-10-08 05:22:23 PDT
Comment on attachment 198320 [details] [diff] [review]
better patch

>+      <handler event="focus">
>+        <![CDATA[
>+          this.treeBoxObject.focused = true;
>+          if (this.currentIndex == -1 && this.view.rowCount > 0) {
>+            this.currentIndex = this.treeBoxObject.getFirstVisibleRow();
>+          }
We invented class="focusing" specially to avoid this. It really confuses the
thread pane.

>+           var currentColumn = this.view.selection.currentColumn;
>+           var primary = this.columns.getPrimaryColumn();
>+           if (currentColumn && currentColumn != primary)
if (currentColumn && !currentColumn.primary) should work, which should make it
possible to simplify the check.

>          var c = this.currentIndex;
>          if (c == -1 || c == 0)
>            return;
>+
>+         var cellSelType = this._cellSelType;
>+         if (cellSelType) {
>+           var column = this.view.selection.currentColumn;
>+           if (!column)
>+             return;
>+
>+           while (c > 0 && !this.view.isSelectable(c-1, column))
>+             c--;
>+           if (c == 0)
>+             return;
>+         }
>+
I think this would look neater if you subtracted 1 immediately (i.e. var c =
this.currentIndex = 1;) then optionally searched for a selectable cell (while
(c >= 0) of course).

>             if (c+1 == this.view.rowCount)
>               return;
>          } catch (e) {}
This try/catch is to work around badly written content views. You're now
calling more view methods, so more could break. (The only sure solution is to
move all this stuff into the C++, presumably somewhere in nsTreeSelection.cpp).

>       <!-- double-click -->
>       <handler event="click" clickcount="2">
>       <![CDATA[
>+        if (this.parentNode._cellSelType)
>+          return;
>+
>         var b = this.parentNode.treeBoxObject;
>         var row = b.view.selection.currentIndex;
>         if (row == -1 || !b.view.isContainer(row))
Why no double-clicks on primary cells?
Comment 15 Jan Varga [:janv] 2005-10-11 11:32:16 PDT
Created attachment 199181 [details] [diff] [review]
patch v0.1
Comment 16 Jan Varga [:janv] 2005-10-11 11:40:00 PDT
> We invented class="focusing" specially to avoid this. It really confuses the
> thread pane.

ok, I removed it from the xpfe version

> if (currentColumn && !currentColumn.primary) should work, which should make it
> possible to simplify the check.

fixed

> I think this would look neater if you subtracted 1 immediately (i.e. var c =
> this.currentIndex = 1;) then optionally searched for a selectable cell (while
> (c >= 0) of course).

fixed

> This try/catch is to work around badly written content views. You're now
> calling more view methods, so more could break. (The only sure solution is to
> move all this stuff into the C++, presumably somewhere in nsTreeSelection.cpp).

you mean the content view or views?

> Why no double-clicks on primary cells?
 
I removed the check

It's now possible to pass a column to invalidateRange()
Anyway, it's not used at the moment, because we support only single cell
selection. I just added this code to SetCurrentColumn()

if (mFirstRange)
  mTree->InvalidateCell(mFirstRange->mMin, mCurrentColumn);
Comment 17 neil@parkwaycc.co.uk 2005-10-12 05:13:27 PDT
(In reply to comment #16)
>>This try/catch is to work around badly written content views.
>you mean the content view or views?
Sorry, I mean web content applications which use custom tree views.

>>Why no double-clicks on primary cells?
>I removed the check
I'm fine with that.

>I just added this code to SetCurrentColumn()
A comment might be welcome :-)
Comment 18 neil@parkwaycc.co.uk 2005-10-12 05:23:12 PDT
Oh, and don't forget to bump the iid's on the changed interfaces.
Comment 19 Jan Varga [:janv] 2005-10-12 11:01:01 PDT
(In reply to comment #18)
> Oh, and don't forget to bump the iid's on the changed interfaces.

sure, r=neil with that?
Comment 20 neil@parkwaycc.co.uk 2005-10-18 05:40:08 PDT
Comment on attachment 199181 [details] [diff] [review]
patch v0.1

You removed all of the constructor, including the bit that sets the first
selected column, which I guess we do need...

I would still like to know your angle on remote xul pages with "badly" written
custom tree views (i.e. don't implement class info or security checked
component).
Comment 21 neil@parkwaycc.co.uk 2005-10-18 05:57:51 PDT
Oh, and for some reason I can't get any sort of selection to display in the
advanced message search/filter edit folder selector.
Comment 22 Jan Varga [:janv] 2005-10-25 10:57:35 PDT
ok, I bumped iid's on nsITreeBoxObject, nsITreeSelection and nsITreeView

>You removed all of the constructor, including the bit that sets the first
>selected column, which I guess we do need...

which bit, xpfe or toolkit?

>I would still like to know your angle on remote xul pages with "badly" written
>custom tree views (i.e. don't implement class info or security checked
>component).

Is there a bug for this, should we file a new one?

Comment 23 Jan Varga [:janv] 2005-10-25 13:48:22 PDT
(In reply to comment #21)
> Oh, and for some reason I can't get any sort of selection to display in the
> advanced message search/filter edit folder selector.

fixed in my tree
Comment 24 neil@parkwaycc.co.uk 2005-10-25 16:23:02 PDT
(In reply to comment #22)
>>You removed all of the constructor, including the bit that sets the first
>>selected column, which I guess we do need...
>which bit, xpfe or toolkit?
xpfe

>>I would still like to know your angle on remote xul pages with "badly" written
>>custom tree views (i.e. don't implement class info or security checked
>>component).
>Is there a bug for this, should we file a new one?
The vk_down handlers were originally checked in with try/catch on rowCount...
Comment 25 Jan Varga [:janv] 2005-10-26 11:58:25 PDT
> The vk_down handlers were originally checked in with try/catch on rowCount...

Neil, I just tried to load remote xul with custom tree view implementation. It doesn't work even w/o this patch (I can't get selection), there's "Permission denied to create wrapper for object class UnnamedClass" in JS console. Shouldn't this be discussed in a different bug?
Comment 26 neil@parkwaycc.co.uk 2005-10-26 15:34:55 PDT
OK, then I guess r=me on patch v0.2 as discussed...
as they don't work you might want to remove the existing try/catch blocks?
Comment 27 Jan Varga [:janv] 2005-10-26 23:45:31 PDT
Created attachment 200963 [details] [diff] [review]
patch v0.2
Comment 28 Boris Zbarsky [:bz] (TPAC) 2005-10-27 07:12:22 PDT
Comment on attachment 200963 [details] [diff] [review]
patch v0.2

I won't be able to get to this in any sort of reasonable timeframe.  Please try another sr?
Comment 29 neil@parkwaycc.co.uk 2005-10-30 09:05:08 PST
Comment on attachment 200963 [details] [diff] [review]
patch v0.2

>-          try {
>-            if (c+1 == this.view.rowCount)
>-              return;
>-         } catch (e) {}
>+         if (c+1 == this.view.rowCount)
>+           return;
Might as well fix this to c + 1 (both copies).
Comment 30 Jan Varga [:janv] 2005-11-13 10:51:21 PST
bryner, would you have time to sr it?
Comment 31 Brian Ryner (not reading) 2006-01-08 17:30:49 PST
Comment on attachment 200963 [details] [diff] [review]
patch v0.2

>--- layout/xul/base/src/tree/public/nsITreeBoxObject.idl	20 Sep 2005 09:30:32 -0000	1.35
>+++ layout/xul/base/src/tree/public/nsITreeBoxObject.idl	27 Oct 2005 06:39:31 -0000
>@@ -156,7 +156,7 @@
>   void invalidateColumn(in nsITreeColumn col);
>   void invalidateRow(in long index);
>   void invalidateCell(in long row, in nsITreeColumn col);
>-  void invalidateRange(in long startIndex, in long endIndex);
>+  void invalidateRange(in long startIndex, in long endIndex, in nsITreeColumn col);

We could preserve script compatibility, in many cases, by leaving the old version of this method (equivalent to col == null), and adding a new version that takes the column parameter (like invalidateColumnRange).

>--- layout/xul/base/src/tree/public/nsITreeView.idl	29 Oct 2004 06:29:49 -0000	1.25
>+++ layout/xul/base/src/tree/public/nsITreeView.idl	27 Oct 2005 06:39:31 -0000
>@@ -198,6 +198,13 @@
>   boolean isEditable(in long row, in nsITreeColumn col);
> 
>   /**
>+   * isSelectable is called to ask the view if the cell is selectable.
>+   * This method is only called if the selection style is |cell| or |text|.
>+   * XXXvarga shouldn't this be called isCellSelectable?
>+   */
>+  boolean isSelectable(in long row, in nsITreeColumn col);

Maybe instead of this, we could have a special property that's returned from getCellProperties to indicate that the cell is selectable?  That way we could avoid requiring any changes to existing TreeView implementations.

>--- layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp	26 Oct 2005 21:46:39 -0000	1.255
>+++ layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp	27 Oct 2005 06:39:32 -0000
>@@ -743,13 +743,19 @@
> }
> 
> NS_IMETHODIMP
>-nsTreeBodyFrame::InvalidateRange(PRInt32 aStart, PRInt32 aEnd)
>+nsTreeBodyFrame::InvalidateRange(PRInt32 aStart, PRInt32 aEnd, nsITreeColumn* aCol)
> {
>   if (mUpdateBatchNest)
>     return NS_OK;
> 
>-  if (aStart == aEnd)
>+  nsTreeColumn* col = NS_STATIC_CAST(nsTreeColumn*, aCol);

I think we really need to be doing better input validation for scriptable methods.  Javascript could crash this just by passing in a nsITreeColumn object of its own.  How about adding a private IID on nsTreeColumn so you can QI to it?

>@@ -1319,9 +1448,33 @@
>     return nsCSSAnonBoxes::moztreeimage;
>   }
> 
>-  // Just assume "text".
>-  // XXX For marquee selection, we'll have to make this more precise and do text measurement.
>-  return nsCSSAnonBoxes::moztreecelltext;
>+  currX += iconRect.width;
>+  remainingWidth -= iconRect.width;    

Can you explain the change to this method?  It's not really clear to me how it ties in with the other changes.

(more later)
Comment 32 Jan Varga [:janv] 2006-01-09 01:20:46 PST
(In reply to comment #31)
> We could preserve script compatibility, in many cases, by leaving the old
> version of this method (equivalent to col == null), and adding a new version
> that takes the column parameter (like invalidateColumnRange).

> Maybe instead of this, we could have a special property that's returned from
> getCellProperties to indicate that the cell is selectable?  That way we could
> avoid requiring any changes to existing TreeView implementations.

Ok, I'll try to rework the patch to address these comments
 
> I think we really need to be doing better input validation for scriptable
> methods.  Javascript could crash this just by passing in a nsITreeColumn object
> of its own.  How about adding a private IID on nsTreeColumn so you can QI to
> it?

There's already a patch for this in bug 309429.

 
> >@@ -1319,9 +1448,33 @@
> >     return nsCSSAnonBoxes::moztreeimage;
> >   }
> > 
> >-  // Just assume "text".
> >-  // XXX For marquee selection, we'll have to make this more precise and do text measurement.
> >-  return nsCSSAnonBoxes::moztreecelltext;
> >+  currX += iconRect.width;
> >+  remainingWidth -= iconRect.width;    
> 
> Can you explain the change to this method?  It's not really clear to me how it
> ties in with the other changes.
 
This change fixes GetCellAt(..., out childElt) to return "text" in childElt for the real text area only (the text is now correctly measured in this method, it was only done in PaintText() before).
The old implementation returns "text" for entire text rect which is a problem when you click on it in "text selection mode". It would be confusing for the user.
Comment 33 neil@parkwaycc.co.uk 2006-01-09 11:57:05 PST
(In reply to comment #31)
>Maybe instead of this, we could have a special property that's returned from
>getCellProperties to indicate that the cell is selectable?  That way we could
>avoid requiring any changes to existing TreeView implementations.
Note that properties aren't available to remote XUL.
Comment 34 Jan Varga [:janv] 2006-01-09 14:16:05 PST
Created attachment 208018 [details] [diff] [review]
new approach

Ok, I didn't remove nsITreeView::IsSelectable() yet
There's a new method nsITreeBoxObject::IsCellSelectable() that calls GetCellProperties() and stuff. nsTreeContentView has been updated to append nsXULAtoms::selectable to the property array if the cell is selectable.
toolkit/tree.xml has been updated to call tree.isCellSelectable() instead of view.isSelectable()
Let me know if you like this approach and I'll finish it off.
Thanks
Comment 35 Jan Varga [:janv] 2006-06-05 08:17:10 PDT
Neil Deakin and Neil Rashbrook, what do you think, should we add a new method IsSelectable() or use GetCellProperties() to return the "selectable" atom. The problem with the latter is that we already use IsEditable() and stuff for other purposes, so it would be quite inconsistent.

I'd like to check in this patch ASAP.
Comment 36 neil@parkwaycc.co.uk 2006-06-05 08:29:59 PDT
My preference would be for IsSelectable().
Comment 37 Neil Deakin (away until Oct 3) 2006-06-05 09:25:03 PDT
I'd prefer IsSelectable as well
Comment 38 Jan Varga [:janv] 2006-06-06 03:02:49 PDT
Created attachment 224531 [details] [diff] [review]
patch v0.3
Comment 39 neil@parkwaycc.co.uk 2006-06-06 04:56:13 PDT
Comment on attachment 224531 [details] [diff] [review]
patch v0.3

>+		       selectable="false"/>
No tabs please...

I assume the following issue applies to all themes:

>+tree[seltype="cell"] > treechildren::-moz-tree-cell {
>+  border: 1px solid transparent;
>+  padding: 0px 1px 1px 1px;
>+}
The "text" selection padding keeps the text away from the border. I'm don't think that "cell" padding has the same issue; given that the cell default is 0px 2px 0px 2px then maybe 0px 1px 0px 1px would best allow for the border.

I assume the following nits apply to both versions:

>+              return seltype;
>+            else
Nit: else after return

+                 col.element.getAttribute("selectable") == "false" ||
Shouldn't you create a new property col.selectable for these?

>+         else if (!cellSelType) // Ctrl+Up moves the anchor without selecting
Unnecessary; this case already only happens for a multiselect tree.
Comment 40 Jan Varga [:janv] 2006-06-06 05:59:31 PDT
Neil R, I addressed all your comments except the theme issue, I don't understand what's the problem. Could you explain it in more details? Thanks
Comment 41 neil@parkwaycc.co.uk 2006-06-06 06:12:12 PDT
Comment on attachment 224531 [details] [diff] [review]
patch v0.3

The existing primary selection padding is specific to selected cell text. I don't think it makes sense to apply the same padding when selecting an entire cell. That's why I think the bottom padding for selected cells should be zero.

>+          var col = this.view.selection.currentColumn;
>+          if (col) {
>+            col = left ? col.getPrevious() : col.getNext();
>+          }
>+          else {
>+            col = this.columns.getFirstColumn();
>+          }
It just occurred to me that getKeyColumn might make more sense here.
Comment 42 Neil Deakin (away until Oct 3) 2006-06-06 09:58:49 PDT
Hmmm. I had just finished reviewing this.

>+NS_IMETHODIMP
>+nsTreeBodyFrame::InvalidateColumnRange(PRInt32 aStart, PRInt32 aEnd, nsITreeColumn* aCol)
>+{
>+  if (mUpdateBatchNest)
>+    return NS_OK;
>+
>+  nsTreeColumn* col = NS_STATIC_CAST(nsTreeColumn*, aCol);

Is there a bug on fixing up these direct casts?


>+  if (col) {
>+    if (aStart == aEnd)
>+      return InvalidateCell(aStart, col);
>+
>+    PRInt32 last = GetLastVisibleRow();
>+    if (aEnd < mTopRowIndex || aStart > last)
>+      return NS_OK;
>+
>+    if (aStart < mTopRowIndex)
>+      aStart = mTopRowIndex;
>+
>+    if (aEnd > last)
>+      aEnd = last;

What happens if aStart is greater than aEnd? Just extra painting?


>+NS_IMETHODIMP nsTreeSelection::SetCurrentColumn(nsITreeColumn* aCurrentColumn)
>+{
>+  if (mCurrentColumn == aCurrentColumn) {
>+    return NS_OK;
>+  }
>+
>+  if (mCurrentColumn) {
>+    if (mFirstRange)
>+      mTree->InvalidateCell(mFirstRange->mMin, mCurrentColumn);
>+    if (mCurrentIndex != -1)
>+      mTree->InvalidateCell(mCurrentIndex, mCurrentColumn);
>+  }
>+  

Why is the old selected column being repainted here? Won't the repainting code still think the column is selected?


>       <handler event="keypress" keycode="vk_left">
>         <![CDATA[
>-         if (!this.changeOpenState(this.currentIndex, false)) {
>-            var parentIndex = this.view.getParentIndex(this.currentIndex);
>-           if (parentIndex >= 0) {
>-             this.view.selection.select(parentIndex);
>-             this.treeBoxObject.ensureRowIsVisible(parentIndex);
>+         var row = this.currentIndex;
>+         if (row < 0)
>+           return;
>+
>+         var cellSelType = this._cellSelType;
>+         var checkContainers = true;
>+
>+         var currentColumn;
>+         if (cellSelType) {
>+           currentColumn = this.view.selection.currentColumn;
>+           if (currentColumn && !currentColumn.primary)
>+             checkContainers = false;
>+         }
>+
>+         if (checkContainers) {
>+           if (this.changeOpenState(this.currentIndex, false))
>+             return;

I think pressing left and right in cell selection mode should always move the current cell rather than open containers. Cells can be opened with Enter, and we should have a bug on making + and - do this as well.


>+         if (cellSelType) {
>+           var col = this._getNextColumn(row, true);
>+           if (col) {
>+             this.view.selection.currentColumn = col;
>+             this.treeBoxObject.ensureCellIsVisible(row, col);

Having a selectCell function would be useful.
Comment 43 Jan Varga [:janv] 2006-06-06 10:01:50 PDT
Created attachment 224577 [details] [diff] [review]
patch v0.4
Comment 44 Neil Deakin (away until Oct 3) 2006-06-06 10:25:56 PDT
Comment on attachment 224577 [details] [diff] [review]
patch v0.4

One issue I saw after testing was that an overflowing cell was cut off when selected. But, otherwise, it looks good.
Comment 45 Jan Varga [:janv] 2006-06-06 11:13:21 PDT
(In reply to comment #42)
> Is there a bug on fixing up these direct casts?

yes, bug 309429

> What happens if aStart is greater than aEnd? Just extra painting?

Ah, this was copied from invalidateRange()
The final rect for invalidation will be invalid.

There should be a check:
if (aStart > aEnd)
  return NS_OK;

I'll add it to both methods

> Why is the old selected column being repainted here? Won't the repainting code
> still think the column is selected?

In theory, there may be 2 cells (selected and current).
Anyway, we support only single selection in cell mode.

> I think pressing left and right in cell selection mode should always move the
> current cell rather than open containers. Cells can be opened with Enter, and
> we should have a bug on making + and - do this as well.

Well, I did it this way originally, but the customer wanted to change it. Personally, I like both aproaches.

> Having a selectCell function would be useful.

in the XBL binding?
Comment 46 Jan Varga [:janv] 2006-06-06 11:19:45 PDT
> > Having a selectCell function would be useful.
> 
> in the XBL binding?

hmm, we should fire the select event, did you mean that?
Comment 47 Neil Deakin (away until Oct 3) 2006-06-06 12:19:36 PDT
(In reply to comment #46)
> > > Having a selectCell function would be useful.
> > 
> > in the XBL binding?
> 
> hmm, we should fire the select event, did you mean that?
> 

It should fire the select event when a cell is changed, yes, but I just meant that a selectCell function would be logical so that a specific cell can be selected.
Comment 48 Jan Varga [:janv] 2006-06-06 13:42:59 PDT
Neil D, it seems it's not so trivial as I thought. e.g. view.selection.currentIndex doesn't fire select events.

I checked it in as it is for now.

Thanks
Comment 49 neil@parkwaycc.co.uk 2006-06-06 17:04:37 PDT
I think Enn was just looking for a single method that sets the selected column and row at the same time, rather than setting currentColumn and calling select(row) separately.

I agree that changing the currentColumn should fire a select event. But what should it do for row-select-style trees?

Given your (temporary, I hope!) confusion over the currentIndex property perhaps you should change the name of currentColumn to selectedColumn?
Comment 50 Wayne Mery (:wsmwk, NI for questions) 2006-11-07 15:27:00 PST
are the regressions on radar?  esp bug 338401
Comment 51 Eric Shepherd [:sheppy] 2007-08-22 15:41:18 PDT
Question: what does "text" do?  I can't figure it out from the discussion here.
Comment 52 neil@parkwaycc.co.uk 2007-08-22 15:53:51 PDT
Created attachment 277798 [details]
Rough guide to selection styles
Comment 53 Eric Shepherd [:sheppy] 2007-08-22 23:02:06 PDT
Excellent; thank you.  Is "text" a new selection type in Fx3?  It appears to be based on what I'm seeing in the docs, but I'd like to be sure before I tag it as such.
Comment 54 neil@parkwaycc.co.uk 2007-08-23 05:04:52 PDT
(In reply to comment #53)
>Is "text" a new selection type in Fx3?
Both "text" and "cell" are new selection types. The nearest equivalent that SeaMonkey 1.x supports is selstyle="primary" which is only a visual effect equivalent to seltype="cell" applied to the column with primary="true" whereas the new selection types apply to the currently selected column (new property).
Comment 55 Sergey Zh 2007-11-05 12:43:47 PST
> Anyway, we support only single selection in cell mode.

Why ?? Multiple selection is a basic user interaction feature. That is the default in row selection.

Could you add an option for multiple cell selection ?
Comment 56 Sergey Zh 2007-11-07 17:07:28 PST
Filed bug about seltype="cell": https://bugzilla.mozilla.org/show_bug.cgi?id=402958

Note You need to log in before you can comment on or make changes to this bug.