Closed
Bug 774520
Opened 12 years ago
Closed 12 years ago
display multiple providers in toolbar ui
Categories
(Firefox Graveyard :: SocialAPI, defect)
Firefox Graveyard
SocialAPI
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 809694
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
(Whiteboard: [Fx18][strings:1] )
Attachments
(3 files, 8 obsolete files)
need to have the ability to switch between providers in the ui
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #642785 -
Flags: feedback?(jaws)
Attachment #642785 -
Flags: feedback?(gavin.sharp)
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Fx17]
Version: unspecified → Trunk
Comment 2•12 years ago
|
||
Comment on attachment 642785 [details] [diff] [review] multiprovider.patch Review of attachment 642785 [details] [diff] [review]: ----------------------------------------------------------------- On a side-note: can you update your .hgrc to include 8 lines of context for your changes? I'm not sure exactly what is changing within the browser.css file unless I apply the patch. This section provides some basic defaults for hgrc files that will include more context on the changes <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F> ::: browser/base/content/browser-social.js @@ +296,5 @@ > + > + renderProviderMenu: function SocialToolbar_renderProviderMenu() > + { > + // Put the list of providers in a submenu: > + Cu.import("resource://gre/modules/SocialService.jsm"); Is there a better way to do this? I don't think we should be loading the JSM each time this function is called. @@ +320,5 @@ > + menuitem.setAttribute("checked", "true"); > + } else { > + menuitem.setAttribute("oncommand", "Social.setProvider(this.getAttribute('origin'))"); > + } > + subMenu.appendChild(menuitem);//insertBefore(menuitem, before); I know this is a feedback pass, but just wanted to let you know about this commented out line that looks like it could just be deleted. ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +653,4 @@ > toolbar button --> > <!ENTITY markupButton.accesskey "M"> > > +<!ENTITY socialProviderMenu.label "Switch social network"> I'm not sure about this terminology here. Not all types of providers may consider themselves "social networks". ::: browser/modules/Social.jsm @@ +16,4 @@ > XPCOMUtils.defineLazyModuleGetter(this, "SocialService", > "resource://gre/modules/SocialService.jsm"); > > +var _provider; It looks like this gets added to the global scope (I could be wrong). Can you move this variable to within the Social object? @@ +49,5 @@ > + try { > + let current = Services.prefs.getCharPref("social.provider.current"); > + SocialService.getProvider(current, function(provider) { > + this.provider = provider; > + if (callback) I think this should also check to see if |provider| is truthy, since the callback may be depending on it. @@ +61,1 @@ > callback(); This changes the previous behavior, which only executed the callback if provider.length > 0. ::: browser/themes/pinstripe/browser.css @@ +3351,5 @@ > -moz-appearance: none; > margin: 0; > padding: 2px; > + min-width: 20px; > + min-height: 20px; Why make these changes as part of this patch? Should there be similar changes for the other themes, or should these style changes be part of a different bug?
Attachment #642785 -
Flags: feedback?(jaws)
Comment 3•12 years ago
|
||
Comment on attachment 642785 [details] [diff] [review] multiprovider.patch I think we should defer this for the moment, and focus on the 16-targeted work items.
Attachment #642785 -
Flags: feedback?(gavin.sharp) → feedback-
Assignee | ||
Comment 4•12 years ago
|
||
just keeping patch updated.
Attachment #642785 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
Assignee: nobody → mixedpuppy
Attachment #648116 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #651603 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #651913 -
Attachment is obsolete: true
Attachment #652627 -
Flags: feedback?(mhammond)
Attachment #652627 -
Flags: feedback?(felipc)
Comment 8•12 years ago
|
||
Comment on attachment 652627 [details] [diff] [review] multiprovider.patch with tests + SocialToolbar.renderProviderMenu(); I can't see that new method? + try { + let current = Services.prefs.getCharPref("social.provider.current"); + SocialService.getProvider(current, function(provider) { + this.provider = provider; + if (callback) + callback(); + }.bind(this)); + } catch(e) { + SocialService.getProviderList(function (providers) { + if (providers.length) + this.provider = providers[0]; + if (callback) + callback(); + }.bind(this)); This exception handler seems overly broad. I guess you are just guarding against the pref not being set? If so, I'd suggest just catching that.
Attachment #652627 -
Flags: feedback?(mhammond) → feedback+
Assignee | ||
Comment 9•12 years ago
|
||
fixes patch with missing method
Attachment #652627 -
Attachment is obsolete: true
Attachment #652627 -
Flags: feedback?(felipc)
Attachment #653619 -
Flags: feedback?(mhammond)
Attachment #653619 -
Flags: feedback?(jaws)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [Fx17] → [Fx17][strings:1]
Assignee | ||
Comment 10•12 years ago
|
||
rebased
Attachment #653619 -
Attachment is obsolete: true
Attachment #653619 -
Flags: feedback?(mhammond)
Attachment #653619 -
Flags: feedback?(jaws)
Updated•12 years ago
|
Blocks: 786131
Summary: support multiple providers → dislpay multiple providers in toolbar ui
Comment 11•12 years ago
|
||
Comment on attachment 655782 [details] [diff] [review] multiprovider.patch > // If the last event was received < 1s ago, ignore this one I think we should kill that code - Gavin says he remembers adding it but doesn't think it is adding any value at the moment. + ["social-notification-box", + "social-status-iconbox"].forEach(function removeChildren(parentId) { Most existing code seems to be using the pattern: for (let parentId of ["social-...", ...]) { + if (!provider.enabled) continue; If a disabled provider later becomes enabled (or vice-versa), it doesn't appear the UI reflect that fact? We probably need to fire a "social:providers-updated" notification when this happens. Plus test coverage for this. + browser_social_multiprovider.js \ appears to be mixing tabs and spaces?
Comment 12•12 years ago
|
||
Oops - I meant to add that the feedback above is in addition to the previous feedback from jaws and myself.
Assignee | ||
Updated•12 years ago
|
Whiteboard: [Fx17][strings:1] → [Fx18][strings:1]
Comment 13•12 years ago
|
||
Minor updates to Shane's patch: * Ensure the profile info on the "share" button is updated. * Moar tests! * All feedback comments from previous patch addressed.
Attachment #655782 -
Attachment is obsolete: true
Comment 14•12 years ago
|
||
This patch is on top of the previous patch and is more about cleanup rather than fixing things. * The various "init" methods of the social elements now perform initialization that should happen exactly once, rather than things which depend on the current provider. These are now always called, even if no provider is current, and should address Gavin's comments in bug 781386 comment 0. * The code for handling "preference changed", "provider changed" and "provider profile changed" has been split into new methods, and is called both at initialize time (assuming a current provider etc exists at that time) and as we observer notifications telling us about the changes. * The Social:Toggle element is updated as we change providers - however, as per bug 789672, this element will probably need more radical changes in a multi-provider world. * There is some additional refactoring that could be done - eg, SocialToolbar methods updateButton, updateButtonHiddenState and updateProfile are all doing similar work), but I've left them alone for now.
Attachment #660293 -
Flags: feedback?(gavin.sharp)
Comment 15•12 years ago
|
||
Comment on attachment 660293 [details] [diff] [review] refactor initialization of social components This looks like a nice cleanup, but this seems like the wrong bug to be tracking the work in (we could land this patch as a precursor to "display multiple providers in toolbar UI"). >diff --git a/browser/base/content/browser-social.js b/browser/base/content/browser-social.js >+ _onPrefChanged: function SocialUI_onPrefChanged() { I noticed you re-ordered the calls in this when copying from the pref-changed observer - was that meaningful or just tidying up? > // Called once Social.jsm's provider has been set >+ _postInit: function SocialUI_postInit() { >+ if (Social.provider) { >+ this._onProviderChanged(); We're now breaking some of the core assumptions this code had. From the comment above: Social.provider would previously always be set in this method, so the null check here wouldn't be needed. If we're changing that, we'll need to change the comments accordingly. > // Called once, after window load, when the Social.provider object is initialized > init: function SSB_init() { Should just remove this if it's going to be empty. > init: function SocialToolbar_init() { >- document.getElementById("social-provider-image").setAttribute("image", Social.provider.iconURL); This gets removed but not re-added anywhere?
Attachment #660293 -
Flags: feedback?(gavin.sharp) → feedback+
Updated•12 years ago
|
Summary: dislpay multiple providers in toolbar ui → display multiple providers in toolbar ui
Comment 16•12 years ago
|
||
This is a refresh against the current tip. It hasn't been extensively tested or self-reviewed, but does basically work. I'll get back to this and the other "refactor" patch asap.
Attachment #660291 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
fixes menus and error state for sidebar
Assignee | ||
Comment 18•12 years ago
|
||
I am closing this bug in favor of a string of bugs with smaller patches. The string of bugs starts with bug 809694
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•