386589 - Promote the prototype event dialog as standard dialog

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Michael Büttner (no reviews TFN)]

The prototype event dialog is currently not enabled by default. We agreed on switching it on after the 0.5 release is out of the door. This patch serves to address this issue.

Comment 1

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

I'm just unconditionally passing the chrome-url that refers to the prototype event dialog, instead of asking the pref for that. I'm keeping the pref for now since there are other features tied to it.

Comment 2

[Joey Minta]

Isn't this bug blocked by bug 370435?  There are a bunch of substantive comments there that, to my knowledge, haven't been addressed.

Comment 3

[Michael Büttner (no reviews TFN)]

(In reply to comment #2)
> Isn't this bug blocked by bug 370435?  There are a bunch of substantive
> comments there that, to my knowledge, haven't been addressed.
There was the decision made to promote the prototype dialog after 0.5 has been released. I will address any bits and pieces that might come up and bug #370435 will also be addressed. But from what I understood we all agreed to switch it on now. I don't want to hold this off any longer, there's simply no reason for doing so.

Comment 4

[Simon Paquet [:sipaq]]

Yes, the decision was made (IIRC on the Hamburg F2F meeting or shortly thereafter in a confcall). We should proceed.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

No, the decision was not that simple. We would review the dialog, apply the changes, and then turn it on by default for a while. We estimated that the review would be done by the time 0.5 was released. But that didn't happen. Well, there were some review comments, but they were not applied.
I'd say that we should stick to the order we agreed, so to apply the review comments before turning on the dialog by default.

Comment 6

[Michael Büttner (no reviews TFN)]

This was not my understanding of what was agreed. The latest decision was that the dialog gets switched on after 0.5 is out of the door, no further condition has to be met. I really don't understand why there's so much discussion about this dialog going on. The comments on the blogpost where absolutely positive and everybody seems to like the new dialog. I really don't understand why we're again starting on this discussion. I already said that I'm going to address all the missing bits and pieces. By the way, those missing stuff was absolutely minor and I didn't have the impression that's what's hindering us from using it. I'm working full-time on this project, so this stuff *will* be addressed. I'm also working on other features as well, more important features, by the way. Moving source code around isn't my main concern. Furthermore, I propose that this should be discussed tomorrow on the weekly meeting.

Comment 7

[Joey Minta]

(In reply to comment #6)
> Furthermore, I propose
> that this should be discussed tomorrow on the weekly meeting.

I have a full-time job so I obviously can't attend the meeting, but I hope to see some of the following addressed:


> I really don't understand why there's so much discussion about
> this dialog going on. 

The main reason I brought this up was because you seem to keep changing the agreement.  When the prototypes/ directory was introduced, it was on the agreement that

(1) Lightning and Lightning-wcap have different target audiences and the prototypes folder let's the secondary audience be addressed.

At that point it was made explicit that "The prototypes/ directory is not an end-run around the review system."

From there, we saw the different use-case agreement rejected in favor of

(2) The use-cases are sufficiently similar that we should migrate features from the prototype dialog to the standard dialog.

This agreement was the root of bug 367893.  It was agreed that we move the features individually, once they are reviewed.

This agreement was then rejected in favor of

(3) We're not moving features, let's just turn the dialog on once it's been reviewed.

This review began (but was not completed) in bug 370435.

This agreement has now apparently been rejected as well.

If the above were not enough, there are compelling reasons for not taking the next step to 
(4) The prototype dialog should be landed without review.
The review system exists for a reason.  Reviews must be done and comments must be addressed prior to checkin, in order to align the interests of the developer (wants to land the patch) and the community (wants the review comments addressed).  After the patch has landed, these interests are no longer aligned.  See http://www.mozilla.org/hacking/code-review-faq.html#13  This is why, for instance, toolkit/ changes cannot land without an associated automated test.  Even if the developer is "working full-time" and promised the tests *will* be written, there are no exceptions.  

There are no exceptions to the review process either.  There are no exceptions for features that people really like.  There are no exceptions for features that have been heavily tested.  There are no exceptions for big patches.  There are no exceptions.  Full stop.  The system is more important than any particular patch, and flouting it here is a mistake.

cc'ing dmose so he can call me out on any liberties I may have taken with the facts.

Comment 8

[Michael Büttner (no reviews TFN)]

(In reply to comment #7)
You did not understand what this bug is all about. It isn't my intention to circumvent the review process. The prototype dialog lives in the prototypes folder an will stay there until the review process has been completed. Full stop. I don't think we need to discuss that any further.

I had the impression that we all agreed that we flip the switch and make the prototype dialog the default dialog. The code stays where it is, the standard dialog is still there and the prototype dialog also stays where it is. After this step we proceed with the outstanding bits and pieces that needs to be addressed. If we successfully accomplished this step, we move the code from the prototypes folder to its final destination.

This is what we agreed on, the decision was made on the Hamburg F2F meeting with Clint, Matt, Daniel and myself sitting on the table (besides the rest of the attendees of that meeting). Since you're no longer involved into this project I doubt that you're in a position to challenge this decision.

Generally, I don't like to be told that I'm playing against the rules, even worse, that this is my intention. I respect the way we're developing software and I even find that it is one of the best ways of doing so that I've ever come across. My goal is to help making this application successful, I'm putting a hell of a lot of effort into achieving this goal. So I'm feeling particular offended by being told that I'm intentionally doing something wrong.

I don't want to continue this discussion here. The patch has been attached to this bug and I'm waiting for the module owner or some other authority to decide on how we shall proceed.

Comment 9

[cmtalbert]

(In reply to comment #8)
> I had the impression that we all agreed that we flip the switch and make the
> prototype dialog the default dialog. The code stays where it is, the standard
> dialog is still there and the prototype dialog also stays where it is. After
> this step we proceed with the outstanding bits and pieces that needs to be
> addressed. If we successfully accomplished this step, we move the code from the
> prototypes folder to its final destination.

This was in fact the decision made at the F2F meeting.  The idea was to go ahead and switch the prototype dialog on by default so that the nightly testing community can begin testing the new dialog more broadly in order to find out what else will need to be fixed w.r.t. it.  Obviously, the review process must still be completed in order to get the dialog working on Sunbird and those comments must be addressed. Bug 371917 has a new patch that addresses the comments, and I don't think Mickey is trying to circumvent any established protocol here.  To me, it seems like we are pushing forward on two different fronts in two different bugs.  There will be ongoing support for Sunbird and the prototype dialog, I think bug 371917 is only the tip of the iceberg there.  However, until the nightly testers can easily test this dialog with Sunbird, the remaining issues are going to remain in the shadows.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

The plan that I rememeber was this:
1) Review the event dialog, not only code-wise, but also the UI. This should be done before 0.5
2) Turn on the dialog by default for a while (after 0.5), to get some more user feedback
3) Update the dialog for the feedback
4) turn on by default
(this plan is what jminta described as number 3)

Now, we didn't make step 1. There are some comments, both on the code and on the UI. But the code is not updated, as far as i can see. At least, there was no feedback in the bug.
The question now is: should we proceed with step 2, even if we didn't make step 1 in time?

In short: Yes, we agreed to turn on the dialog after 0.5. BUT that is not all of the agreement. There is more.



Ok, those are the facts as I remember them. Now for my personal opinion:
There are a few things in the dialog that I still think that need to be fixed before the dialog is ready for use by the general public. (see [1] and bug 370435.)
The fact that 0.5 is now out of the door doesn't change the order in which we agreed that we would do things, and I still think that we should stick to that.


[1] http://groups.google.com/group/mozilla.dev.apps.calendar/msg/7620c7e8c7c5d6d


btw, making the dialog work in sunbird (bug 371917) is really a blocker to this bug imo.

Comment 11

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

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

>-    var url = "chrome://calendar/content/calendar-event-dialog.xul";
>+    url = "chrome://calendar/content/sun-calendar-event-dialog.xul";

Minor Nit: Missing |var| before url.

Aside from the discussion, which I do not intend to circumvent, this patch does what it is supposed to. r=philipp when a final decision is made.

Comment 12

[Simon Paquet [:sipaq]]

Joey, mvl,
as Mickey and Clint have already said, this was discussed at the Hamburg F2F meeting (which you did not attend for various reasons) and it was decided unanimously by everyone who was there (Daniel, Mickey, Christian from Sun, Matt, Clint, Philipp and me from outside Sun) that we should switch to the new dialog shortly after the 0.5 release and address the open issues after that. 

There's nothing more to discuss here IMO, since I have seen no indication, that one or more of the people involved have changed their opinion since then.

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Sorry, but the review comments were made almost half a year ago. What reasons do we have to assume that they will now suddenly get addressed real soon? If this needs to be done _now_, why didn't anybody look into the reviews before?

And I _AM_ one of the involved. And I didn't change my mind either. Why does my opinion (and that of joey) suddenly lost its value? Just because we couldn't make it to the meeting?

Comment 14

[Daniel Boelzle [:dbo]]

Back from vacation, I feel sorry reading the above. IMO it shows that we (the F2F attendees) should have documented more background to the decision on the F2F:
Please have in mind that the new dialog has been throughly designed by Christian and gone thru several iterations inside Sun. It has already been proven usable and sensible for more than half a year now to a bigger public, e.g. Simon has blogged about it and people are eagerly waiting for it. Thus we don't need a formal UI-review anymore; Christian has UI authority for good reason and has done that job for several years now.
There is no disagreement that the vast majority of the new design is accepted, and switching it on now puts more pressure to finally resolve the outstanding issues that mvl, joey and *others* (who probably never switch the prototypes pref by themselves) raise. We need to resolve all those issues for 0.7, IMO those block. As mickey stated, the dialog will be finalized in the prototypes folder, the current dialog isn't lost.
I hope this helps understanding the proceeding.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

There are two things to this:

- The UI. 
I do believe that the UI is tested and reveiwed etc for use withing lighting, likely with wcap turned on. But there are open issues for use in sunbird and without wcap (and ldap and addressbook etc). Those should be addressed. (this is what comment 14 talks about)

- The code. 
There are a whole bunch of code-level review comments. The must be addressed too. (but this part is neglected in comment 14. but it's just as important)


Why can't those issues be addressed first, before asking for more comments? What good is there in asking for testers for code that already has known problems? I have to admit that I don't believe that turning on the dialog will really make anybody fix the issues. The only pressure that I can see that I can apply to make the issues be fixed is blocking turning on the dialog. Delaying the blocking to a release is just too risky: it's too likely that we will be in a hurry then, and the blocking be removed, and the fixes never come.

Comment 16

[Daniel Boelzle [:dbo]]

(In reply to comment #15)
> - The UI. 
> I do believe that the UI is tested and reveiwed etc for use withing lighting,
> likely with wcap turned on. But there are open issues for use in sunbird and
> without wcap (and ldap and addressbook etc). Those should be addressed. (this
> is what comment 14 talks about)
Isn't that covered by bug 371917?

> - The code. 
> There are a whole bunch of code-level review comments. The must be addressed
> too. (but this part is neglected in comment 14. but it's just as important)
> 
> 
> Why can't those issues be addressed first, before asking for more comments?
> What good is there in asking for testers for code that already has known
> problems? I have to admit that I don't believe that turning on the dialog will
> really make anybody fix the issues. The only pressure that I can see that I can
> apply to make the issues be fixed is blocking turning on the dialog. Delaying
> the blocking to a release is just too risky: it's too likely that we will be in
> a hurry then, and the blocking be removed, and the fixes never come.
We should bring it in early (even without full code review) to gather further feedback and stabilize the dialog which will *most likely* replace the existing one in 0.7. I am absolutely sure Mickey will fix the outstanding problems for 0.7 and complete the code review. I don't know the exact status of the current code review, but I doubt that the code is of low quality.
I won't go for another 6-12 weeks until the last UI discussions have been resolved and the code may has been reviewed; this practically don't work. The past has shown that even then real life shows up the real bugs.
However, the current dialog isn't lost for 0.7, so if it turns out that my assumptions have been too optimistic, we can switch back e.g. in september.

Comment 17

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 18

[Martin Schröder [:mschroeder]]

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6pre) Gecko/20070726 Calendar/0.7pre and Tb 2.0.0.6pre (20070728) + Lightning 0.7pre (2007072704) on WinXP.