402365 - [Trunk] Tabs break calendar mode view

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect, P1)

Description

[Omar Bajraszewski]

Attachment: Screenshot

Opening tabs in Tb 3 breaks calendar view. The main calendar isn't visible.

1)Open a message in a tab
2)Switch to calendar mode

Result:
The main calendar isn't visible

Lightning 0.6 a1 2007110304 and Tb version 3.0a1pre (2007110302)

Comment 1

[Omar Bajraszewski]

Attachment: Screenshot 2

I also noticed that closing the opened tab in calendar mode and switching back to mail mode broke mail mode too.
Notice there's no mail list above message pane

Comment 2

[Stefan Sitter [:ssitter]]

Lightning doesn't support multiple windows/tabs, see e.g. Bug 332401.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This is pretty easy to hit on and is important if TB 3 were to incorporate Lightning. Confirming as well.

(This also depends on the tab implementation in TB 3)

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

I've placed some details up on the moz-wiki for tab integration.  The details would be blocking bug 446306 to be implemented.

https://wiki.mozilla.org/Thunderbird:Calendar_Integration#Initial_Integration_with_Tabs

Comment 5

[Simon Paquet [:sipaq]]

Bryan, do I understand this correctly, that you want TB to have two toolbars (Mail and Calendar), when the Calendar tab is active?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

If I'm understanding the mockups correctly, there don't appear to be any sort of visual cues about upcoming tasks/events nor a mini-month.  I'd be interested in understanding more about the thinking behind removing those things...

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #5)
> Bryan, do I understand this correctly, that you want TB to have two toolbars
> (Mail and Calendar), when the Calendar tab is active?

I think with the extra space of the tab and toolbar we can't have a usable calendar.  The best approach is the start removing the toolbar items as shown in the future mockups.  There's already progress on a new toolbar prototype like what is in the mockups. 

(In reply to comment #6)
> If I'm understanding the mockups correctly, there don't appear to be any sort
> of visual cues about upcoming tasks/events nor a mini-month.  I'd be interested
> in understanding more about the thinking behind removing those things...

Are you talking about the today pane?  If not there are some ideas to place more calendar and task features in the Folder Pane itself.  Take a look at the following mockup which gives some minimal jump points, similar options can be made available for tasks and even contacts.

https://wiki.mozilla.org/Image:New-folder-pane-w-expanded-calendar-tasks.png

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

The current tab implementation limitations affect ThunderBar too.  I will take this bug and attempt to provide patches to calendar to use the to-be-revised tab model.  Calendar people, please speak up if you already have an effort underway.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Attachment: a first pass at integrating lightning into a revised thunderbird tab model, includes thunderbird tab changes too

Here is my current patch, including both the Thunderbird changes and the lightning changes.  My tree was apparently last updated 9/26, so I would not be surprised if that does not apply cleanly to the trunk.

The only deficiency I am aware of is that if you add new tasks, they don't show up until you change tabs.  I do not know if I introduced this or it was already there.

I don't think it's review time yet, but I'd appreciate preliminary feedback from calendar folks.

Comment 10

[Simon Paquet [:sipaq]]

Wow, +660 lines and -1420 lines. Good cleanup :)

I see however, that you're removing the mode toolbar. Does your patch already include Bryan's thoughts as outlined in bug 218999 comment #89?

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #10)
> I see however, that you're removing the mode toolbar. Does your patch already
> include Bryan's thoughts as outlined in bug 218999 comment #89?

approximately, see below

(Quoting from bug 218999 comment #89)
> Seems like the initial stages are this (in order):
> 1) getting the tab bar to show by default
> 2) naming the default tab bar (i keep using All Mail, even though I think it
> should just sail Mail - bike sheds are best colored aquamarine)
> 3) add new tab button for opening a new tab
> 4) add tab shortcut area for opening different tab types

1) yes, done
2) yes, but the initial tab gets named after the folder it is displaying, if any.  This is primarily for consistency of implementation; I'm not sure what differentiates one folder view from another view, apart from I made it impossible to close that default tab.
3) no, primarily because I think Bryan is thinking about having a blank tab for the default add case, and I didn't want to get into that yet.
4) yes.  I added the small icons from the mode bar for calendar and tasks to the right of the tab bar.  Clicking on them will open a calendar/task tab if they don't exist yet and bring you to them if they do exist.  I didn't want to get into attempting to have multiple calendar tabs at this time.

I will update the patch; per davida it has bitrotted.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v2 first pass at integration, just un-bit-rotting

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Attachment: screenshot of tabs with shortcut buttons, linux

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 341994 [details] [diff] [review]
v2 first pass at integration, just un-bit-rotting

Okay, let's get the review ball rolling.  berend for the calendar parts, philor for the thunderbird parts.

Comment 15

[Berend Cornelius]

Comment on attachment 341994 [details] [diff] [review]
v2 first pass at integration, just un-bit-rotting

The patch works as promoted  and looks very good. As we discussed on irc we should get it in as fast as possible and then tidy up the code around it consecutively.
I appreciate that you hung yourself so deep into the calendar project code and removed so much dispensible code.

Comment 16

[Phil Ringnalda (:philor)]

Comment on attachment 341994 [details] [diff] [review]
v2 first pass at integration, just un-bit-rotting

Feels scary-late to be throwing things at the tree and iterating, but I don't know how else we're going to get there. r=me with the nits from IRC, which I think were:

* whitespace from http://beaufour.dk/jst-review/?patch=341994
* turn the comment about "we need to create the rep for the tab that already exists" into either something easier to comprehend, or an involved joke about the tab getting rid of a bad reputation
* for each over arrays still considered harmful, sucks to be you (and to be all the rest of us, too, of course)
* do what you meant to with the commented-out t.minWidth in openTab
* document the fun bits about putting the panelcontainer in the XUL so it can have children in a more thorough way than just by having a commented-out <tabpanels/> in the XBL.

And in general: dude, you rock. I already feel like I understand the new code better than I did the old.

Comment 17

[Phil Ringnalda (:philor)]

Oh, and the nit I forgot, a couple of instances of a space before the /> in self-closing elements. Curse you, fake-XHTML, for making that seem so natural to all our fingers!

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v3 integration with philor changes and more documentation [checked-in comment #19]

Made philor's requested changes, bulking up the documentation for the tabmail binding while I was at it.  Carrying forward r=philor, r=berend.

Comment 19

[Mark Banner (:standard8)]

Checked in, changeset id: 560:85b938c0ab77

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 342401 [details] [diff] [review]
v3 integration with philor changes and more documentation [checked-in comment #19]

I am going to leave this open and for davida/drivers to decide when to close, since this is a tb-integration-blocking bug.  Also, more polish will be required at the minimum.  (I believe the calendar toolbar has background styling issues on OS X.)  I suspect we may want to spin most polish issues off onto their own bugs that block this bug?

bienvenu: you may still want to take a look at this patch/changeset and sanity check things.

Comment 21

[Stefan Sitter [:ssitter]]

> (I believe the calendar toolbar has background styling issues on OS X.)

See Bug 392333.

Comment 22

[Mark Banner (:standard8)]

Removed unused files (calendar/lightning/content/customize-toolbar.js calendar/lightning/content/customize-toolbar.xul calendar/lightning/content/lightning-today-pane.js) in Changeset id: 561:033794801913

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #21)
> > (I believe the calendar toolbar has background styling issues on OS X.)
> 
> See Bug 392333.

It's worse than that.  I moved the calendar/tasks toolbar into the tab area for lightning (clarkbw!).  On OS X, toolbars have different background colors than content areas, at least some of the time.  So it looks funky.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

Attachment: make non-message-folder errors not happen/show up in the error console [checked in comment #27]

if you click on non-message-folders in the folder tree, certain globals will be null, resulting in errors.  Requesting bienvenu review for this and also any related systemic oversights I may have made :)

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

Attachment: [checked in] remove lightning/calendar strings that are no longer used as a result of the committed patch

The customize toolbar strings are no longer relevant.  The menu option to toggle the mode toolbar is also moot, but it lived in calendar proper's strings rather than lightning's.  (The strings for the mode toolbar are still used, though.)

Comment 26

[David :Bienvenu]

Comment on attachment 342407 [details] [diff] [review]
make non-message-folder errors not happen/show up in the error console [checked in comment #27]

this looks OK, other than a slight indentation issue in the second part of the patch (maybe a -w issue). I've pulled from the repo and will try things out.

Comment 27

[David :Bienvenu]

Comment on attachment 342407 [details] [diff] [review]
make non-message-folder errors not happen/show up in the error console [checked in comment #27]

fix checked in changeset:   563:42022c4cfd55

Comment 28

[Simon Paquet [:sipaq]]

Comment on attachment 342409 [details] [diff] [review]
[checked in] remove lightning/calendar strings that are no longer used as a result of the committed patch

r=sipaq
Thanks for the cleanup.

Comment 29

[David :Bienvenu]

There is still one issue I see - if I have a non-folder selected, and then open an other folder in a new tab using the context menu, the original tab gets confused about what to load when I switch back to it. It loads the same folder that was opened in the new tab, but has the tab title is the non-folder.

It's great that you're improving this code! I'm looking forward to tab session store :-)

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

I'm seeing occasional instances of:

Error: Ci is not defined
Source File: chrome://messenger/content/mailWindowOverlay.js
Line: 1489

in my error console that I suspect are related to this patch.

Comment 31

[Phil Ringnalda (:philor)]

Ack. There was a whole chunk of review comments around that code in showTab that slipped my mind: the Ci, the busted whitespace on the brackets, and that big wad of try/catch{dump} that needs to just try what it expects might fail, and needs to catch just the error it expects to catch.

Comment 32

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #26)
> (From update of attachment 342407 [details] [diff] [review])
> this looks OK, other than a slight indentation issue in the second part of the
> patch (maybe a -w issue). I've pulled from the repo and will try things out.

Yeah, it was particularly a bad call to use -b; I would have been better off leaving the trailing whitespace fixes in.  (Better would have been to have done the legwork to lose the spurious changes.)

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #31)
> Ack. There was a whole chunk of review comments around that code in showTab
> that slipped my mind: the Ci, the busted whitespace on the brackets, and that
> big wad of try/catch{dump} that needs to just try what it expects might fail,
> and needs to catch just the error it expects to catch.

Most of the try/catch stuff is inherited.  I'll try and figure out what the original code was concerned about and defuse the blocks.

For Ci, can I just provide a patch that makes sure the first javascript file loaded by messenger.xul defines the Cc/Ci/Cr/Cu aliases?  We'd put it right after globalOverlay.js and everyone could be happy.

Comment 34

[Phil Ringnalda (:philor)]

Heh. No. If you want to file and take a bug for that, I'll try to list as many of the problems as I know (hiddenWindow, AB, toolkit, interacting with SeaMonkey devs who think it's evil, bound to be more), and if you solve them I'll love you forever, but for now, a Components.interfaces patch would be lovely.

For inherited try/catch, you're off the hook, jminta's already got a bug for that - probably what I was supposed to remember last night before I got distracted was to look at whether or not that was inherited.

Comment 35

[Andrew Sutherland [:asuth] (he/him)]

Attachment: fix bad whitespace introduced by v3's use of -b in patch generation

Comment 36

[Andrew Sutherland [:asuth] (he/him)]

Attachment: [checked in] make Ci be Components.interfaces again (introduced by v3 patch)

Comment 37

[Martin Schröder [:mschroeder]]

Comment on attachment 342409 [details] [diff] [review]
[checked in] remove lightning/calendar strings that are no longer used as a result of the committed patch

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/36eceaf394eb>

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 342552 [details] [diff] [review]
fix bad whitespace introduced by v3's use of -b in patch generation

+            <calendar-decorated-day-view id="day-view" flex="1"
+                                               context="calendar-view-context-menu"
+                                               item-context="calendar-item-context-menu"/>
+            <calendar-decorated-week-view id="week-view" flex="1"
+                                               context="calendar-view-context-menu"
+                                               item-context="calendar-item-context-menu"/>
+            <calendar-decorated-multiweek-view id="multiweek-view" flex="1"
+                                               context="calendar-view-context-menu"
+                                               item-context="calendar-item-context-menu"/>
+            <calendar-decorated-month-view id="month-view" flex="1"
+                                               context="calendar-view-context-menu"
+                                               item-context="calendar-item-context-menu"/>

I think the second and third lines need lining up with the id they of the first line they belong to.


   <toolbox id="calendar-toolbox">
     <toolbarpalette id="CalendarToolbarPalette">
   <!-- All toolbar buttons that messenger-overlay-toolbar.xul wishes to include
-     *must* go either into the calendar-toolbar.inc file (all toolbar buttons
-     shared with Sunbird) or lightning-toolbar.inc file (toolbar buttons
-     relevant only for Lightning). -->
+       *must* go either into the calendar-toolbar.inc file (all toolbar buttons
+       shared with Sunbird) or lightning-toolbar.inc file (toolbar buttons
+       relevant only for Lightning). -->
 #include ../../base/content/calendar-toolbar.inc
 #include lightning-toolbar.inc
-  </toolbarpalette>
+    </toolbarpalette>
   
nit: I think the <!-- should probably be two space indented from <toolbarpalette...> ?

Comment 39

[Andrew Sutherland [:asuth] (he/him)]

Attachment: [needs bitrot fix] v2 whitespace fixes, resolve Standard8's action items

Comment 40

[Mark Banner (:standard8)]

Comment on attachment 342553 [details] [diff] [review]
[checked in] make Ci be Components.interfaces again (introduced by v3 patch)

Checked in, changeset id 604:97d3a55be970

Comment 41

[Mark Banner (:standard8)]

Comment on attachment 342567 [details] [diff] [review]
[needs bitrot fix] v2 whitespace fixes, resolve Standard8's action items

This patch is bitrotted :-(

Comment 42

[David :Bienvenu]

p1, since tabs are very important to calendaring (though I'm not sure how much of this bug is actually left)

Comment 43

[Magnus Melin [:mkmelin]]

Andrew: what part is actually left here?

Comment 44

[Martin Schröder [:mschroeder]]

(In reply to comment #43)
> Andrew: what part is actually left here?

Last patch (attachment#343567 [details]) needs to be de-bitrotted.

Comment 45

[Martin Schröder [:mschroeder]]

(In reply to comment #44)
> (In reply to comment #43)
Oops... it's the patch attachment#342567 [details] [diff] [review].

Comment 46

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

I'll take care of de-bitrotting this one.

Comment 47

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

Attachment: [checked in] v3 whitespace fixes, resolve Standard8's action items

Debitrot, fixes more whitespaces in calendar code. Requires patch from bug 456379.

Does this need a further review, or can I check it in as is after the dependant bug is fixed?

Comment 48

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 350208 [details] [diff] [review]
[checked in] v3 whitespace fixes, resolve Standard8's action items

Thank you for de-bitrotting!  As long as the patch still applies cleanly to mail/, I don't think any additional thunderbird review should be required; it's just whitespace.

Comment 49

[Martin Schröder [:mschroeder]]

Andrew, Philipp, who will land the patch, or do we need checkin-needed because there is code outside /calendar in the patch?

Comment 50

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

This patch required another patch in my queue.

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/b74725908891>

-> FIXED

Comment 51

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

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".

Comment 52

[Jens Müller (:tessarakt)]

The documentation added in Attachment 342401 [details] [diff] does not accurately describe the implementation in the same patch, see bug 903665.