Deprecate the theme map option

[ahocevar]

Since #884 we are encouraged to include a theme css resource in our OpenLayers pages. But there is still the theme map option which has a default that does not match this practice.

We should deprecate the theme option, remove the theme check from the map constructor, and require users to include the theme css resource in the html. Another good reason for this is that passing non-UTF8 encoded arguments to the page URL will cause the page to not load because the check for the theme css in the Map constructor uses Util.isEquivalentURL (see #2617).

[ahocevar]

The above patch removes theme related code and tests. Tests pass in Safari 4. Thanks for any review. Feel free to set the ticket state back to "Needs Discussion" if there are concerns.

[bartvde]

Running into this same issue now, so I'll start reviewing your patch ahocevar.

[bartvde]

This all makes perfect sense, please commit.

[bartvde]

Tests pass in FF3.6 as well btw.

[ahocevar]

Bart, please commit on my behalf - I won't have an svn environment available before Jun 21.

[bartvde]

(In [10372]) Deprecate the theme map option, p=ahocevar, r=me (closes #2619)

[elemoine]

sorry for coming late, but aren't we breaking backward compatibility here?

[ahocevar]

Eric: I don't think so. Using the theme option is known to not work reliably since #884, and all our examples and docs show that the css has to be included since.

[elemoine]

Thanks for the explanation Andreas.

[crschmidt]

Two users have reported now that this change has caused their applications to break. If we are really interested in breaking applications like this -- specifically, people who reference just a single library and had it work before, which was the majority -- then we should at least document this change clearly in a release note. (Preferably, something available now, so I can point people to it going forward.)

The cases where the ordering of the loading of the style sheet causes the code to break are very few and far between: as far as I know, only the scalebar and overviewmap controls would be affected, because those are the only two that try to read CSS code before the page is loaded.

This change creates a backwards incompatibility without documentation at the moment. If it's important to break this backwards compatibility (I don't think it is, but I'm willing to let the majority rule), we at least need to document it clearly.

We have never explicitly stated in release notes that we expect users to include CSS files in their pages, even in the fix for #884, so far as I can tell. The ticket basically just (silently) changed the examples.

Users still use the single tag method of using OpenLayers. I can't evaluate how common it is without more investigation, but it's not uncommon. If we really want to change this, we need to at least document it better, and honestly, unless this is actively *breaking* something (you can always set theme: null in your map, so I don't know what it would be), I'd rather announce this and wait for another release to change it.

The following users have complained of this problem on #openlayers:

• strk
• zzolo
• tom_o_t

Others will be bit by it, including any application I've written personally, most likely. I don't really like the idea of going back and changing all my applications to improve this behavior, but if I've got to, let's at least document it.

[bartvde]

Chris, personally I am still in favour of this change. I was triggered by an excellent web designer on a project who really did not like applications adding in stylesheets dynamically, he found it "not done". Also as Andreas has pointed out this can cause problems with non UTF-8 characters in urls.

However, since Eric had the same worries as you, it seems this is at a draw (2-2). Maybe Tim can shed his light?

I am happy to document this change somewhere on the Wiki if we agree this is the way forward.

[bartvde]

Chris, I've added a compatibility note here:

 http://trac.openlayers.org/wiki/Release/2.10/Notes

[crschmidt]

This change breaks applications that worked previously (in many cases, auto-detection worked) in favor of making things that didn't work previously work (URLs with non-UTF-8 characters in them), which has a workaround (specify theme: null, so that we don't try to add the stylesheet).

It also prevents users from having to configure their map to have it not request stylesheets on the fly (which can be configured by doing theme: null in map options).

I would say that kind of change that causes existing applications to behave differently is not appropriate for a minor release, especially when we can offer a workaround to give all the same behavior. (If we *can't* do that, then we should fix that.) Having controls not be usable (because they all pile up in the upper left corner) for me, fits in the definition of 'not usable', and I would consider this behavior a bug.

We currently have no documentation anywhere that tells users they must include two tags in their HTML pages, so far as I've seen. Our documented introductory example:

 http://docs.openlayers.org/library/introduction.html

doesn't include the <style> tags.

I'm fully willing to admit that this is a bug, that we should fix, but I don't think that it's fair to break existing applications because we have changed our mind about how things should be done.

(On the other hand, if people are wanting to call the next version 3.0, then this complaint goes out the window.)

[bartvde]

If we agree on doing a 2.10 release, we will have to revert r10372 and apply it in the 3.0 branch instead.

[tschaub]

I think I agree with Chris that this is a pretty big compatibility breaking change - not worth making before 3.0. I've seen plenty of places where the dynamic stylesheet loading and url checking have had unintended consequences. My approach has been to always set theme: null. As far as I can tell, this should still be a good way to get by until 3.0.

[bartvde]

Agreed. Reverted in r10402 Leave this one for 3.0.