Bugzilla – Full Text Bug Listing
| Summary: | [Preferences] preferenceTransfer: Allow wildcards on keys | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Martin Aeschlimann <martinae> | ||||||||||||||
| Component: | UI | Assignee: | Oleg Besedin <ob1.eclipse> | ||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||
| Severity: | enhancement | ||||||||||||||||
| Priority: | P3 | CC: | daniel_megert, markus.kell.r, Mike_Wilson, ob1.eclipse, pwebster, richardfearn, semion, tjwatson | ||||||||||||||
| Version: | 3.3 | Keywords: | api, helpwanted | ||||||||||||||
| Target Milestone: | 3.6 M3 | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Windows XP | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 163543, 203008, 210013, 279415 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Martin Aeschlimann
Any chance that we can add that for 3.4? We would like to add a transfer for compiler options but it would fill up our plugin.xml to name all the options involved. Kim, is this something we could do for 3.5? I'll investigate in M3. I can't imagine this being a big issue. Moving target as M3 has shipped. This missed the boat for 3.5 API freeze. Is it OK to move this to 3.6? Yes, fine by me as at this point we won't have time to add the transfers for 3.5 anyway. Created attachment 146061 [details]
First version of patch
I suggest to use name of key as a regular expression - optionally, by control of some additional boolean attribute.
Patch includes some changes, proposed for equinox.preferences.
Semion, thank you for the patch! Its a good start and the shape of the change to the extension point makes a lot of sense. However, the bundle "org.eclipse.equinox.preferences" specifies that it can be run in either Java 1.4 VM or a Foundation 1.1 VM: Bundle-RequiredExecutionEnvironment: CDC-1.1/Foundation-1.1, J2SE-1.4 The java.util.regex.* classes are available in Java 1.4, but not in the Foundation 1.1 VM. (The best way to check is to compile the code agains the specific VM.) Realistically, we can't increase minimum execution environments for current bundles in the 3.x stream. Hence, we can't use regular expressions in the "org.eclipse.equinox.preferences" bundle. There seem to be two possibilities here: - just limit ourselves to processing '*'s, not using regex'es - move processing of the regex in the "org.eclipse.ui.workbench" bundle - say, using Preferences.keys() and filtering results in the ui.workbench. (I was not sure if it is OK to mention regex in the extension point definition in the "org.eclipse.ui" bundle as that bundle is Foundation 1.0. However, activity point already mentions regex so I guess it is fine.) Created attachment 146749 [details]
Second patch
Here regular expressions are used only in ui.workbench.
About Foundation 1.1: I don't have it, and only heard, that it includes Java 1.4.2 with some additions... How can I download it?
(In reply to comment #9) > About Foundation 1.1: I don't have it, and only heard, that it includes Java > 1.4.2 with some additions... How can I download it? There is some writeup on this page: http://wiki.eclipse.org/J9 (In reply to comment #9) > Created an attachment (id=146749) [details] > Second patch > Here regular expressions are used only in ui.workbench. > About Foundation 1.1: I don't have it, and only heard, that it includes Java > 1.4.2 with some additions... How can I download it? Ops... The bundle org.eclipse.ui.workbench says it supports Foundation 1.1: Bundle-RequiredExecutionEnvironment: J2SE-1.4, CDC-1.1/Foundation-1.1 (Turns out that code in the org.eclipse.ui.workbench using regular expressions was a mistake made a while back, see bug 255849 comment 3.) With this in mind, I guess, the best we can do is to follow the original request supporting <key name="org.eclipse.jdt.core.compiler." match="prefix"/> With this approach, if in the future we get a request to support "*" in the middle, we can add "match=filter" and remain backward compatible. (Or, even "match=regex" if in some distant future we'll be allowed to move up to Java 1.4 :-).) One other point: for this change it would be nice to add a JUnit test to make sure that the filtering works properly. There are several JUnits for the runtime code in the PreferencesServiceTest class in the "org.eclipse.core.tests.runtime" bundle. Created attachment 147088 [details]
Third patch
OK. This patch includes "match" attribute with "PREFIX" value.
Created attachment 147111 [details]
JUnit test
Another world... Please, check, if this suitable.
Cool, nice work!
=> I would describe the new API a bit differently. You added the new constructor
public PreferenceFilterEntry(String key, boolean isPrefix)
That would work fine for now, but what if we need to add another match type in the future("filter", "regex")? I'd rather put:
public PreferenceFilterEntry(String key, String matchType)
which would accept "null" for matchType and added an API constant for the "prefix" match type.
Also, those are API modifications, they need "@since" and the version number of the org.eclipse.equinox.preferences needs to be incremented to 3.3.
=> PreferenceFilterEntry#addMatch() / getMatches()
I am not sure if it is a good idea to use this class to store match results.
From what I see matches are added in PreferencesService#matches() and used in #applyPreferences() / #exportPreferences().
Logically, match results are are specific to a particular prefrences branch, and to me it is not clear why (or how) the filter it tied to a specific branch.
Or, in different terms, it assumes that PreferencesService#matches() always used before PreferencesService#exportPreferences(). It is true for the current implementation of the export wizard, but, I think, only because wizard calls the #matches() to filter out empty exports.
For instance, the JUnit will fail if:
verifier = new ExportVerifier(..., new IPreferenceFilter[] {transfer})
is used instead of
matching = service.matches(..., new IPreferenceFilter[] {transfer})
verifier = new ExportVerifier(..., matching)
=> The PreferenceService implementation has references to regex ("internalMatchesWithRegexp") which no longer match functionality.
=> The new attribute in the schema ("match"): in the last couple versions of Eclipse it is possible to specify list of expected values for string attributes (in this case just "prefix"). To do this, in the schema editor, see "Restrictions" -> Add. Then a plugin who wants to use this attribute can select "prefix" from a drop-down menu.
=> The [arguably] slightly more readable documentation text for the "match" attribute could be (thanks to Eric!):
"Setting match to "prefix" treats the name as if it were a regular expression with an asterisk at the end; otherwise name must be an exact match. Added in 3.6."
=> Copyright headers (both in JUnits and the other patch): you have them updated in some case, but not for all files.
Created attachment 147765 [details]
One more match, including changes for JUnit test
Created attachment 147826 [details]
Slightly modified Semion's patch
This looks very good. I'll modify bundle versions and Javadoc a bit in this patch:
=> Versions: this is actually a perfect example of how bundle version changes propagate. The preferences bundle has its bundle version properly incremented to 3.3. That bundle is re-exported by the "org.eclipse.core.runtime", so:
- minimum version for preferences has to be upgraded to 3.3 in the list of dependencies of the "org.eclipse.core.runtime"
- the version of the "org.eclipse.core.runtime" itself has to be upgraded to 3.6.
- the dependency on preferences in the "org.eclipse.ui.workbench" has to be updated to indicate that it needs minimum ver. 3.3 of preferences
(Also, the "@since" version on new APIs have to correspond to the new bundle version, 3.3 in this case, not 3.6.)
=> I changed language on Javadocs for PreferenceFilterEntry a bit. The schema for the extension point had a sentence that keys can contain comma-separated values. That does not work and from what I can see did not at least as far as CVS history goes. Hence, I agree that it is better to remove that one sentence from the schema description.
By the way, I just downloaded Eclipse 3.1 (the first release were the preferenceTransfer extension point appears) and verified that it does not work with a comma-separated list of names. After a bit of searching in Bugzilla I came across bug 91298 comment 8. Aparently the support for comma-separated keys was removed during 3.1 development cycle but the schema was not updated. The patch looks good. I'll release both portions (Equinox and UI) next Monday to minimize disruption caused by the version changes. (In reply to comment #18) > The patch looks good. I'll release both portions (Equinox and UI) next Monday > to minimize disruption caused by the version changes. I have not looked at the patch, but I recommend releasing to CVS in the next day or so to get N-Builds. We don't want the first build with the changes to be the I-Build. Good point. I applied changes to both Equinox and Platform/UI components. I'll send a message to ui-dev list warning that developers might need to check out preferences and runtime bundles from the CVS Head. Semion, thank you for the patches! The documentation in the preferenceTransfer extension point is wrong: 'match - Setting match to "prefix" treats the name as if it were a regular expression with an asterisk at the end.' => The implementation does not treat the name as a regular expression (e.g. I can't add other regex instructions in my prefix name). And even if it was treated like a regex, the appendix would have had to be '.*', not '*'. (In reply to comment #21) > The documentation in the preferenceTransfer extension point is wrong ... That text is a result of trying to explain its functionality in a natural terms (and a product of a mini-meeting on that subject :-)). How about this: == If match is set to "prefix", all keys on the preference node that start with the specified name will be included in the filter results. If match type is not specified, only keys equal to the name will be included. Added in 3.6. == or any other proposals? > If match is set to "prefix", all keys on the preference node that start with
> the specified name will be included in the filter results. If match type is not
> specified, only keys equal to the name will be included. Added in 3.6.
Sounds good. Nitpicking as I am, I would say 'transferred' instead of 'included in the filter results' and 'If the match type is not specified' (add 'the').
That sounds good, thanks Markus! The updated text is now in CVS Head. Verified in I20090929-0800. Works as expected! We might need the option to exclude some keys out of the whole matching set but I'll file a feature request if I stumble of a real case. |