386714 - Encapsulate Lightning-View-Deck in vbox, as it is in Sunbird

Status: VERIFIED FIXED

(Calendar :: Lightning Only, enhancement)

Description

[Sven Giermann]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: Lightning 0.5 (build 2007062404)

The current implementation of '<deck id="calendar-view-box">' does not provide ability to change the look of lightning to be the same as Sunbird.

My Proposal is to introduce a surrounding vbox that can be modified by overlays.

Unfortunately I was unable to move the deck dynamically without loosing the XML bindings... Maybe one can accomplish this and give me a hint?!


Reproducible: Always

Comment 1

[Sven Giermann]

Attachment: Rename Lightning deck to 'view-deck' and put it into vbox 'calendar-view-box'

I chose the same IDs that Sunbird uses and did not find any problems on TB 1.5.0.10 and TB 2.0.0.4 on WinXP SP2.
I also think that this would reduce the overhead to overcome differences between Lightning and Sinbird. (i.e. in getViewDeck)

But I did not check, whether the different IDs are used to workaround some other problems.

Comment 2

[Sven Giermann]

Attachment: Corresponding patch for messenger-overlay-sidebar.js

Sorry, I forgot to attach the patch for messenger-overlay-sidebar.js to use the unified getViewDeck() instead of the Lightning ID to toogle Workdays-Only and Tasks-In-View.

I think this patch does not hurt, even if the other one will be rejected - it is just another unify-lighting-and-sunbird patch!

Comment 3

[Sven Giermann]

For anyone who is interested: I need this for my extension 'ltnPlus' to work:
https://addons.mozilla.org/thunderbird/addon/5285

Comment 4

[Michael Büttner (no reviews TFN)]

Comment on attachment 270715 [details] [diff] [review]
Rename Lightning deck to 'view-deck' and put it into vbox 'calendar-view-box'

> <deck id="displayDeck">
>-  <deck flex="1" id="calendar-view-box">
>+ <vbox id="calendar-view-box">
>+  <deck flex="1" id="view-deck">
In general each attempt to further consolidate Sunbird and Lightning code gets a warm welcome, but this particular attempt is a bit half-hearted. Just have a look at getViewDeck(), see [1]. This function could be condensed into a one-liner, there wouldn't be the need to check for both names. Furthermore, both names ("view-deck" and "calendar-view-box") are still used throughout the code. This change wouldn't cut down the code but would increase the number of DOM nodes. You could flip this relationship by making the name equal (I assume calendar-view-box is the better choice) and consolidate all the code that differentiates between them. I'd love to immediately check-in a patch that does just that.

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/content/calendar-views.js#291

Comment 5

[Sven Giermann]

Well, I think it's not that easy.

In Sunbird there are some differences between getting the "calendar-view-box" and the "view-deck".
Even in Lightning it would be better to have both, because the "calendar-view-box" would mean the whole (main-) display, that Lightning is able to use. The deck only displays one of its childs.

Imagine someone is going to write an extension to make the view-deck shoing tabs to switch between views. To my mind this would not be possible currently, because you would have to overlay each single view (month/day/week).
If there would be a surrounding "something" (i.e. vbox) you would be able to position something before and/or after the view-deck.

Maybe this request is too related to bug 372829, I don't know ;-)

Comment 6

[Michael Büttner (no reviews TFN)]

I don't want to reject the request, I just tried to squeeze a bit more out of this patch ;-) I really would like to see all the bits and pieces addressed that can benefit from the adjusted xul layout. Please let me know if you're going to do that or if we should get this one done and file a spin-off bug.

Comment 7

[Sven Giermann]

Mickey,

I just read your comment [1] to bug 376086 and would please you to check-in attachment 270885 [details] [diff] [review] and the line you discussed there...

Although it's Lightning only code in both cases, I would prefer to also differentiate wheter one wants to get the displayed element in 'displayDeck', or exactly the deck holding the different calendar views.

Unfortunately I don't have any cvs installed here, so I am unable to get the checked-in version of bug 376086 to create a complete patch, but I think it's not too difficult to find the two lines addressed in attachment 270885 [details] [diff] [review] and the one inserted by attachment 272015 [details] [diff] [review].

Regarding the added DOM node: I didn't count them, but I think only one more would be okay ;-)
In fact I did not understand exactly what you mean with "making the name equal"... Do you want to reduce the two DOM nodes to one; even in Sunbird?
I don't think that this would be possible.

And to answer your question, I don't feel able to address your request, so please file another bug for that.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=376086#c23

Comment 8

[Michael Büttner (no reviews TFN)]

Would you please be so kind explaining the rationale behind this request? I really don't get it. As far as I understood the discussion you really *want* to have a deck inside of a vbox instead of a deck with a unified name. Could you please elaborate on this? To make this clear, as far as I see it we have these alternatives:

1) <vbox id="calendar-view-box">
    <deck flex="1" id="view-deck">
    </deck>
   </vbox>

2) <deck flex="1" id="view-deck">
   </deck>

Of course I don't case about the number of DOM nodes, but if there's no compelling reason I would always opt for the second version. Where's the need for the additional vbox? All I tried to say is, that it would buy us much in terms of code consolidation if we strive for the second version and adapt the code.

As far as I see it, there are two options for this bug. Either it will be accepted and checked-in (with your first patch) or it will be WONTFIX. I'll address the code consolidation in a separate bug. But please explain exactly what's the benefit of the additional vbox.

Comment 9

[Sven Giermann]

The first reason to introduce the additional vbox was my failure to add the Unifinder event list to Lightning (which I will explain in detail later).

But another point is, that I think it's not very useful to have just a deck inside another deck (displayDeck)...
The problem is, that a deck only displays one of its childs at one moment. So if you have just one element, you cannot extend this by extensions.
Please have a look into messenger.xul [1], there is also an surrounding vbox for the 'accountCentralPane'!

By this it is possible to extend the calendar view by extensions.
The most simple one would be, to just display an explanation on top of the window "You are now in calendar-view, press 'Mail' button to return to your mails..."
But this is not possible, if the only element is a deck again...

Now let's explain my problems to understand me right:
I tried to change the ID of the 'calendar-view-box' to 'view-deck', then to append my vbox with id 'calendar-view-box' as a child of 'displayDeck' and to move the 'view-deck' into this vbox.
As this should work by design, I realized, that the XML bindings seem to have problems with this, as they produce errors after that and nothing is being displayed in calendar view.
When I do the modification as outlined in the patch, I do not have to do it dynamically and it works.

Don't get me wrong: I do not want to workaround my poor programming abilities, but I think this would be the right way, as it seems to be a common practice to have boxes inside of decks.

[1] http://lxr.mozilla.org/mozilla/source/mail/base/content/messenger.xul#312

Comment 10

[Michael Büttner (no reviews TFN)]

First of all, after thinking about the rationale for some time now, I find it quite reasonable. The reason that extensions might want to hook into the calendar view without just being another child node of the deck is a scenario that could well be realistic.

Second, as soon as I tried the patch I found that it gives us some chance on code consolidation without much effort. That's what I was initially talking about, but it turned out that it's much easier than I thought.

So, bottom line, I think we want to add the additional vbox.

Unfortunately, the patch as it currently stands is not complete. First, two of the fragments are split up in two distinct patches. Although, the complete version would need both fragments. This makes both attached patches incomplete and therefore it was a correct decision to reject them.

Furthermore, there's a twist in the function getViewDeck(). Even with both fragments applied, this function just works by accident. The function asks the tree for 'view-deck' and 'calendar-view-box'. With the patch applied, both elements exist, and the function succeeds just because 'return sbDeck || ltnDeck' now always returns the 'sbDeck'. This function definitely should be modified to accommodate the new tree layout. Functions that just work by accident are a no-no.

Last but not least, the patch breaks the toggleCompletedTasks() functionality.

As I'm convinced that we should introduce the additional vbox plus the fact that it gives us some chance to clean up the code, we'll go ahead with this issue.

Comment 11

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

Comment 12

[Sven Giermann]

Mickey,

thanks for your work on this and for completing my patch(es).
First I simply "forgot" to change the 2 functions I added later, second I have no working CVS settup to create "clean" patches on all related files and third I did not mention the call in toggleCompletedTasks(), because I always overlay with UnifinderToDo (see bug 372830) which makes this obsolete, sorry!

Thanks again,
Sven.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 272461 [details] [diff] [review]
patch v1

Looks good and works as advertised, only one minor style nit:

> +    for each (view in getViewDeck().childNodes) {
While you are at it, use |var view| here and elsewhere.

r=philipp

Comment 14

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 15

[Sven Giermann]

VERIFIED with Build 2007-09-23-03-mozilla1.8
(sorry for the delay...)