357579 - Housecleaning of calendar.dtd file

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Cédric Corazza]

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.

Comment 1

[Cédric Corazza]

Attachment: Entities list

Comment 2

[Cédric Corazza]

Attachment: 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 *

Comment 3

[Cédric Corazza]

Attachment: Patch removing unused entities

Comment 4

[Matthew (lilmatt) Willis]

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.

Comment 5

[Cédric Corazza]

Attachment: Patch fixing the nits asked by lilmatt

Comment 6

[Cédric Corazza]

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.

Comment 7

[cmtalbert]

(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.

Comment 8

[Cédric Corazza]

Attachment: Patch adressing Clint's comments

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.

Comment 9

[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 10

[Cédric Corazza]

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

Forwarding r+

Comment 11

[cmtalbert]

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

Fixing the reviewer.

Comment 12

[cmtalbert]

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

Editing to get the reviewer correct (apologize for the spam)

Comment 13

[Matthew (lilmatt) Willis]

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

r2=lilmatt
okay to r2 from mvl via IRC

Comment 14

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED