1484680 - [de-xbl] Remove treenode-checkbox

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Arshad Khan [:arshad]]

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

Comment 1

[Arshad Khan [:arshad]]

Attachment: treenode-checkbox-patch.diff

Comment 2

[Magnus Melin [:mkmelin]]

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

Comment 3

[Arshad Khan [:arshad]]

Attachment: treenode-checkbox-patch

Comment 4

[Arshad Khan [:arshad]]

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?

Comment 5

[Richard Marti (:Paenglab)]

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.

Comment 6

[Magnus Melin [:mkmelin]]

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

Comment 7

[Arshad Khan [:arshad]]

Attachment: 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?

Comment 8

[Richard Marti (:Paenglab)]

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

Comment 9

[Magnus Melin [:mkmelin]]

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?

Comment 10

[Jorg K (CEST = GMT+2)]

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

Comment 11

[Arshad Khan [:arshad]]

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

Comment 12

[Arshad Khan [:arshad]]

Attachment: treenode-checkbox.patch

Comment 13

[Arshad Khan [:arshad]]

Jorg, could you please check whether the patch is applying or not?

Comment 14

[Jorg K (CEST = GMT+2)]

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

Comment 15

[Jorg K (CEST = GMT+2)]

Most likely you invalidated the patch in bug 1484653 which has now landed.
https://hg.mozilla.org/comm-central/log/tip/calendar/base/content/widgets/calendar-widget-bindings.css

Comment 16

[Arshad Khan [:arshad]]

Attachment: treenode-checkbox.patch

Comment 17

[Arshad Khan [:arshad]]

Attachment: treenode-checkbox.patch

Comment 18

[Arshad Khan [:arshad]]

I have rebased the patch. Could you please take a look again?

Comment 19

[Jorg K (CEST = GMT+2)]

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

Thanks. Sticking the r+ back on.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2dc8e39bc37f
Remove treenode-checkbox binding. r=philipp

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/84f675096b81
Backed out changeset 2dc8e39bc37f for causing a test failure in testTodayPane.js. a=backout

Comment 22

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 23

[Arshad Khan [:arshad]]

Attachment: treenode-checkbox.patch

Comment 24

[Arshad Khan [:arshad]]

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

Comment 25

[Magnus Melin [:mkmelin]]

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: ..."

Comment 26

[Arshad Khan [:arshad]]

Attachment: treenode-checkbox.patch

fixed the dash nit.

Comment 27

[Magnus Melin [:mkmelin]]

When you upload a patch that has r+, please also set the review+ flag on it, and note e.g. "carrying forward r=Fallen"

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/779d0a64b2dd
Remove treenode-checkbox binding. r=philipp,mkmelin

Comment 29

[Geoff Lankow (:darktrojan)]

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

[Jorg K (CEST = GMT+2)]

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"