Closed Bug 321134 Opened 19 years ago Closed 18 years ago

Next and Previous menu items do what?

Categories

(Calendar :: Sunbird Only, defect)

x86
Linux
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: elreydetodo, Assigned: mschroeder)

References

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051220 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051220 Firefox/1.5

The Next and Previous menu items on the Go menu go to the next and previous month but this is not apparent from the title of the menu item.  I had to try them out to figure out what they meant - it could have been next/previous day.  I suggest the names "Next Month" and "Previous Month" or something similar.

Reproducible: Always

Steps to Reproduce:
This seems pretty reasonable, so I'm confirming.  I put something like this into the Lightning context menus, and that could easily be ported to the Sunbird menubar.  See the patch on Bug 298504 for more info about that.
Assignee: mostafah → nobody
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug]
The caption of these buttons for en-US is set at calendar/resources/locale/en-US/menuOverlay.dtd lines 186 and 189

Changing these values will affect the en-US locale, but all the others will need to be updated as well, right?  Accesskey specifications are an the line immediately after each of the two lines previously listed.
(In reply to comment #2)
> Changing these values will affect the en-US locale, but all the others will
> need to be updated as well, right?

Patches are normally landed with only the en-US changes made.  It is up to the localization team to change the appropriate files in other translations.  Most often this is done only prior to a release, to avoid excess work (since the strings can change several times during development).

Keep in mind that Next and Previous menu items depend on the current view.

If the day view is selected they mean next/previous day.
If the week/multiweek view is selected they mean next/previous week.
If the month view is selected they mean next/previous month.

So changing the dtd only is not sufficient.
Is there a string stored anywhere that can indicates the current view unit?  If so would be use that value to append to the menu item label?  If so we could so something like goPreviousCmd.label = "Previous " + view_unit which would produce something like "Previous month" or "Previous day".  This would have to be implemented in javascript, probably in the change view code I would think.

If the base viewing unit is not available now, it seems like this could be useful enoough to save somewhere or make available somehow.
(In reply to comment #5)
> Is there a string stored anywhere that can indicates the current view unit?  If
> so would be use that value to append to the menu item label?  If so we could so
> something like goPreviousCmd.label = "Previous " + view_unit which would
> produce something like "Previous month" or "Previous day".  This would have to
> be implemented in javascript, probably in the change view code I would think.

This is exactly the type of trick I used in Bug 298504, so yes, all the strings are already there (in calendar.dtd).

That is a *really* neat trick!  I think the same thing could work for the menu items here but I think that the label-type will have to refer to a localized string that we would have to create.  If we weren't concerned about l10n I'd suggest we just chuck "+ type" on the end of the base label :)

On a side note, would anyone be willing to point me in the right direction on how to create patches? I think I could make the adjustments to the code if someone could tell me how exactly to make a patch to post in the bug.  My email address is good, I should be able to get anything that's sent there.
(In reply to comment #7)
> On a side note, would anyone be willing to point me in the right direction on
> how to create patches?

I just started writing http://wiki.mozilla.org/Calendar:Hacking to try and answer this type of question.  It's still a work in progress, but hopefully you'll find it helpful.  Feedback from you on which parts are unclear to a first-time hacker would of course be very beneficial.  (Please post feedback on the discussion page of that wikipage, not in this bug.)

This patch does the following:

* in menuOverlay.dtd, replace the goNextCmd.label with goNextCmd.label.day/week/month and same for goPrevCmd
* in menuOverlay.xul, replac the label attribute with label-day/month/year
* in {day/week/month}View.js in the switchTo() function, add code to set the menu command label appropriately

One issue this may have is setting the value at initial startup.  Is the switchTo() method called for the initial view? If not then we'll end up with no label on those menu items :-/

I hope I made this patch right, I couldn't get it to add on the function names or have the leading mozilla/ on the path name.
(In reply to comment #9)
> Created an attachment (id=206637) [edit]
> patch to make next and previous labels depend on view mode

This looks like a reasonable start to this!  Nice work.  Now, for the bad news:  There's no real way you could have known this, but bug 297934 is about to land (maybe today, hopefully tomorrow at the latest, then again there could always be unforseen delays :-( ).  As you can see from the patch size there, that bug is going to change a LOT of code.  For your purposes, you need to know that that patch is going to completely remove dayView.js, weekView.js multiweekView.js and monthView.js (and their corresponding xul files) from Sunbird/calendar.

In other words, once that lands, these fixes won't work any more. :-(

Therefore, I'm going to ask that you wait until that bug lands before submitting another patch, since bug 297934 has priority over this one.

When you do submit another patch, I'll recommend that you try to avoid unneccessary code duplication.  (Having the same 6 lines in 3 different places isn't good, because it means that to fix a problem in one, you have to remember to fix it in all the rest.)  Take a look at calendarWindow.js (now and after the  new-view land).  That file dispatches all of the switchTo commands, and so it would probably be a good, central location to place this functionality (and only write it once).  Then you can probably use the 'type' trick again, so that you don't have to change any strings.

And most importantly, ask for a review from someone! :-)  Good patches fall through the cracks when they don't have a review request on them.

(You should also be able to check the 'Take Bug' box when submitting a patch, to assign it to yourself.)(In reply to comment #9)
> One issue this may have is setting the value at initial startup.  Is the
> switchTo() method called for the initial view? If not then we'll end up with no
> label on those menu items :-/
The answer is 'yes'.  gCalendarWindow should trigger the switchTo call even at startup.

> 
> I hope I made this patch right, I couldn't get it to add on the function names
> or have the leading mozilla/ on the path name.
It looks like you may have made the patch from the jar file, rather than from CVS source?  That might explain it, but for now it's not a big deal.  This is acceptable.  The leading mozilla/ isn't required, as long as there's enough of the path that it's obvious which file you're referring to.

Depends on: 297934
This version implements the label changing code in the switchToView function in calendarWindow.js.  This seems to be what each of the switchToX methods call, so the code isn't repeated.  I renamed the extra label attributes in menuOverlay.xul to match the newView values that are passed to switchToView, but I'm concerned that those values could change and thus invaldate this patch.  Are these values pretty stable or is it possible they could change later?

Also, I fixed the label changing code so it's not completely broken like it was in the last patch.  And I removed a tab that accidentally broke another line of code in there.

re: jar v cvs
I actually am using cvs but I have to be inside the mozilla directory to use any cvs commands, otherwise I kept getting errors on, well, it doesn't matter really.  I'm probably just stupid.  Oh, and I don't see a check here to assign the bug to me but I'll do it momentarily.
Attachment #206637 - Attachment is obsolete: true
Attachment #206778 - Flags: first-review?(jminta)
(In reply to comment #11)
> Created an attachment (id=206778) [edit]
> revised version, code now in switchToView

-<!ENTITY goPreviousCmd.label                    "Previous">
-<!ENTITY goNextCmd.label                        "Next">

Before you remove items you should ensure that these items are not used in other places. The Mozilla Cross Reference at http://lxr.mozilla.org/ is a good place for doing this. If you look (for example) at 
http://lxr.mozilla.org/mozilla/search?string=goNextCmd.label you will notice that this entity is e.g. still used in file calendar-menubar.inc.

Also I think that this needs some kind of fallback strategy or error handling. If I understand the new views concept correct they allow (or will allow) custom views, maybe provided by an extension. What will happen if someone provides e.g. a year view?
Comment on attachment 206778 [details] [diff] [review]
revised version, code now in switchToView

Looking better :-)

I can see 2 main issues that need to be addressed right now.  Stefan alluded to the first.

1.) There is currently a fork in Sunbird/Calendar code.  menuOverlay.xul corresponds to the menu for the calendar extension, whereas builds of Sunbird actually use the code in http://lxr.mozilla.org/mozilla/source/calendar/sunbird/base/content/calendar-menubar.inc  for their menus.  You'll need to make changes to that code as well.  (This code split is one of the reasons why it's harder than most people think to release a new version of the calendar extension.)

2.) What about the multiweek view?  That should probably use the same code as the week-view, since next/previous moves +/- 1 week.

Regarding Stefan's comment on adding new views (like a year view), the code isn't currently designed to handle that, so I don't think you need to worry about that yet.  I'm planning to eventually remove gCalendarWindow entirely, and at that point, we'll make adding additional views easier (and introduce error checking).  For now, I don't think that case should concern us.

About the element ID's stability:  Element IDs are fairly stable.  If someone wants to change them, it's their responsibility to fix all of the code that depends on them, including code you introduce here, if it's already checked in.
Attachment #206778 - Flags: first-review?(jminta) → first-review-
Assignee: nobody → elreydetodo
Attached patch fixed and extended patch (obsolete) — — Splinter Review
Because there was no work done the last three month I took over, extended the patch to cover also multiweek and fixed some mistakes in the js-part of the patch. (Only tested in Sunbird!)
Attachment #216901 - Flags: first-review?(jminta)
Comment on attachment 216901 [details] [diff] [review]
fixed and extended patch

-                   label="&goPreviousCmd.label;"
+                   label=""
+                   label-day-view="&goPreviousCmd.day.label;"
LXR says that goPreviousCmd.label isn't used anywhere but here.  So, if we're removing it from use, we should also remove it from the dtd, so that localizers don't do extra work.  Also, can you include a comment here pointing to where/how this item's label gets set, so that it's easier for future hackers to find/follow?

Everything else looks solid.  (Note that this won't make 0.3a2)
Attachment #216901 - Flags: first-review?(jminta) → first-review-
Attached patch fixed and extended patch v2 — — Splinter Review
Added comment & removed unused labels.
Attachment #216901 - Attachment is obsolete: true
Attachment #217276 - Flags: first-review?(jminta)
Comment on attachment 217276 [details] [diff] [review]
fixed and extended patch v2

As you may have seen in the status meeting today, we're going to be moving towards merging the calendar.xul files, and the menu files you touched here will probably come along for the ride.  Until such time though, it'd be nice to keep them in sync.  This is all just a long way of saying "You forgot to add the comment to menuOverlay.xul as well." r=jminta with that.
Attachment #217276 - Flags: first-review?(jminta) → first-review+
Assignee: elreydetodo → mschroeder
Whiteboard: [good first bug] → [needs landing]
Checked in on trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
(In reply to comment #18)
> Checked in on trunk and MOZILLA_1_8_BRANCH

According to lxr and bonsai this patch was _not_ checked in on MOZILLA_1_8_BRANCH.

Matthew, please check this and commit to branch again.
My apologies. REALLY committed to MOZILLA_1_8_BRANCH this time.

Checking in calendar/sunbird/base/content/calendar-menubar.inc;
/cvsroot/mozilla/calendar/sunbird/base/content/calendar-menubar.inc,v  <--  calendar-menubar.inc
new revision: 1.13.2.7; previous revision: 1.13.2.6
done
Checking in calendar/resources/content/menuOverlay.xul;
/cvsroot/mozilla/calendar/resources/content/menuOverlay.xul,v  <--  menuOverlay.xul
new revision: 1.70.2.3; previous revision: 1.70.2.2
done
Checking in calendar/resources/locale/en-US/menuOverlay.dtd;
/cvsroot/mozilla/calendar/resources/locale/en-US/menuOverlay.dtd,v  <--  menuOverlay.dtd
new revision: 1.33.2.5; previous revision: 1.33.2.4
done
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: