Basic mechanism for displaying text links for licenses.

[demiankatz]

This is intended as a proof of concept -- I don't think it's useful to merge as-is.

The idea is that we want to be able to display descriptions associated with the license URLs. I've just populated this with a single URL to allow the "Bride of the Tomb" example to work correctly. Ideally I think we would want the labels to account for i18n and be able to easily pull from local configuration... but I wasn't sure how to address that all the way down at the BaseProvider level where we need it.

Thoughts?

[edsilv]

Looks good. We could add these to the l10n files in the root content (where generic stuff like "canvasIndexOutOfRange" is). I guess they'd be keyed by their url:

"content": {
    "canvasIndexOutOfRange": "Canvas index out of range.",
    "degradedResourceMessage": "Please log in to view at full quality.",
    "degradedResourceLogin": "log in",
    "http://creativecommons.org/licenses/by-sa/3.0/": "Creative Commons Attribution-ShareAlike 3.0 Unported (CC BY-SA 3.0)"
}

Then we wouldn't need this.map. We could just say:

var text = this.config.content[url] ? this.config.content[url] : url;

[demiankatz]

That makes sense to me, but two questions:

1.) How do we get config injected into the LicenseFormatter class? Since it's not tied to a DOM element like all of the classes with setConfig methods, I wasn't sure how to approach the configuration loading.

2.) Would it make sense to add a "license" section parallel with "content" to keep things orderly in the configuration? If it has to go in content, I guess that would be okay... but it seems like it could get pretty cluttered, and keeping it separate would probably be preferable!

[edsilv]

1.) I'd pass the provider to the LicenseFormatter. You can then use provider.config.content. This is a pattern I've used elsewhere, such as ExternalResource.ts

https://github.com/UniversalViewer/universalviewer/blob/master/src/modules/uv-shared-module/ExternalResource.ts#L19

I think we need to investigate proper dependency injection strategies though at some point.

2.) Yep, better to add a license section.

[demiankatz]

Makes sense. I'll work on this as soon as time permits.

[edsilv]

Actually, looks like provider isn't being used in ExternalResource..! Must be a vestige of some early naive approach. Will remove.

[demiankatz]

Okay, I have made some revisions:

• There is now a generic UriLabeller class which can be injected with any set of labels. We configure this to use the license section of the configuration in the BaseProvider. Thus, we can reuse this class to label other types of URIs if similar use cases appear elsewhere. (I opted to pass just the configuration, rather than the whole provider, in order to facilitate reuse, and on the principle that we shouldn't inject more complex dependencies than we actually need).
• I've put strings for all of the CreativeCommons licenses into the en-GB file. This should be a good start on the list, though I'm sure there are some other common licenses we'll want to add.

Questions:

• For now, I've only added the licenses to the uv-seadragon-extension. Thus, the license mapping doesn't work correctly for other extensions. Is there a more universal place where we can define these? If so, I couldn't find it... but it seems undesirable to have to copy and paste (and then subsequently maintain) an unwieldy block of license data into every extension.
• I haven't tested adding/overriding licenses in my local configuration (loaded via data-config). Do you expect that will work? How does that interact with i18n when multiple languages are involved?

I don't think we should merge this until we address the first question above, but I think we're getting closer.

[edsilv]

There isn't currently a way to share l10n between extensions. Config can be extended within an extension like this:

https://github.com/UniversalViewer/universalviewer/blob/master/src/extensions/uv-seadragon-extension/config/cy-GB.json#L2

If there's no locale-specific config, it defaults to en-GB. But sometimes you want to have the same config as default except for a few settings - hence "extends".

We definitely don't want to duplicate these licences across extensions, and I think they definitely belong in l10n as you'd want them to be translatable.

The answer must be to create a shared l10n file for all extensions that is extended by default as part of the build process. Duplicate entries in an extension's individual l10n file would override.

Maybe it would be named src/l10n/default.json?

We could potentially adopt the same pattern for config too, but keeping "extends" so we can inherit from other locales when convenient.

data-config (I think...!) should override any l10n or config regardless of the current selected locale. It's kind of only meant for simple overrides, like disabling panels etc.

Not sure if data-config should be altered to support locale-specific overrides...?

[demiankatz]

I think the idea of a shared i10n file is a good one. The name would have to incorporate the locale somehow, however -- maybe src/l10n/default/en-GB.json or src/l10n/en-GB/default.json.

Would you like me to investigate implementing this as a separate pull request as a pre-requisite to completing this one?

I think it's likely that we'll need locale-specific overrides sooner or later. It seems like best practice to allow language strings to be overridden without modifying core code, and it's only a matter of time before an institution with multiple supported languages decides to take advantage of such a feature. Certainly, with this license feature, it might be necessary to define descriptions for locally-defined licenses in multiple languages. Since Villanova only uses one locale, this isn't a use case for me, so perhaps we can worry about it later when a real need arises -- but I would expect somebody to need it sooner or later.

[edsilv]

src/l10n/en-GB.json could suffice?

I'm currently in the process of improving the themes. Everything is being moved to src/modules/uv-shared-module/css. This constitutes the defaults, then the themes only contain the overrides and nothing else.

I think the l10n system could work in a similar manner. src/extensions/[extension]/l10n would not be all the strings, only those that override/extend the defaults. The default l10n files would contain strings that are common to all extensions.

In order to avoid duplicating the licences, we could employ the "extends" keyword in the default l10n files to inherit from en-GB.

Not sure how locale-specific data-config overrides would work. I suppose either by having data-config-en-GB, data-config-cy-GB attributes, or by keeping it just as data-config and partitioning it into locales within the json. The latter approach would result in an unnecessarily large download though if you only ever used the en-GB locale.

A data- attribute per locale would be unwieldy, and a pain to maintain. Perhaps based on the default data-config file, when switching locales the UV attempts to load another from the same location using a file name convention such as myconfig.cy-GB.json?

[demiankatz]

Yes, I'd be fine with src/i10n/en-GB.json.

I definitely like the idea of making the themes more light-weight, and extending that approach to l10n makes a lot of sense. As I said, let me know if I can be of any assistance with this. If this is something you're planning for the near future and I can help do some of the work, I'm happy to take direction and do some refactoring. If you think it's a bit further out but would like to get this PR resolved, let me know if you'd like me to just copy and paste the licenses across extensions as a temporary measure.

Regarding data-config, etc., I agree that it would be unreasonably cumbersome to specify a data attribute for every locale in cases where more than a small number are being maintained. Without being familiar with the configuration loading code, I'm not sure what the best strategy might be. Using the filename convention sounds like it could work, though I imagine it would trigger a lot of unnecessary HTTP traffic and 404 errors since the majority would not use it. That might cause some confusion that would better be avoided. (Though I suppose you could actually make this mechanism configurable, which might offer the best of both worlds -- i.e. data-use-locale-in-config-filename=true). The other alternative would be to allow the main config file to have sections, like:

{
  "global": { ... },
  "en-GB": { ... },
  "cy-GB": { ...}
}

That might work fairly well -- the parsing code would just have to check for a "global" key (or some other distinctive flag) to determine whether to treat the entire config as global or to extract multiple relevant sections.

[edsilv]

Yep, more than happy for you to take this on :-)

Go ahead and copy the licences into each l10n file for now and we can resolve this PR. Then, maybe create a new issue outlining what we've agreed around default l10n files.

I like the idea of including the paths to other data-config locales in the default config file. Maybe create an issue for that too and we can continue the conversation there.

[demiankatz]

Okay, license data is cloned for now -- go ahead and merge if you're satisfied. I'll open those other two issues momentarily.

[demiankatz]

Just opened #170 and #171. I've posted a comment for discussion on #171, and I may follow up with more thoughts once I've reviewed the existing config inheritance functionality.