Closed Bug 357579 Opened 13 years ago Closed 13 years ago

Housecleaning of calendar.dtd file

Categories

(Calendar :: General, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Sunbird 0.5

People

(Reporter: cedric.corazza, Assigned: cedric.corazza)

Details

Attachments

(3 files, 2 obsolete files)

I found more than a hundred of unused entities in calendar.dtd.
For some of them, I'm not totally sure they are unused.
Some other entities would need to be moved in another file.
I attached a txt file listing all of them with some explanations.

This cleaning would be helpful for the new l10n teams that would want to localize Sb/Ln as it represents 25% less strings in this file.
I will provide a patch after having other opinions about that file.
Attached file Entities list
Attached file file to launch grep
For an easier check of the first 117 entities, here's the parameter file containing the entities to launch the following command:
~/CVS/mozus/mozilla/calendar> grep -R --colour -f calgrep.txt *
Severity: enhancement → major
Version: unspecified → Trunk
Attached patch Patch removing unused entities (obsolete) — Splinter Review
Attachment #243893 - Flags: first-review?(lilmatt)
Comment on attachment 243893 [details] [diff] [review]
Patch removing unused entities

>Index: locales/en-US/chrome/calendar/calendar.dtd
>+
Let's not add blank lines to this file. We're trying to _remove_ lines from it :)

>Index: locales/en-US/chrome/prototypes/sun-calendar-event-dialog.dtd
> <!ENTITY event.freebusy.plus             "Next hour" >
> <!ENTITY event.freebusy.minus            "Previous hour" >
> 
>+<!ENTITY repeat.units.weeks.both        "Week(s)" >
>+<!ENTITY repeat.units.months.both       "Month(s)" >
>+<!ENTITY repeat.units.years.both        "Year(s)" >
>+
Either align these as found elsewhere in that file, or don't align at all and use one space between the entity name and the double-quote. If you do the latter, remove the space between the double-quote and the >


r+lilmatt with those fixed.
Attachment #243893 - Flags: first-review?(lilmatt) → first-review+
Attachment #243893 - Attachment is obsolete: true
Attachment #245126 - Flags: second-review?
Attachment #245126 - Flags: second-review? → second-review?(cmtalbert)
Noone commented about those lines from attachment #243091 [details] :
For the following entities, I'm not sure :

newevent.recurrence.title --> this seems to be unused, I couldn't find it through the UI. Still in base/content/calendar-recurrence-dialog.xul

newevent.itemType.event.label --> this seems to be unused, I couldn't find it through the UI. Still in base/content/calendar-event-dialog.xul
newevent.itemType.todo.label --> this seems to be unused, I couldn't find it through the UI. Still in base/content/calendar-event-dialog.xul

calendar.unifinder.tree.enddate.label --> this one is not in the filter list button for tasks and is also in sunbird/base/content/calendar.xul

calendar.nextday.button.tooltip --> I couldn't find this button in the UI. I guess this was the tooltip for the arrow button, but no tooltip is displayed when mousing over. Still in lightning/content/messenger-overlay-sidebar.xul
calendar.prevday.button.tooltip --> same as previous
calendar.nextweek.button.tooltip --> same as previous
calendar.prevweek.button.tooltip --> same as previous
calendar.nextmonth.button.tooltip --> same as previous
calendar.prevmonth.button.tooltip --> same as previous

If this patch is applied and doesn't break anything, we should also clean :
base/content/calendar-recurrence-dialog.xul
base/content/calendar-event-dialog.xul
sunbird/base/content/calendar.xul
lightning/content/messenger-overlay-sidebar.xul

right ? In a different bug maybe.
(In reply to comment #6)
I agree that we need to clean up the XUL as well in order to remove these unused fields. If you can't get to them via the UI, then they should probably go. However, it would be problematic to check in this change, break both sunbird and lightning and then have to fix a different bug to correct the XUL, so I would like to see another patch with the corrected XUL on this bug so that they can be checked in at the same time.

I'll withhold my review until the XUL cleanup is here, and I'll review them together.
Clint,
I thouroughly checked again the entities involving xul files cleaning and actually, this won't be necessary:
- newevent.recurrence.title is actually a sub-dialog of the "New Event/Task > Set Pattern... " dialog (How could I miss it?)
- newevent.itemType.event.label and newevent.itemType.todo.label are in fact the greyed dropdown list, next to the "Title" field in the New Event/Task dialog
- calendar.unifinder.tree.enddate.label : though this doesn't appear anywhere in the UI, I found a work in progress in the unifinder.js file for the tree col id unifinder-search-results-tree-col-enddate , so I guess we should let this entity
- and last for calendar.*.button.tooltip entities, they are actually contextual menu in Lightning (though this menus are greyed at the moment)

So, I put back all these entities in calendar.dtd and no xul files are involved.
Attachment #245126 - Attachment is obsolete: true
Attachment #245126 - Flags: second-review?(cmtalbert)
(In reply to comment #8)
> Created an attachment (id=245643) [edit]
> Patch adressing Clint's comments
> 
Looks great to me. r+ if you wanted it... 

Comment on attachment 245643 [details] [diff] [review]
Patch adressing Clint's comments

Forwarding r+
Attachment #245643 - Flags: first-review+
Comment on attachment 245643 [details] [diff] [review]
Patch adressing Clint's comments

Fixing the reviewer.
Comment on attachment 245643 [details] [diff] [review]
Patch adressing Clint's comments

Editing to get the reviewer correct (apologize for the spam)
Attachment #245643 - Flags: first-review+
Attachment #245643 - Flags: first-review+
Attachment #245643 - Flags: second-review?(lilmatt)
Comment on attachment 245643 [details] [diff] [review]
Patch adressing Clint's comments

r2=lilmatt
okay to r2 from mvl via IRC
Attachment #245643 - Flags: second-review?(lilmatt) → second-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.