Closed Bug 594704 Opened 14 years ago Closed 14 years ago

add better notification setup for Fx4

Categories

(Firefox :: Sync, defect)

Product:
Firefox
Component:
Sync
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: mconnor, Assigned: philikon)

References

Details

Attachments

(3 files, 8 obsolete files)

screenshot OSX (patch v3.1)
310.52 KB, image/png
Details
v4
11.86 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
v4.1 for check in
11.99 KB, patch
Details | Diff | Splinter Review
Attached patch wip (obsolete) — — Splinter Review 
starting with moving the current notificationbox to a separate toolbar that shows/hides based on having active errors.  This means we don't need to reinvent priorities and the like, since it's basically the old infobar API.  This can be easily munged into an app-wide notification framework (and we can later redirect that API to home tab, or whatever, if we see fit in the future).
Attachment #473449 - Attachment is patch: true
Attachment #473449 - Attachment mime type: application/octet-stream → text/plain
Attached patch part 1 (v1) (obsolete) — — Splinter Review 
Given the lateness of the hour, this is part 1, just to get stuff out the status bar.

Part 1: move the existing notifications framework to a hidden toolbar that shows/hides when there are errors.

Part 2 is converting all of this to a single XBL binding that we can insert when we need it.

Part 3 is where we can play with display/stacking options if we want to modify the direction.
Assignee: nobody → mconnor
Status: NEW → ASSIGNED
Attachment #474124 - Flags: review?(gavin.sharp)
blocking2.0: --- → beta6+
Comment on attachment 474124 [details] [diff] [review]
part 1 (v1)

>diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js

>+    // xxxmpc - force the constructor to fire, sigh
>+    document.getElementById("sync-notifications-box");

Why do you need to do this?

>diff --git a/browser/base/content/syncNotification.xml b/browser/base/content/syncNotification.xml

>+      <method name="_updateButton">
>+        <body><![CDATA[
>+          Cu.import("resource://services-sync/notifications.js");

Can you only import this once?

>@@ -116,7 +142,6 @@

>-          node.className = notification.constructor.name;

Why are you removing this?

>     <implementation>
>+      <constructor><![CDATA[
>+        // xxxmpc - this shouldn't be necessary, but seems to be!
>+        this.setAttribute("type", this.type);
>+        this.setAttribute("priority", this.priority);

This doesn't make any sense - these getters just call getAttribute. What happens if you remove them?

Otherwise this looks OK, but dolske reviewed the original notification stuff, so perhaps he'd be a better reviewer for this?
Depends on: 595954
Attached patch part 1 (v2) (obsolete) — — Splinter Review 
From discussion:

a) init the binding on the first received notification, kill the observer after that
b) filed and fixed bug 595954 to make the one hack unnecessary
c) cleaned up show/hide to work sanely with a)
Attachment #473449 - Attachment is obsolete: true
Attachment #474124 - Attachment is obsolete: true
Attachment #474790 - Flags: review?(gavin.sharp)
Attachment #474124 - Flags: review?(gavin.sharp)
Comment on attachment 474790 [details] [diff] [review]
part 1 (v2)

wrong diff
Attachment #474790 - Attachment is obsolete: true
Attachment #474790 - Flags: review?(gavin.sharp)
Attached patch part 1 (v2) (obsolete) — — Splinter Review 

        Attachment #474791 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 474791 [details] [diff] [review]
part 1 (v2)

>diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js

>+  initNotifications: function SUI_initNotifications() {
>+    // force the constructor to fire
>+    document.getElementById("sync-notifications-box");

I'm still not happy with this - the hackiness of forcing binding construction to do work offends me a lot more than the thought of having the notification logic not be self-contained within the binding :) Rather than having the notificationbox constructor display all pending notifications, can you not instead just have it behave like a normal notificationbox and do nothing on construction, and then have this code do the observing and calling of .hidden=false/appendNotification on it?

        Attachment #474791 -
        Flags: review?(gavin.sharp)

    
  
Assignee: mconnor → philipp

    
    

    
  
philIKON: need an updated ETA ASAP on this.

    
    

    
  
(In reply to comment #7)
> philIKON: need an updated ETA ASAP on this.

Will have a new patch ready later today.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3 (obsolete)
        — 
          — Splinter Review
      

    


  
This makes notifications directly display in the lower toolbar, much like the mockups in bug 589981. Create the toolbar + notificationbox lazily on the first notification.

No styling yet, though. Will happen either in a part2 patch or in a follow-up bug (probably latter since we don't have a definitive style mockup yet).

        Attachment #474791 -
        Attachment is obsolete: true

        Attachment #477369 -
        Flags: review?(gavin.sharp)

    
    

    
  

      
      
      
      

        Attached image
          
        screenshot OSX (patch v3) (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3.1 (except it's v3 again) (obsolete)
        — 
          — Splinter Review
      

    


  
Feedback from mconnor: Drop title in notification. Not really necessary since we're displaying it in primary UI now.

        Attachment #477369 -
        Attachment is obsolete: true

        Attachment #477375 -
        Flags: review?(gavin.sharp)

        Attachment #477369 -
        Flags: review?(gavin.sharp)

    
    

    
  


  

        Attachment #477371 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3.1 (obsolete)
        — 
          — Splinter Review
      

    


  
D'oh, v3.1 was just v3 again... Here's the proper v3.1 patch with mconnor's feedback.

        Attachment #477375 -
        Attachment is obsolete: true

        Attachment #477378 -
        Flags: review?(gavin.sharp)

        Attachment #477375 -
        Flags: review?(gavin.sharp)

        Attachment #477375 -
        Attachment description: v3.1 → v3.1 (except it's v3 again)

    
    

    
  
Do we need a toolbar at all? Can we not just insert the <notificationbox> directly into the bottombox? If we can do that, then I think we can also avoid the need to hide/unhide anything, right? Then these notifications can work the same way that normal notifications work, just in a different container.

Are there any tests for this functionality (sync notifications in general)?

    
    

    
  
(In reply to comment #14)
> Do we need a toolbar at all? Can we not just insert the <notificationbox>
> directly into the bottombox?

You're right, we can! I guess I just blindly carried that over from the previous patch without questioning it...

> If we can do that, then I think we can also avoid
> the need to hide/unhide anything, right?

Yes! There's a rather ugly fade-out transition now, but that's all fixable with styling (= follow up bug). Will prepare a new patch.

> Are there any tests for this functionality (sync notifications in general)?

We have unit tests for Weave.Notifications. Unfortunately we're not doing great on UI tests atm :(

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v4
        — 
          — Splinter Review
      

    


  
Incorporate gavin's feedback: Get rid of superfluous toolbar.

        Attachment #477378 -
        Attachment is obsolete: true

        Attachment #477642 -
        Flags: review?(gavin.sharp)

        Attachment #477378 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 477642 [details] [diff] [review]
v4

>diff --git a/browser/base/content/syncNotification.xml b/browser/base/content/syncNotification.xml

>   <binding id="notification" extends="chrome://global/content/bindings/notification.xml#notification">

>-            <xul:description anonid="messageText" class="messageText" flex="1" xbl:inherits="xbl:text=label"/>
>+          <xul:description anonid="messageText" class="messageText" xbl:inherits="xbl:text=value"/>

Why this change to the inherited attribute? It seems to me like you actually want to change the order of notification.title/notification.description parameters in _appendNotification (first argument to appendNotification is "label", i.e. text displayed in the notification, while the second a "name" for the notification).

It also seems like you're intentionally differing from the base binding's <content> to have the buttons appear left-aligned (immediately following the text) rather than right-aligned - why is that?

r=me with those addressed.

        Attachment #477642 -
        Flags: review?(gavin.sharp) → review+

    
    

    
  
(In reply to comment #17)
> >   <binding id="notification" extends="chrome://global/content/bindings/notification.xml#notification">
> 
> >-            <xul:description anonid="messageText" class="messageText" flex="1" xbl:inherits="xbl:text=label"/>
> >+          <xul:description anonid="messageText" class="messageText" xbl:inherits="xbl:text=value"/>
> 
> Why this change to the inherited attribute? It seems to me like you actually
> want to change the order of notification.title/notification.description
> parameters in _appendNotification (first argument to appendNotification is
> "label", i.e. text displayed in the notification, while the second a "name" for
> the notification).

Oh right. We were displaying both title + description earlier, but this isn't necessary anymore.

> It also seems like you're intentionally differing from the base binding's
> <content> to have the buttons appear left-aligned (immediately following the
> text) rather than right-aligned - why is that?

See mockups in bug 589981 (attachment 471288 [details]).

    
    

    
  


  
Address review comments: flip order of appendNotification() parameters, continue to use the "label" attribute in the binding.

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/8a7efa16fd64

We need to get tests for this stuff. Followup bug?
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
OS: Windows Vista → All
Hardware: x86 → All
Resolution: --- → FIXED

    
    

    
  
(In reply to comment #20)
> http://hg.mozilla.org/mozilla-central/rev/8a7efa16fd64
> 
> We need to get tests for this stuff. Followup bug?

Filed bug 598803 for the tests, bug 598799 for the styling.

    
    

    
  
verified with recent nightly minefield builds
Status: RESOLVED → VERIFIED
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: