Closed Bug 455542 Opened 16 years ago Closed 9 years ago

Make configuration of Today Pane "Soon" duration available in preference dialog too

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
4.0.0.1

People

(Reporter: u218749, Assigned: bv1578)

References

Details

Attachments

(3 files, 8 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) Gecko/2008072820 Firefox/3.0.1
Build Identifier: version 2.0.0.16 (20080724)

It is not clear how far in the future "soon" will display events.
A few tests suggest "one week". It should be clarified.
It would also be nice if I could decide myself.

Reproducible: Always

Steps to Reproduce:
1. 
2.
3.
This proposal add a variable configurable with Config Editor so it would solve only a part of this issue.
I'm not sure if it's the wanted solution and, if it was, not sure if something is missing.
It works on Lightning 0.9 2008091821.
Attachment #339421 - Flags: review?(philipp)
Comment on attachment 339421 [details] [diff] [review]
[checked in] Proposal for a configurable "Soon" duration

Possibly you want make this dynamic. With your patch, if the pref is changed then the number of "Soon" days is not changed. 

My suggestion would be to give Synthetic a third parameter called aPrefName that allows passing a preference. Then make |duration| be a getter that checks if a pref was passed to the contsturctor and then gets the number from that preference instead of the fixed number. Alternatively, you could reuse the second parameter and check if (typeof(aDuration) == "number") to see if either a fixed number of days was passed, or a prefernce string.

Passing review to berend since he has done more work on the agenda listbox.
Attachment #339421 - Flags: review?(philipp) → review?(Berend.Cornelius)
Assignee: nobody → bv1578
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Component: Calendar Views → Lightning Only
OS: Linux → All
QA Contact: views → lightning
Hardware: PC → All
I think Decathlon's solution is fine. One thing I had in mind anyway was to add a preference observer to the today pane. For this issue I already filed  "Bug 409094 -  Today-pane does not provide a timezonechange listener" some months ago. This added preference would be an argument to push that bug forward.
Attachment #339421 - Flags: review?(Berend.Cornelius) → review+
Comment on attachment 339421 [details] [diff] [review]
[checked in] Proposal for a configurable "Soon" duration

r=berend due to my comment.
Comment on attachment 339421 [details] [diff] [review]
[checked in] Proposal for a configurable "Soon" duration

http://hg.mozilla.org/comm-central/rev/8bbc55aa6550
Attachment #339421 - Attachment description: Proposal for a configurable "Soon" duration → [checked in] Proposal for a configurable "Soon" duration
Keywords: checkin-needed
Target Milestone: --- → 1.0
Daniel: You can use the -u parameter in hg to specify the patch author, for example hg commit -u "Name <mail@example.com>". If you work with queues, you can also the -u parameter with the qrefresh command.
Thanks for the hint, Frank. If I read it correctly, the -u user will be specified as committer. Should we do so even though an author doesn't have commit/push rights (like decathlon)?
My name is not important, if the patch is right you can put the reviewer's name (if reviewer agrees ;-)) or your name as well.
Yes, with hg it should be the name of the patch author. The name of the real committer will still be visible in the pushlog, see http://hg.mozilla.org/comm-central/pushloghtml.
This proposal would solve the second part of the bug adding a drop-down box (menulist) in the Option dialog: Lightning section -> Views tab -> General groupbox as the image attached in the next comment shows.
The dropdown box position seems fine to me, others tabs or groupboxes don't look so good (maybe General tab?). Anyway it needs a ui-review.
I don't know whether "soon" duration needs a preference control in the Option dialog or it's a bit exaggerate, but why not? There is free space in the dialog window, IMHO it's better a preference control than a white space.
I've put 28 days in the menu list (four weeks) but this is a proposal, like the sentence in English language in the label, natural English speaker could suggest a better string. 
The dynamic update issue is still open as Berend Cornelius reported in comment #3. 
Since the previous patch hasn't been checked in, this patch contains the other one.

As usual I hope there will be few mistakes.
Attachment #340849 - Flags: ui-review?(christian.jansen)
Attachment #340849 - Flags: review?(Berend.Cornelius)
Attached image dropdown-box position inside Option dialog (obsolete) β€”
Why not use a textbox, allowing the user to enter any number of days? Aside from that, it might be a bit confusing:

Does "7 Days" mean 7 days in the future, or 7 days after tomorrow (i.e 9 days in the future)

I don't know a good way to solve this though. Personally I don't see the need for this in the UI though, since the average user doesn't need to set up how many days are soon.
My initial reaction was: Do we need preference UI for this at all? There should be a good default value that fits most users needs. And for the other users it can be changed via advanced preference editor.
I will wait with the review until the ui-review has been done. I personally think a preference ui is not ++very++ necessary but at helpful for this. I already thought that also other preference settings for the today-pane are thinkable, e.g.  a setting that determines whether all events of today should be displayed or only  the ones from now on. 
I am also not a friend of the implemented menulist that has so many entries and is yet restricted to a certain amount of days. I think we should either follow  Philipp's "textbox" proposal in comment #12 or reuse the years-popup menu that we already use in the minimonth. Howver to use that we should consolidate the existing implementation.
I personally like the idea of allowing users to customize "Soon", but I suggest the following changes.

1. Depending on users definition of "Soon" the string displayed in
   the "Today Pane" should change.

   e.g [+] Next 4 Days

2. The Option should be shown in an own section, below:
   Tools -> Options -> Lightning -> Views -> Multi Week View

3. I suggest to shorten the drop down list to:
   
   2 Days
   3 Days
   4 Days (Default)
   5 Days
   6 Days
   7 Days
   2 Weeks
   3 Weeks
   4 Weeks
