Clarify API of RecordableEventDescriptor about returning translation keys or translated string

Hi everyone,

I found an inconsistency in the way we declare our RecordableEventDescriptor for the Notification system in XWiki standard.
The documentation for creating a new RecordableEventDescriptor can be seen there: Notifications API (XWiki.org)

As you can see the piece of code seems to suggest to use translation keys for #getApplicationName and getEventDescription. And that’s how we defined some of our descriptors, for example MentionEventDescriptor or LikeEventDescriptor. And that’s also how we defined some of the descriptors defined in contrib extensions, see: Sign in to GitHub · GitHub

We also display in the notification settings the list of notifications applications by using the localization tool, which emphasize the fact that this API should return a translation key, see: xwiki-platform/NotificationsPreferencesMacros.xml at master · xwiki/xwiki-platform · GitHub

But that’s not true everywhere: we actually also have in XWiki platform an AbstractRecordableEventDescriptor which on its side returns directly translated String for the same APIs, see: xwiki-platform/AbstractRecordableEventDescriptor.java at master · xwiki/xwiki-platform · GitHub
This AbstractRecordableEventDescriptor is currently extended by the standard notification events that occurs on pages.

And in some templates, such as the default email notification templates, we do not use the localization tool to display this application name, see: xwiki-platform/macros.vm at master · xwiki/xwiki-platform · GitHub

The fact that we call the localization tool on a string already localized is not that much a problem (it could be a problem in the future if the translation is a key itself), but it’s really a problem if we don’t call the localization tool for what we expect to be a translation key.

So this proposal aims at clarifying the situation: I propose that the RecordableEventDescriptor only returns translation keys, and not translated strings in the future.
IMO it’s the better solution since as I mentioned it won’t break anything if we call the localization tool and a string already localized. On the other hand, if we decide that the RecordableEventDescriptor should always return a translated string, then we need to change the implementation for all existing extensions.

WDYT?

I agree it’s the safest in terms of retro compatibility.

Honestly, I’m never a fan of low level APIs like this one returning translated result, but it’s a bit late now to remove getApplicationName and getEventDescription and rely on generated keys based on the application ID.

FTR the clarification and fixes have been done with Loading...