Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[Trunk] Tabs break calendar mode view

RESOLVED FIXED in 1.0b1

Status

Calendar
Lightning Only
P1
normal
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: Omar Bajraszewski, Assigned: Fallen)

Tracking

(Depends on: 1 bug)

Dependency tree / graph
Bug Flags:
blocking-calendar1.0 +
tb-integration +

Details

Attachments

(8 attachments, 4 obsolete attachments)

54.85 KB, image/jpeg
Details
76.75 KB, image/jpeg
Details
9.85 KB, image/png
Details
131.49 KB, patch
asuth
: review+
Details | Diff | Splinter Review
1.47 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
2.63 KB, patch
sipaq
: review+
Details | Diff | Splinter Review
826 bytes, patch
standard8
: review+
Details | Diff | Splinter Review
48.32 KB, patch
asuth
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
Created attachment 287234 [details]
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)
(Reporter)

Comment 1

10 years ago
Created attachment 287236 [details]
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

10 years ago
Lightning doesn't support multiple windows/tabs, see e.g. Bug 332401.
(Reporter)

Updated

10 years ago
Depends on: 332401
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)
Depends on: 218999
(Reporter)

Updated

9 years ago
Flags: tb-integration?

Updated

9 years ago
Flags: tb-integration? → tb-integration+
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
Bryan, do I understand this correctly, that you want TB to have two toolbars (Mail and Calendar), when the Calendar tab is active?
OS: Windows XP → All
Hardware: PC → All
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...
(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

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.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED

Updated

9 years ago
Flags: blocking-calendar1.0+
Created attachment 341903 [details] [diff] [review]
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.
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?
(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.
Created attachment 341994 [details] [diff] [review]
v2 first pass at integration, just un-bit-rotting
Attachment #341903 - Attachment is obsolete: true
Created attachment 341996 [details]
screenshot of tabs with shortcut buttons, linux
Attachment #341994 - Flags: review?(Berend.Cornelius)
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.
Attachment #341994 - Flags: review?(philringnalda)

Updated

9 years ago
Attachment #341994 - Flags: review?(Berend.Cornelius) → review+

Comment 15

9 years ago
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 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.
Attachment #341994 - Flags: review?(philringnalda) → review+
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!
Created attachment 342401 [details] [diff] [review]
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.
Attachment #341994 - Attachment is obsolete: true
Attachment #342401 - Flags: review+
Checked in, changeset id: 560:85b938c0ab77
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.
Attachment #342401 - Attachment description: v3 integration with philor changes and more documentation → v3 integration with philor changes and more documentation [checked-in comment #19]

Comment 21

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

See Bug 392333.
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
(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.
Created attachment 342407 [details] [diff] [review]
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 :)
Attachment #342407 - Flags: review?(bienvenu)
Created attachment 342409 [details] [diff] [review]
[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.)
Attachment #342409 - Flags: review?
Attachment #342409 - Flags: review? → review?(bugzilla)
Blocks: 459203

Comment 26

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

9 years ago
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
Attachment #342407 - Flags: review?(bienvenu) → review+

Updated

9 years ago
Attachment #342409 - Flags: review?(bugzilla) → review+
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

9 years ago
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 :-)
Depends on: 459239
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.
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.
(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.)
(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.
Attachment #342407 - Attachment description: make non-message-folder errors not happen/show up in the error console → make non-message-folder errors not happen/show up in the error console [checked in comment #27]
Attachment #342409 - Attachment description: remove lightning/calendar strings that are no longer used as a result of the committed patch → remove lightning/calendar strings that are no longer used as a result of the committed patch [check-in needed]
Keywords: checkin-needed
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.
Created attachment 342552 [details] [diff] [review]
fix bad whitespace introduced by v3's use of -b in patch generation
Attachment #342552 - Flags: review?(bugzilla)
Created attachment 342553 [details] [diff] [review]
[checked in] make Ci be Components.interfaces again (introduced by v3 patch)
Attachment #342553 - Flags: review?(bugzilla)
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>
Attachment #342409 - Attachment description: remove lightning/calendar strings that are no longer used as a result of the committed patch [check-in needed] → [checked in] remove lightning/calendar strings that are no longer used as a result of the committed patch
Keywords: checkin-needed
Attachment #342552 - Flags: review?(bugzilla) → review+
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...> ?
Attachment #342553 - Flags: review?(bugzilla) → review+
Created attachment 342567 [details] [diff] [review]
[needs bitrot fix] v2 whitespace fixes, resolve Standard8's action items
Attachment #342552 - Attachment is obsolete: true
Attachment #342567 - Flags: review+
Attachment #342553 - Attachment description: make Ci be Components.interfaces again (introduced by v3 patch) → [needs check-in] make Ci be Components.interfaces again (introduced by v3 patch)
Keywords: checkin-needed
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
Attachment #342553 - Attachment description: [needs check-in] make Ci be Components.interfaces again (introduced by v3 patch) → [checked in] make Ci be Components.interfaces again (introduced by v3 patch)
Comment on attachment 342567 [details] [diff] [review]
[needs bitrot fix] v2 whitespace fixes, resolve Standard8's action items

This patch is bitrotted :-(
Attachment #342567 - Attachment description: [needs check-in] v2 whitespace fixes, resolve Standard8's action items → [needs bitrot fix] v2 whitespace fixes, resolve Standard8's action items
Keywords: checkin-needed

Comment 42

9 years ago
p1, since tabs are very important to calendaring (though I'm not sure how much of this bug is actually left)
Priority: -- → P1

Comment 43

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

Last patch (attachment#343567 [details]) needs to be de-bitrotted.
(In reply to comment #44)
> (In reply to comment #43)
Oops... it's the patch attachment#342567 [details] [diff] [review].
(Assignee)

Comment 46

9 years ago
I'll take care of de-bitrotting this one.
Assignee: bugmail → philipp
(Assignee)

Comment 47

9 years ago
Created attachment 350208 [details] [diff] [review]
[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?
Attachment #342567 - Attachment is obsolete: true
Attachment #350208 - Flags: review?(bugmail)
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.
Attachment #350208 - Flags: review?(bugmail) → review+
Andrew, Philipp, who will land the patch, or do we need checkin-needed because there is code outside /calendar in the patch?
(Assignee)

Comment 50

9 years ago
This patch required another patch in my queue.

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

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
(Assignee)

Updated

9 years ago
Attachment #350208 - Attachment description: v3 whitespace fixes, resolve Standard8's action items → [checked in] v3 whitespace fixes, resolve Standard8's action items
(Assignee)

Comment 51

6 years ago
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".
Target Milestone: 1.0 → 1.0b1
The documentation added in Attachment 342401 [details] [diff] does not accurately describe the implementation in the same patch, see bug 903665.
You need to log in before you can comment on or make changes to this bug.