[de-xbl] Remove treenode-checkbox

RESOLVED FIXED in 6.6

Status

defect
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 1 bug)

Trunk
Dependency tree / graph

Details

Attachments

(1 attachment, 7 obsolete attachments)

Assignee

Description

10 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36
Assignee

Comment 1

10 months ago
Posted patch treenode-checkbox-patch.diff (obsolete) — Splinter Review
Flags: needinfo?(mkmelin+mozilla)
Attachment #9002433 - Flags: review?(philipp)
Assignee

Updated

10 months ago
Component: Untriaged → General
Product: Thunderbird → Calendar
Assignee

Updated

10 months ago
Assignee: nobody → arshdkhn1
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Updated

10 months ago
Assignee

Updated

10 months ago
Summary: [de-xbl]Remove treenode-checkbox → [de-xbl] Remove treenode-checkbox
Comment on attachment 9002433 [details] [diff] [review]
treenode-checkbox-patch.diff

Looks ok in general. The patch no longer applies cleanly though.

Also, you missed at least this bit: https://searchfox.org/comm-central/rev/cce392b6a57170eb2139bbf15eaa9a88340f97b2/calendar/base/content/widgets/calendar-widgets.xml#105
Attachment #9002433 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #9002433 - Flags: review?(philipp)
Assignee

Comment 3

10 months ago
Posted patch treenode-checkbox-patch (obsolete) — Splinter Review
Assignee

Comment 4

10 months ago
Hey, the icon for treenode-checkbox was different than the icon I had in production build of Thunderbird I have right not. I was also not able to inspect it. Can you take a look at the UI?
Flags: needinfo?(richard.marti)
Assignee

Updated

10 months ago
Attachment #9006051 - Flags: review?(mkmelin+mozilla)
Yes, this changed in bug 1483332 on august 14. to use the new shared twisty icons from toolkit.

You work on c-c, correct? When something changes in m-c we depend on, we have to follow.
Flags: needinfo?(richard.marti)
Comment on attachment 9006051 [details] [diff] [review]
treenode-checkbox-patch

Review of attachment 9006051 [details] [diff] [review]:
-----------------------------------------------------------------

Someone from calendar/ should review this.

I tried it out, and it seems to work. You did miss one mention of treenode-checkbox in calendar-widgets.xml still so please remove that.
When you remove/convert things it's often useful to do a quick grep once you think you have it all, to see if anything is left, like
  grep -ri treenode-checkbox mail mailnews calendar

In the commit message, please add " - " after Bug. Also r?<reviewer>, or r=? until it's reviewed
Attachment #9006051 - Flags: review?(mkmelin+mozilla) → feedback+
Assignee

Comment 7

10 months ago
Posted patch treenode-checkbox-patch (obsolete) — Splinter Review
Magnus, I have addressed issues you commented. 

