Closed Bug 1582573 Opened 5 months ago Closed 5 months ago

Indentation of XUL strings in custom elements are off

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(thunderbird_esr6869+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 69+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: pmorris, Assigned: pmorris)

References

Details

Attachments

(6 files, 2 obsolete files)

The change to two space indent with the Prettier formatting in calendar code left the indentation of the XUL strings passed to parseXULToFragment for custom elements in a messy state. Let's clean them up.

Edit: the indentation of XUL strings in non-calendar custom elements is also off, due to wrapping custom elements in curly brackets and Prettier formatting. Lets use this bug for fixing these too.

Summary: Re-indent XUL strings in calendar custom elements → Indentation of XUL strings in calendar custom elements are off
Attached patch xul-string-indent-calendar.patch (obsolete) — Splinter Review

Re-indents the XUL strings in calendar custom elements.

Attachment #9094032 - Flags: review?(geoff)
Comment on attachment 9094032 [details] [diff] [review]
xul-string-indent-calendar.patch

TBH, you probably could've gotten away with the "white-space-only" rubber-stamp on this. I had a quick scan and checked the stuff that `diff -w` didn't ignore.
Attachment #9094032 - Flags: review?(geoff) → review+
Status: NEW → ASSIGNED
Product: Calendar → Thunderbird
Summary: Indentation of XUL strings in calendar custom elements are off → Indentation of XUL strings in custom elements are off
Target Milestone: --- → Thunderbird 71.0

Re-indents XUL strings in non-calendar custom elements as needed. I've never used the whitespace-only option, not sure how it works, so flagging mkmelin for review. Feel free to cancel that if you'd rather I do the whitespace-only option.

Attachment #9094156 - Flags: review?(mkmelin+mozilla)
Attachment #9094156 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

Hmm, this should also go into beta and ESR 68, no? Are we done with reformatting one day?

Also, this clashes with bug 1582796 which is more important.

Keywords: checkin-needed
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a55d5e1c611a
Re-indent XUL strings in non-calendar custom elements. r=mkmelin
Attachment #9094156 - Attachment filename: xul-string-indent-non-calendar-0.patch → [cheked in] xul-string-indent-non-calendar-0.patch
Attachment #9094156 - Attachment description: xul-string-indent-non-calendar-0.patch → [cheked in] xul-string-indent-non-calendar-0.patch
Attachment #9094156 - Attachment filename: [cheked in] xul-string-indent-non-calendar-0.patch → xul-string-indent-non-calendar-0.patch

Calendar patch needs unbitrotting

Attachment #9094156 - Attachment description: [cheked in] xul-string-indent-non-calendar-0.patch → [checked in] xul-string-indent-non-calendar-0.patch

This just rebases the calendar patch.

"This should also go into beta and ESR 68, no?"
Yeah, I suppose so. (I'm still not 100% sure how you indicate that among all the knobs bugzilla provides.) Aren't we about to release another beta? If so, then maybe we don't need to backport to beta. I can upload patches for ESR.

"Are we done with reformatting one day?"
I hope so, although there are other similar places with multi-line template strings that may be off. I've explored them a bit and they do not seem horrible. I could see the case for just fixing them as we are editing the code, rather than going hunting for every instance.

Attachment #9094032 - Attachment is obsolete: true
Comment on attachment 9094610 [details] [diff] [review]
xul-string-indent-calendar-1.patch

Something went wrong with patch export. :-/  New patch in a sec.
Attachment #9094610 - Attachment is obsolete: true

Fixed.

Keywords: checkin-needed

Aren't we about to release another beta? If so, then maybe we don't need to backport to beta. I can upload patches for ESR.

Yes, TB 70 beta 2 was released yesterday (Sunday!). Between now and 2019-10-21 (see https://wiki.mozilla.org/Release_Management/Calendar) we'll still be doing a TB 70 beta 3, maybe even 4. So backporting makes sense to avoid merge conflicts later. I tried one of the patches and the already didn't apply to beta. Yes, ESR is also required.

Keywords: leave-open

1 of 2 for ESR 68.

2 of 2 for ESR 68.

Attachment #9094628 - Flags: approval-comm-esr68+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cf7a0bacde48
Re-indent XUL strings in calendar custom elements. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Geoff, can you please confirm beta/esr approval for the Calendar bits. I assume you can't set it and I can't request Calendar approval on this bug.

Flags: needinfo?(geoff)

1 of 2 for beta.

Attachment #9094663 - Flags: approval-comm-beta?

2 of 2 for beta.

Attachment #9094664 - Flags: approval-comm-beta?
Attachment #9094663 - Flags: approval-comm-beta? → approval-comm-beta+

Approved.

Flags: needinfo?(geoff)
Attachment #9094629 - Flags: approval-comm-esr68+
Comment on attachment 9094664 [details] [diff] [review]
beta-xul-string-indent-calendar-0.patch

Setting flags according to Geoff's approval.
Attachment #9094664 - Flags: approval-comm-beta? → approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.