Remove the QFB-button from defaultset

RESOLVED FIXED in 1.3

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

7 years ago
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.
Assignee

Comment 1

7 years ago
Posted 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?
Assignee

Updated

7 years ago
Blocks: 730072

Comment 2

7 years ago
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
Assignee

Comment 3

7 years ago
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?
Assignee

Updated

7 years ago
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.

Comment 6

7 years ago
Yes, sorry, will do that now. Thanks!

Comment 7

7 years ago
(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)
Assignee

Comment 8

7 years ago
(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

7 years ago
Right, hence my last comment :)

Comment 10

7 years ago
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: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
Backported to releases/comm-aurora changeset 810dff72d281
Target Milestone: 1.5 → 1.4
Backported to releases/comm-beta changeset 503595f53b2e
Target Milestone: 1.4 → 1.3
You need to log in before you can comment on or make changes to this bug.