Richard, If you goto https://ibin.co/4EXmKwwxXKrE.png you ll see that there should be some space between the treenode-checkbox and localfolders text. I am not addressing this issue in this patch. Also I can't inspect the sidebar unfortunately. Can you tell me why can't I do it?
Attachment #9006051 - Attachment is obsolete: true
Flags: needinfo?(richard.marti)
Attachment #9006245 - Flags: review?(mkmelin+mozilla)
(In reply to Arshad Khan [:arshad] from comment #7)
> Created attachment 9006245 [details] [diff] [review]
> treenode-checkbox-patch
> 
> Magnus, I have addressed issues you commented. 
> 
> Richard, If you goto https://ibin.co/4EXmKwwxXKrE.png you ll see that there
> should be some space between the treenode-checkbox and localfolders text. I
> am not addressing this issue in this patch. Also I can't inspect the sidebar
> unfortunately. Can you tell me why can't I do it?

This isn't a treenode-checkbox, this is a tree twisty in the folder pane. Trees are a bit special and can't be inspected. Your treenode-checkbox is in the today pane (the "Today", "Tomorrow" and the "Upcoming (x days)"). And this are inspectable without any problem.

For the missing space between the twisty and the icon you can file a bug and cc me. Or I'll do this later.
Flags: needinfo?(richard.marti)
Comment on attachment 9006245 [details] [diff] [review]
treenode-checkbox-patch

Review of attachment 9006245 [details] [diff] [review]:
-----------------------------------------------------------------

Yep looks good. Since this is calendar/, maybe Philipp wants to still r+ it?
Attachment #9006245 - Flags: review?(philipp)
Attachment #9006245 - Flags: review?(mkmelin+mozilla)
Attachment #9006245 - Flags: feedback+
Attachment #9006245 - Flags: review?(philipp) → review+
Assignee

Updated

10 months ago
Keywords: checkin-needed

Comment 10

10 months ago
The patch doesn't apply. Also, the commit message is wrong:
  Bug 1484679 Remove treenode-checkbox binding.
Apart from the missing hyphen, the bug number is wrong. If I don't notice that, the but gets landed with the incorrect number and needs to be backed out and relanded with the correct number :-(
Keywords: checkin-needed
Assignee

Comment 11

10 months ago
(In reply to Jorg K (GMT+2) from comment #10)
> The patch doesn't apply. Also, the commit message is wrong:
>   Bug 1484679 Remove treenode-checkbox binding.
> Apart from the missing hyphen, the bug number is wrong. If I don't notice
> that, the bug gets landed with the incorrect number and needs to be backed
> out and relanded with the correct number :-(

I am very sorry. I ll be careful about this in future. I think i messed up the bug no while ammending the commit. :(
Assignee

Comment 12

10 months ago
Posted patch treenode-checkbox.patch (obsolete) — Splinter Review
Attachment #9006245 - Attachment is obsolete: true
Assignee

Comment 13

10 months ago
Jorg, could you please check whether the patch is applying or not?
Flags: needinfo?(jorgk)

Comment 14

10 months ago
Apart from the header, it's the same patch, so it still doesn't apply. Please rebase the patch.

patching file calendar/base/content/widgets/calendar-widget-bindings.css
Hunk #1 succeeded at 4 with fuzz 2 (offset 0 lines).
patching file calendar/base/content/widgets/calendar-widgets.xml
Hunk #1 FAILED at 47
1 out of 3 hunks FAILED -- saving rejects to file calendar/base/content/widgets/calendar-widgets.xml.rej
Flags: needinfo?(jorgk)
Assignee

Comment 16

10 months ago
Posted patch treenode-checkbox.patch (obsolete) — Splinter Review
Attachment #9008375 - Attachment is obsolete: true
Assignee

Comment 17

10 months ago
Posted patch treenode-checkbox.patch (obsolete) — Splinter Review
Attachment #9008445 - Attachment is obsolete: true
Assignee

Comment 18

10 months ago
I have rebased the patch. Could you please take a look again?
Flags: needinfo?(jorgk)

Comment 19

10 months ago
Comment on attachment 9008448 [details] [diff] [review]
treenode-checkbox.patch

Thanks. Sticking the r+ back on.
Flags: needinfo?(jorgk)
Attachment #9008448 - Flags: review+
Assignee

Updated

10 months ago
Keywords: checkin-needed

Comment 20

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2dc8e39bc37f
Remove treenode-checkbox binding. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

10 months ago
Target Milestone: --- → 6.6

Comment 21

10 months ago
Backout by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/84f675096b81
Backed out changeset 2dc8e39bc37f for causing a test failure in testTodayPane.js. a=backout

Comment 22

10 months ago
Sorry I had to back this out since it caused the following test failure:
https://taskcluster-artifacts.net/NwBfUnSjSj6CIEj9mNyAZA/0/public/logs/live_backing.log

INFO -  SUMMARY-UNEXPECTED-FAIL | testTodayPane.js | testTodayPane.js::testTodayPane
INFO -    EXCEPTION: Expression "{"class":"agenda-checkbox"}" returned null. Anonymous == true
INFO -      at: elementslib.js line 486
INFO -         reduceLookup elementslib.js:486 15
INFO -         Lookup.prototype.getNode elementslib.js:501 10
INFO -         testTodayPane testTodayPane.js:268 8
INFO -         Runner.prototype.wrapper frame.js:584 9

Please fix this and resubmit the patch for review. You can run MozMill tests locally from your object directory via:
mozmake -C calendar/test/mozmill SOLO_TEST=testTodayPane.js mozmill-one

Most likely an easy fix. Don't worry, even they Mozilla luminaries have their patches backed out at times ;-)
Status: RESOLVED → REOPENED
Flags: needinfo?(arshdkhn1)
Resolution: FIXED → ---
Assignee

Comment 23

10 months ago
Posted patch treenode-checkbox.patch (obsolete) — Splinter Review
Attachment #9008448 - Attachment is obsolete: true
Flags: needinfo?(arshdkhn1)
Assignee

Comment 24

10 months ago
Comment on attachment 9008671 [details] [diff] [review]
treenode-checkbox.patch

Review of attachment 9008671 [details] [diff] [review]:
-----------------------------------------------------------------

please check the testTodayPane.js and lemme know if I can do it in better way...
Attachment #9008671 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9008671 [details] [diff] [review]
treenode-checkbox.patch

Review of attachment 9008671 [details] [diff] [review]:
-----------------------------------------------------------------

Yes this works. 
nit: for the hg comment, we use "Bug 1484680 - ....", not "Bug 1484680: ..."
Attachment #9008671 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 26

10 months ago
fixed the dash nit.
Attachment #9008671 - Attachment is obsolete: true
Attachment #9008695 - Flags: review+
When you upload a patch that has r+, please also set the review+ flag on it, and note e.g. "carrying forward r=Fallen"
Keywords: checkin-needed

Comment 28

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/779d0a64b2dd
Remove treenode-checkbox binding. r=philipp,mkmelin
Status: REOPENED → RESOLVED
Closed: 10 months ago10 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Just something I noticed…

When you're modifying XML like this, you should line up the attributes again:

> <checkbox id="calendar-list-header"
>           checked="true"
>           class="treenode-checkbox"

etc.

Comment 30

10 months ago
Agreed, but they weren't lined up to start with :-( - And the reviewers didn't complain.

-              <treenode-checkbox id="task-tree-filter-header"
+              <checkbox id="task-tree-filter-header"
                                checked="true"
                                class="treenode-checkbox"
You need to log in before you can comment on or make changes to this bug.