Closed Bug 306522 Opened 19 years ago Closed 19 years ago

richlistbox needs to use template listeners

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8beta4

People

(Reporter: doronr, Assigned: doronr)

References

Details

(Keywords: fixed1.8)

Attachments

(3 files, 4 obsolete files)

puts back the last-selected setting code into richlistbox, 1.8 only still
21.01 KB, patch
asaf
: first-review+
Details | Diff | Splinter Review
patch (1.8)
21.90 KB, patch
asa
: approval1.8b4+
Details | Diff | Splinter Review
trunk patch
16.43 KB, patch
Details | Diff | Splinter Review
Blocks: 306299
If we want to ship at least a semi-stable widget, I believe it should be done by
b4, see bug 306299.
Flags: blocking1.8b4?
Attached patch 1.8 branch patch (obsolete) — — Splinter Review 
This is 1.8 only - won't apply on trunk due to aaronl's other patch.

This seems to fix all the issues I've seen reported.
Attachment #194437 - Flags: first-review?(bugs.mano)
Asaf, when requesting approval to be a beta stopper, it would be helpful if you
could right a summary about why you are nominating it, why we need the fix. That
helps the bug traige team out a lot when we go through a bunch of bugs. Thanks!



        Attachment #194437 -
        Attachment is obsolete: true

        Attachment #194441 -
        Flags: first-review?(bugs.mano)

    
    

    
  
Comment on attachment 194441 [details] [diff] [review]
with irc comments addressed (still 1.8 only)

Haven't tested this yet; drive-by comments:


>       <constructor>
>         <![CDATA[
>           var x = document.getAnonymousElementByAttribute(this, "anonid", "main-box");
>           this.scrollBoxObject = x.boxObject.QueryInterface(Components.interfaces.nsIScrollBoxObject);
>+
>+          // add a template build listener
>+          this.builder.addListener(this._builder());
>         ]]>
>       </constructor>

What about plain-richlistbox with persist="last-selected" set?

>+      <method name="_builder">
>+      <body>
>+        <![CDATA[
>+          var myThis = this;
>+          return {
>+            item: null,
>+            willRebuild : function(builder) {},
>+            didRebuild : function(builder) {
>+              myThis._refreshSelection()
>+            }
>+          }
>+        ]]>
>+      </body>
>+      </method>

This should be implemented as a <filed>; also, a better name would be
_builderListener.

>+
>+      <method name="_refreshSelection">
>+      <body>
>+        <![CDATA[
>+          // fist look for a last-selected attribute
>+          var lastSelected = this.getAttribute("last-selected");
>+          if (lastSelected != "") {
>+            var element = document.getElementById(lastSelected);
>+
>+            if (element)
>+              this.selectedItem = element;
>+
>+            if (this.selectedItem)
>+              this.scrollBoxObject.scrollToElement(this.selectedItem);
>+
>+            return;
>+          }
>+

how about:
	  if (lastSelected != "") {
	    var element = document.getElementById(lastSelected);
	    if (element) {
	      this.selectedItem = element;
	      this.scrollBoxObject.scrollToElement(this.selectedItem);
	      return;
	    }
	  }

I don't think you want to return if you haven't changed selectedItem.

>+          if (this.selectedItem) {
>+            if (this.selectedItem.hasAttribute("id")) {
>+              var id = this.selectedItem.getAttribute("id");
>+              var item = document.getElementById(id);
>+  
>+              // if we find no item, clear selection so that the code at the bottom
>+              // takes over
>+              if (item)
>+                this.selectedItem = item;
>+              else
>+                this.clearSelection();
>+             } else {
>+               // if no id, we clear selection so that the below code will select
>+               // based on the current index
>+               this.clearSelection();
>+             }
>+          }

ah? aren't you doing too much work here? if the selected item was removed, you
should be able to test if this.selectedItem belongs to any document... instead
of getting it from the current document via its id.


>Index: toolkit/mozapps/downloads/content/downloads.js
>===================================================================

>@@ -416,18 +426,22 @@
...
>+
>   gDownloadViewController.onCommandUpdate();
>+
>+  // retry always places the item in the first spot
>+  gDownloadsView.selectedIndex = 0;
>+focus();

focus()?



>Index: toolkit/mozapps/extensions/content/extensions.js
>===================================================================

>   
>   // Initialize the richlistbox.  This will select the last selected extension
>   // or default to the first listed one.
>-  gExtensionsView.init();
>+  //gExtensionsView.init();

remove the commented line.

        Attachment #194441 -
        Flags: first-review?(bugs.mano) → first-review-

    
  

        Attachment #194437 -
        Flags: first-review?(bugs.mano)

    
    

    
  
(In reply to comment #3)
> Asaf, when requesting approval to be a beta stopper, it would be helpful if you
> could right a summary about why you are nominating it, why we need the fix. That
> helps the bug traige team out a lot when we go through a bunch of bugs. Thanks!

FWIW, raeasoning here was/is the new borken richlistbox API which leads to bugs
like 306299.

    
    

    
  
Comment on attachment 194441 [details] [diff] [review]
with irc comments addressed (still 1.8 only)

Also, can you move the last-selected implementation bits from extensions.js to
richlistbox?

    
    

    
  
> ah? aren't you doing too much work here? if the selected item was removed, you
> should be able to test if this.selectedItem belongs to any document... instead
> of getting it from the current document via its id.
> 

_refreshSelection is always called when we know we need to reset selection. 
Either on creation of the widget or during template rebuilds.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        with comments addressed (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #194441 -
        Attachment is obsolete: true

        Attachment #194538 -
        Flags: first-review?(bugs.mano)

    
    

    
  
Comment on attachment 194538 [details] [diff] [review]
with comments addressed

+      <field name="_builderListener">
>+        <![CDATA[
>+          ({
>+            mOuter: this,
>+            item: null,
>+            willRebuild : function(builder) {},
>+            didRebuild : function(builder) {
>+              this.mOuter._refreshSelection()

missing semicolon here.

>+            }
>+          })

another one


>+      <method name="_refreshSelection">
>+      <body>
>+        <![CDATA[
....
>+          // happens.
>+          if (this.selectedItem) {
>+            if (this.selectedItem.hasAttribute("id")) {
>+              var id = this.selectedItem.getAttribute("id");
>+              var item = document.getElementById(id);
>+  

Please add a comment here explaining that the orginal item is gone regardlees.


>+          // if we have no previously selected item or the above if check fails to
>+          // find the previous node (which causes it to clear selection)
>+          if (!this.selectedItem) {
>+            // if the selectedIndex is larger than the row count, select the last
>+            // item.
>+            if (selectedIndex >= this.getRowCount())
>+              this.selectedIndex = this.getRowCount() - 1;
>+            else
>+              this.selectedIndex = selectedIndex;
>+          }
...
>+      <field name="_selectedIndex">-1</field>

So, it looks like the first time you open a richlistbox, |_refreshSelection|
will try to set the selectedIndex to -1, then the first <richlistitem> ctor
selects the first item. How about changing the |_selectedIndex| field initial
value to 0 and remvoing the richlistitem ctor part?

>       <property name="selectedIndex">
>         <getter>
>           <![CDATA[
>@@ -121,6 +203,9 @@
>         </getter>
>         <setter>
>           <![CDATA[
>+            if (val == this._selectedIndex)
>+              return;
>+

I'm in favor of removing this optimization, unless this is really desired.

Please rediff with -u8.

        Attachment #194538 -
        Flags: first-review?(bugs.mano) → first-review-

    
    

    
  
(Please also fix the missing semicolon in the selectedIndex setter).

    
    

    
  
we want this fix before we ship 1.5 so we can have a good API here, and the
sooner the better, but pushing it out to a beta 2 blocker since it's not yet
ready to land.
Flags: blocking1.8b5+
Flags: blocking1.8b4?
Flags: blocking1.8b4-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        third time's a charm (obsolete)
        — 
          — Splinter Review
      

    


  
I tested this pretty decently and all seems well.

        Attachment #194538 -
        Attachment is obsolete: true

        Attachment #194578 -
        Flags: first-review?(bugs.mano)

    
  

        Attachment #194578 -
        Attachment is obsolete: true

        Attachment #194578 -
        Flags: first-review?(bugs.mano)

    
    

    
  
Comment on attachment 194597 [details] [diff] [review]
puts back the last-selected setting code into richlistbox, 1.8 only still

Index: toolkit/content/widgets/richlistbox.xml
===================================================================
...
+      <method name="_refreshSelection">
...
+	       if (item) {
+		 this.selectedItem = item;
+		 this.ensureSelectedElementIsVisible();

Remove this call (it was already done).

+		} else {

fix indent.

...
+	     if (selectedIndex >= this.getRowCount())
+	       this.selectedIndex = this.getRowCount() - 1;
+	     else
+	       this.selectedIndex = selectedIndex;
+
+	     this.ensureSelectedElementIsVisible();

see above.


...
	     if (val) {
-	       val.selected = true;
-	       this.scrollBoxObject.ensureElementIsVisible(val);
	       this.fireActiveItemEvent();
	     }

remove single line brackets.


...
       <destructor>
	 <![CDATA[
	   // when we are destructed and we are selected, unselect ourselves so
	   // that richlistbox's selection doesn't point to something not in
the DOM.
	   if (this.selected) {
-	     this.control.clearSelection();
+	     this.control._setItemSelection(null);

Maybe add a comment here explaing we don't want last-selected to be modified
(i.e. why we're not using clearSelection).


Index: toolkit/mozapps/downloads/content/downloads.js
===================================================================

...
+    gDownloadsView.selectedIndex = selectedIndex


missing semicolon.



r=mano with those fixed (please do attach le final patch).

        Attachment #194597 -
        Flags: first-review?(bugs.mano) → first-review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (1.8)
        — 
          — Splinter Review
      

    


  
I had to keep the final ensureSelectedElementIsVisible() because it works
around a bug in scrollbox, I added an XXX: comment.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        trunk patch
        — 
          — Splinter Review
      

    


  

    
    

    
  
Comment on attachment 194655 [details] [diff] [review]
patch (1.8)


>             if (val) {
>-              val.selected = true;
>-              this.scrollBoxObject.ensureElementIsVisible(val);
>               this.fireActiveItemEvent();
>             }

remove single line brackets before checking this in.

    
    

    
  
checked into trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED

    
  
Target Milestone: --- → mozilla1.8beta4

    
    

    
  
Comment on attachment 194655 [details] [diff] [review]
patch (1.8)

No known regressions so far, asking for approval.

        Attachment #194655 -
        Flags: approval1.8b4?

    
  

        Attachment #194655 -
        Flags: approval1.8b4? → approval1.8b4+

    
    

    
  
checked into branch.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: