Closed Bug 730078 Opened 9 years ago Closed 9 years ago

Remove the QFB-button from defaultset

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 3 obsolete files)

Now the QFB-button is moved to the mail-toolbar. To not blocking the migration the defaultset in messenger-overlay-sidebar.xul have to be removed.
Attached patch Change the default (obsolete) — Splinter Review
Concerning Mike Conley the migration problem is the defaultset.

I'm asking for approval‑calendar‑beta because this needs to land in beta.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #600145 - Flags: review?(philipp)
Attachment #600145 - Flags: approval-calendar-beta?
Attachment #600145 - Flags: approval-calendar-aurora?
Blocks: 730072
I took a different approach to this. The problem with hardcoding defaultset in an overlay is that it can break migrations like this, but it can also conflict with other addons if 2 addons try to override the defaultset
Comment on attachment 600352 [details] [diff] [review]
Don't hardcode defaultset in an overlay, as it can break migrations and conflict with other addons

This looks better than my amateurish approach. So asking for review.
Attachment #600352 - Flags: review?(philipp)
Attachment #600352 - Flags: approval-calendar-beta?
Attachment #600352 - Flags: approval-calendar-aurora?
Attachment #600145 - Attachment is obsolete: true
Attachment #600145 - Flags: review?(philipp)
Attachment #600145 - Flags: approval-calendar-beta?
Attachment #600145 - Flags: approval-calendar-aurora?
Comment on attachment 600352 [details] [diff] [review]
Don't hardcode defaultset in an overlay, as it can break migrations and conflict with other addons


>+    try {
>+        currentUIVer = Services.prefs.getIntPref(UI_VERSION_PREF);
>+    } catch(ex) {}
you could use cal.getPrefSafe(UI_VERSION_PREF, -1) here


>+  <toolbarpalette id="MailToolbarPalette">
>     <toolbarbutton id="calendar-tab-button"
We already have buttons in the Mail Toolbar, here:

http://mxr.mozilla.org/comm-central/source/calendar/lightning/content/lightning-toolbar.xul#93

Please adapt accordingly.

Also please use {} even for one-line if's to match calendar style

r=philipp and approval with those changes.
Attachment #600352 - Flags: review?(philipp)
Attachment #600352 - Flags: review+
Attachment #600352 - Flags: approval-calendar-beta?
Attachment #600352 - Flags: approval-calendar-beta+
Attachment #600352 - Flags: approval-calendar-aurora?
Attachment #600352 - Flags: approval-calendar-aurora+
Chris, do you have time to update this patch? This should go in soon since I'd like to create a new beta build this week.
Yes, sorry, will do that now. Thanks!
(In reply to Philipp Kewisch [:Fallen] from comment #4)

Ok, I've updated this now

> Comment on attachment 600352 [details] [diff] [review]
> Don't hardcode defaultset in an overlay, as it can break migrations and
> conflict with other addons
> 
> 
> >+    try {
> >+        currentUIVer = Services.prefs.getIntPref(UI_VERSION_PREF);
> >+    } catch(ex) {}
> you could use cal.getPrefSafe(UI_VERSION_PREF, -1) here
> 
> 
> >+  <toolbarpalette id="MailToolbarPalette">
> >     <toolbarbutton id="calendar-tab-button"
> We already have buttons in the Mail Toolbar, here:
> 
> http://mxr.mozilla.org/comm-central/source/calendar/lightning/content/
> lightning-toolbar.xul#93
> 
> Please adapt accordingly.
> 

I've dropped one set of buttons. I hope I understood correctly, and apologise if I've misunderstood what you were asking.

> Also please use {} even for one-line if's to match calendar style
> 
> r=philipp and approval with those changes.
Attachment #600352 - Attachment is obsolete: true
Attachment #601581 - Flags: review?(philipp)
(In reply to Chris Coulson from comment #7)
> Created attachment 601581 [details] [diff] [review]
> Don't hardcode defaultset in an overlay, as it can break migrations and
> conflict with other addons (v2)

This patch would remove lightning-button-calendar and lightning-button-tasks completely? I think this isn't what Philipp wants now. lightning-button-calendar and lightning-button-tasks should stay in the MailToolbarPalette or if customized on a toolbar. calendar-tab-button and task-tab-button should be irremovable (until a decision is made what's the future of this buttons) in tabbar-toolbar.

Is this correct Philipp?
Right, hence my last comment :)
Ok, so it was pointed out to me on IRC that the buttons on the tabbar aren't currently removable, which I hadn't realized. This makes things much simpler.

All we need to do now is to just not set defaultset at all. The only benefit of doing that previously is that it defined the placement of the buttons wrt the QFB for an unmodified tabbar, but as the defaultset is empty now in Thunderbird, we don't need to do this.
Attachment #601581 - Attachment is obsolete: true
Attachment #601734 - Flags: review?(philipp)
Attachment #601581 - Flags: review?(philipp)
Comment on attachment 601734 [details] [diff] [review]
Don't hardcode defaultset in an overlay, as it can break migrations and conflict with other addons (v3)

Ooh, simple! I like! Please push to aurora and beta too.
Attachment #601734 - Flags: review?(philipp)
Attachment #601734 - Flags: review+
Attachment #601734 - Flags: approval-calendar-beta+
Attachment #601734 - Flags: approval-calendar-aurora+
Pushed to comm-central changeset d3f4108df62e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.