730078 - Remove the QFB-button from defaultset

Status: RESOLVED FIXED

Description

[Richard Marti (:Paenglab)]

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.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: Change the default

Concerning Mike Conley the migration problem is the defaultset.

I'm asking for approval‑calendar‑beta because this needs to land in beta.

Comment 2

[Chris Coulson]

Attachment: Don't hardcode defaultset in an overlay, as it can break migrations and conflict with other addons

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 3

[Richard Marti (:Paenglab)]

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.

Comment 4

[Philipp Kewisch [:Fallen] [:📆]]

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.

Comment 5

[Philipp Kewisch [:Fallen] [:📆]]

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.

Comment 6

[Chris Coulson]

Yes, sorry, will do that now. Thanks!

Comment 7

[Chris Coulson]

Attachment: Don't hardcode defaultset in an overlay, as it can break migrations and conflict with other addons (v2)

(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.

Comment 8

[Richard Marti (:Paenglab)]

(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?

Comment 9

[Chris Coulson]

Right, hence my last comment :)

Comment 10

[Chris Coulson]

Attachment: Don't hardcode defaultset in an overlay, as it can break migrations and conflict with other addons (v3)

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.

Comment 11

[Philipp Kewisch [:Fallen] [:📆]]

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.

Comment 12

[Philipp Kewisch [:Fallen] [:📆]]

Pushed to comm-central changeset d3f4108df62e

Comment 13

[Philipp Kewisch [:Fallen] [:📆]]

Backported to releases/comm-aurora changeset 810dff72d281

Comment 14

[Philipp Kewisch [:Fallen] [:📆]]

Backported to releases/comm-beta changeset 503595f53b2e