(In reply to comment #15)
> 2. The Option should be shown in an own section, below:
>    Tools -> Options -> Lightning -> Views -> Multi Week View

Please remember that the content of the View tab already exceeds the Preference window size. See screenshot in Bug 396425 and Bug 385249.
Attached image Proposal with control in general tab (obsolete) β€”
What about a solution like this (see attachment)?
A group-box for today pane in General tab where there is a lot of space so no problem with issue in comment #16. In the group-box could be included other settings related to today pane e.g. that one in comment #14: a setting that determines whether all events of today should be displayed or only events from now on.
Just an idea.

About comment #15 point 1,
I agree with this but maybe should also be considered bug 417222 because some ideas reported there are not even going to use the header "Soon" (see bug 417222 comment #11)

About comment #15 point 3,
with fixed values the users could not customize entirely the Soon duration (I imagine that someone would set e.g. 10 days), in this case IMHO would be better a textbox (maybe with a upper bound on entered number?).
I've done changes suggested by Christian Jansen in comment #15.
Menu list contains only 9 values and is positioned in Multi Week View groupbox though I don't like so much that position.
I added a check on the preference value to avoid a misbehavior if the number is different from the options in the menu list (if preference is edited manually by the editor config). Maybe there's a more "professional" way to do this check, could you suggest something?
About the word "Next" used in the header (and the confusing generated from this section of Agenda as reported in comment #12), I meant that Next section in Agenda shows events for a range of time, days or weeks, from tomorrow included on (tomorrow is the first of the "next" days). In this way "Next 2 days" is "until the 2nd day" from tomorrow, i.e. tomorrow and the day after tomorrow; "Next 7 days" is until next Sunday (included) if today is Sunday. It seems to me better than "Soon".
As I reported in comment #17, I preferred an option to choose any day in the range, so I added the same patch with a textbox control, see attachment below.
Please review the patch you prefer and discard the other.
Attachment #340849 - Attachment is obsolete: true
Attachment #340850 - Attachment is obsolete: true
Attachment #342318 - Flags: review?(Berend.Cornelius)
Attachment #340849 - Flags: ui-review?(christian.jansen)
Attachment #340849 - Flags: review?(Berend.Cornelius)
The same patch reported above but with a text box element instead of a menu list.
Please, review only that one you prefer.
Attachment #342319 - Flags: review?(Berend.Cornelius)
(In reply to comment #18)
> [...] I meant that Next section in
> Agenda shows events for a range of time, days or weeks, from tomorrow included
> on (tomorrow is the first of the "next" days). In this way "Next 2 days" is
> "until the 2nd day" from tomorrow, i.e. tomorrow and the day after tomorrow;
> "Next 7 days" is until next Sunday (included) if today is Sunday. It seems to
> me better than "Soon".

Thanks for making the Soon section customizable.  However, I think that "Next 2 days" should mean the two days after tomorrow.  Otherwise it's too confusing IMO.  Consider that you had to write an entire paragraph to explain what it means.  When I read it my question was:  will tomorrow's events be in both the Tomorrow and the Soon sections?

I'm strongly in favor of keeping "Soon" for the name of the section.  In English at least, it's immediately obvious that this refers to the days after tomorrow (in this context), unlike "Next N Days".  "Following N Days" is better, but I think that's too long.  "Soon" is short and sweet.

Also, I remember in the past there were suggestions to use "Next Week" and "Next Two Weeks" and those were also confusing.  If 'soon' doesn't translate well, maybe there's a different word that could be used in other localizations.

I don't think it's necessary to show the number of days as part of the section name (i.e. 'next N days') because it seems that this can be pretty easily determined by looking at the events in the section, all of which will hopefully eventually be prefixed by the name of the day or be grouped by day.  Also, users who have set the number of days in Options surely know this number.
Having said that, what I would really like is to eliminate the Soon section and instead show the names of the days after tomorrow.  This idea was stated by Trevor in bug 417222 comment 11.  Then the number of days in the options that Decathlon is implementing could represent the total number of days to show in the agenda.  So if today is Monday and I choose to show 4 days, I would see sections for Today, Tomorrow, Wednesday, and Thursday.
>The same patch reported above but with a text box element instead of a menu
>list.
>Please, review only that one you prefer.

Which patch I would give the preference is not as important as you might think because Christian has the last word about this. Therefor I will ask him for ui-review for the two patches.
Attachment #342318 - Flags: ui-review?(christian.jansen)
Attachment #342319 - Flags: ui-review?(christian.jansen)
Small ignorant question, but I can't seem to find an answer: how to I grab one of those diff files and patch my thunderbird?!

Thanks in advance!
Attached patch unbitrotted version of patch 342318 (obsolete) β€” β€” Splinter Review
I unbitrotted the patches and showed them to Christian who has been quite busy the last weeks. This is what came out of it:
- Christian (and me also) generally liked both of your patches a lot and gave the preference to your first patch with the menu-list (https://bugzilla.mozilla.org/attachment.cgi?id=342318).
- The agenda preference setting should not be made in the tab reserved for the calendar views. Instead we should add a new tab for the "Today Pane" with a first heading "Agenda" over your your labeled menulist.
- There should be a preference listener attached to the agenda-richlistitem just like the preference observer "mPrefObserver" for the calendar-views. We have not yet talked about the suggestion of Pete in comment #20 concerning the labeling of the agenda-richlistitem, but one more aspect came to my mind: When the user changes the preference setting we should give him some kind of feedback that the change is taking place immediately by adapting the label to "next x days". Besides this I find Pete's comment very reasonable. Maybe you can leave this as it is for the time being.
- the default should be set to 5 days as done already.
For now I will deny the review.
We apologize for the late review and look forward to a new patch.
Attachment #342318 - Attachment is obsolete: true
Attachment #342319 - Attachment is obsolete: true
Attachment #342318 - Flags: ui-review?(christian.jansen)
Attachment #342318 - Flags: review?(Berend.Cornelius)
Attachment #342319 - Flags: ui-review?(christian.jansen)
Attachment #342319 - Flags: review?(Berend.Cornelius)
Summary: Soon is subjective and should be configurable → Make configuration of Today Pane "Soon" duration available in preference dialog too
Decathlon, please change the status to NEW if you no longer work on this issue.
Thanks.
(In reply to comment #26)
> Decathlon, please change the status to NEW if you no longer work on this issue.
> Thanks.

Actually I'm working on this but to be honest, since the original bug is changed and it has become a bit hard for me, it's better to leave it to someone more skilled than me.

I've done some work after the last requests in comment #24 and I have a totally working patch that I attach here if someone can\want extract from it something useful, but for which _I won't ask for review_ because:
1. I'm not able to build Lightning from it (I can't understand what I'm doing wrong maybe something is missing or there is a silly thing to change);
2. it is completely working but for sure there are things that should be done in a different way and most probably I won't be able to do them;

On my system I get it working modifying every file involved in the patch (apart from jar.mn files) inside Thunderbird profile and adding the following line to chrome.manifest file inside Lightning folder (in Thunderbird profile):

overlay chrome://messenger/content/preferences/preferences.xul chrome://calendar/content/preferences/todaypane.xul
(after line 23)

The patch adds a tab for today pane options in Tools->Options->Lightning where the user can set the "soon" duration with a dropdown menu. It changes the soon header in agenda listbox from "Soon" to "Next x days/weeks" as originally planed by Christian Jansen in comment #15 and adds a preference observer to monitor "soondays" preference changes from tab panel and from config editor as well.
Attachment #341359 - Attachment is obsolete: true
Status: ASSIGNED → NEW
Target Milestone: 1.0 → ---
Decathlon, maybe you want to give this another try? I think it would be nice to have for Lightning 4.0. String freeze is in a week.
Flags: needinfo?(bv1578)
OK, I planned to finish this since I use the try-build server when I need to build some patch, but I can't promise about the deadline.
Flags: needinfo?(bv1578)
Status: NEW → ASSIGNED
If you can't get the whole patch finished that is fine, just make sure you have all the strings ready that you will use. We can then push the strings only before the deadline and the patch plus its backport afterwards.
Attached patch patch - v1 (obsolete) β€” β€” Splinter Review
I attach this one with the try builds for all the platforms:

http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/bv1578@gmail.com-c3efff1ae049/

It works fine but I don't ask for review because there are things that I would like to do differently, anyway the strings should be the right ones.

Philipp, could you take a look to the strings, maybe by installing a build, to see if we want to have a functioning like that?
If it's OK I will attach a strings only patch.
Attachment #355995 - Attachment is obsolete: true
Attachment #382485 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Comment on attachment 8566306 [details] [diff] [review]
patch - v1

Review of attachment 8566306 [details] [diff] [review]:
-----------------------------------------------------------------

I'd suggest getting a comment about the prefs organization from Richard. Maybe we can collapse this into one of the other pref panes instead? I don't see any reason not to fix this bug so I'm happy to have it added. Given the string freeze is on Monday and you already said you'd like to create a strings only patch, I think you should add LOCALIZATION_NOTEs to each string to make it easy to understand for localizers what they are translating since they can't test the feature.

::: calendar/locales/en-US/chrome/calendar/preferences/todayPane.dtd
@@ +7,5 @@
> +   - your editor isn't using UTF-8 encoding and may munge up the document!
> +  -->
> +
> +<!ENTITY pref.calendar.todaypane.agenda.caption "Agenda">
> +<!ENTITY pref.soondays.label "After tomorrow agenda shows:">

Do we use Agenda in other places? I thought we also called it today pane in user visible text?

If so, maybe it would be better to be more explicit: 
"The 'Soon' section shows:"

Or maybe rename "Soon" to "Upcoming", because "The upcoming section shows" sounds better.

I don't have too strong feelings on that, I'm just a little unsure about the "Agenda" part.

@@ +9,5 @@
> +
> +<!ENTITY pref.calendar.todaypane.agenda.caption "Agenda">
> +<!ENTITY pref.soondays.label "After tomorrow agenda shows:">
> +<!ENTITY pref.soondays.accesskey "g">
> +<!ENTITY pref.numberofdays.1 "one day">

Instead of doing this with a .dtd file, maybe it makes sense to use a properties file and then just take advantage of PluralForm.jsm? We might already have strings for "x days" and "x weeks".
Attachment #8566306 - Flags: feedback?(richard.marti)
Flags: needinfo?(philipp)
Yeah, a separate tab for only one options is a bit overkill. The best place would be the views tab. But this is already the tallest. If we can place it here, great. The second best place would be the General tab.

(In reply to Philipp Kewisch [:Fallen] from comment #32)
> 
> Or maybe rename "Soon" to "Upcoming", because "The upcoming section shows"
> sounds better.

+1

Then in the today pane it should also be renamed to something like "Upcoming Events" to have a better relation to this option. Maybe put the span which was choosen in brackets.
(In reply to Philipp Kewisch [:Fallen] from comment #32)
> 
> I'd suggest getting a comment about the prefs organization from Richard.
> Maybe we can collapse this into one of the other pref panes instead?

I initially did a patch that added a section in another preference tab (the view tab), but afterwards I added a specific tab for the today pane because has been decided in that way by Berend and Christian in comment 24, that was the conclusive specific for this issues (a long time ago actually).
I think they decided in that way also because Berend planned to add some other setting for the today pane as reported in comment 14 
>  e.g.  a setting that determines whether all events of today
> should be displayed or only  the ones from now on.
(that is bug 412740 for which I think I have a fix).

I know that SUN's guys no longer are in the Calendar team, so if we want to decide differently we can do it, but anyway it should be a good starting point.

> see any reason not to fix this bug so I'm happy to have it added. Given the
> string freeze is on Monday and you already said you'd like to create a
> strings only patch, I think you should add LOCALIZATION_NOTEs to each string
> to make it easy to understand for localizers what they are translating since
> they can't test the feature.

Definitely, for a complete patch I have to revise a few things, for example the preference observer. In this patch I added one in the agendaListbox item because it could be also useful for bug 412740, but in comment 24 had been requested an observer in the agenda-richlistitem (I had forgotten).

> 
> > +<!ENTITY pref.calendar.todaypane.agenda.caption "Agenda">
> > +<!ENTITY pref.soondays.label "After tomorrow agenda shows:">
> 
> Do we use Agenda in other places? I thought we also called it today pane in
> user visible text?
> 
> If so, maybe it would be better to be more explicit: 
> "The 'Soon' section shows:"
> 
> Or maybe rename "Soon" to "Upcoming", because "The upcoming section shows"
> sounds better.
> 
> I don't have too strong feelings on that, I'm just a little unsure about the
> "Agenda" part.

I know it's a bit hard to find the right terms, we discussed also in comments 12, 15, 20 ... and in other bugs too.
The "agenda" should be the part of the "today pane" that shows the list upcoming events, instead the "today pane" should be the overall right side panel, but I agree we never used "agenda" in the UI apart form the code, so it's better to use "today pane".

The real fix for this issue is a fix for bug 417222 as Pete Riley mentioned in comment 21. In that way all the ambiguity about the "soon" section would disappear.

> > +
> > +<!ENTITY pref.calendar.todaypane.agenda.caption "Agenda">
> > +<!ENTITY pref.soondays.label "After tomorrow agenda shows:">
> > +<!ENTITY pref.soondays.accesskey "g">
> > +<!ENTITY pref.numberofdays.1 "one day">
> 
> Instead of doing this with a .dtd file, maybe it makes sense to use a
> properties file and then just take advantage of PluralForm.jsm? We might
> already have strings for "x days" and "x weeks".

OK, it's fine for me.
Comment on attachment 8566306 [details] [diff] [review]
patch - v1

Forgot to clear the feedback.
Attachment #8566306 - Flags: feedback?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #33)
> Yeah, a separate tab for only one options is a bit overkill. The best place
> would be the views tab. But this is already the tallest. If we can place it
> here, great. The second best place would be the General tab.

I took a look, in the Views tab there is absolutely no room for another section unless we want to make higher the options dialog. 
The new options tab, enabled with the  preference mail.preferences.inContent, uses the whole window height, but it also uses gigantic elements and characters (that is another Firefox madness, but this is another matter) that fill the window with a few elements.
The unique place seems the general tab where we can add only a section with a one-line option and it will fill completely the tab.

> > 
> > Or maybe rename "Soon" to "Upcoming", because "The upcoming section shows"
> > sounds better.
> 
> +1
> 
> Then in the today pane it should also be renamed to something like "Upcoming
> Events" to have a better relation to this option. Maybe put the span which
> was choosen in brackets.

I know this element is a minefield, there have been lot of discussions about that in the past.
If I use "Upcoming" instead of "Soon" or "Next" (as suggested in the previous comments), what should be the header in the today pane with the indication of the period? 
  
   Upcoming 3 days
   Upcoming Events in 2 Weeks
   Upcoming (5 days)
   ...
Attached patch patch-v2 β€” β€” Splinter Review
With this patch I've moved the today pane section in the general tab, changed the strings as suggested in the comments above:

- caption's label:  "The upcoming section shows:";
- renamed "Soon" into "Upcoming" in the today pane;
- used the brackets for the number of days e.g. "Upcoming (2 weeks)";
- deleted the labels for the menulist (now created dynamically with the plural form);
- deleted the labels about the period of time for the "Upcoming" label in the today pane (now created dynamically with the plural form);

The patch seems working fine. I found, and corrected, a little issue about the presence of the date for the events in the "Upcoming" section when the duration is one day, so, in general, I would take a bit more time to test, but the strings should be good therefore if we want to freeze them for tomorrow we can do it with all the patch and maybe verify carefully in the next days for others things and a complete review.

Philipp, it's your decision.

Here there are builds for all the platforms:
http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/bv1578@gmail.com-29275963a098/
Attachment #8566306 - Attachment is obsolete: true
Attachment #8567624 - Flags: review?(philipp)
Comment on attachment 8567624 [details] [diff] [review]
patch-v2

Review of attachment 8567624 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to push the patch as is before the freeze, with the entity name changed. Maybe you can take care of the review comments and extra testing in the next week or two.

::: calendar/base/content/agenda-listbox.js
@@ +14,5 @@
>      agendaListboxControl: null,
>      mPendingRefreshJobs: null,
>      kDefaultTimezone: null,
> +    showsToday: false,
> +    soonDays: 5

Maybe we should change "soon" to upcoming also in our code.

::: calendar/locales/en-US/chrome/calendar/calendar.dtd
@@ +16,2 @@
>  <!ENTITY calendar.tomorrow.button.label       "Tomorrow">
> +<!ENTITY calendar.soon.button.label           "Upcoming">

We need to change the entity name.
Attachment #8567624 - Flags: review?(philipp) → review+
Pushed to comm-central changeset 9903ed277c05
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.0
>> +<!ENTITY pref.soondays.label "The upcoming section shows:">
>> +<!ENTITY pref.soondays.accesskey "g">

Please avoid using a lowercase g for access keys, as this is against the policy - see https://developer.mozilla.org/en-US/docs/XUL_Accesskey_FAQ_and_Policies. As there are lots of keys to choose from, maybe β€˜u’ would do fine.
Thanks for the note. We can change this without changing the entity since we are not changing the meaning of the string. Decathlon, can you upload a new patch that changes the access key?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(bv1578)
Attached patch fix accesskey β€” β€” Splinter Review
Changed the accesskey to "u".
Flags: needinfo?(bv1578)
Attachment #8570138 - Flags: review+
Attachment #8570138 - Flags: approval-calendar-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: