Community
Participate
Working Groups
Build ID: I20071101-2000 Steps To Reproduce: It is expected that you know how to use Activities. 1. Open the manifest editor, go to the Extension page. Create an entry at the extension point: "org.eclipse.ui.activities". 2. Create an Activity. 3. Create an ActivityPatternBinding. Use a binding which contains the full non-modified id of the object, e.g. "a.b.c.d.e.f.g" - without any backslashes. 4. Use your activity with the given pattern. It won't bind! More information: If you have a deep nested id (like org.company.for.anotherCompany.branch.fork.native.win.win32.menu.menuX.menuPoint), it can become _very_ annoying to write such strings over and over as a regular expression with a dozen backslashes. It's also harder to read. And a solution to stay API compatible and allow non-regex strings can be easy implemented. I implemented already a solution in Bug 201052. The necessary code-changes in the parts which actual check if the pattern fits are really small: Before: public boolean isMatch(String toMatch) { return pattern.matcher(toMatch).matches(); } After: public boolean isMatch(String toMatch) { if (nonRegExp) { return patternString.equals(toMatch); } return pattern.matcher(toMatch).matches(); } To make things easier I recommend to implement the option to commit non-regexp strings as: an option. An extra boolean field which determines if the string is a reg-exp or not.
Created attachment 86467 [details] Implementation of nonregular expression strings If you apply this patch and don't want to use regular expressions for your activityPatternBindings, you can set the option "noRegularExpression" to true, and your pattern string will be interpreted as plain simple string. That means, it will be processed as: "pattern.equals(id)", instead of "pattern.matcher(id).matches(); /* where 'pattern' is a compiled reg-ex pattern */".
I'm not opposed to this shorthand notation but there problems with this patch. Firstly, the schema xml doesn't refer to this new attribute. Secondly, returning null for the Pattern when this shortcut is applied isn't okay - it's API that it never returns null so it can't now. Also, in the event the short cut IS in place for a given binding, there's no way to get the matching string. I would suggest the following. a) add the attribute (with documentation) to the schema b) always return a pattern for the IActivityPatternBinding. If it's using your new attribute simply create a new Pattern on request from the match string - this should be easy enough: just escape the string such that it's a valid regex string. Mention in the javadoc for this method that if the attribute is true you're better off referring to the new get* method. You can still use the raw string in an equality check - I'm not suggesting you undo that - just allow the old API to work without change. c) add another get method to return the raw string used for equality testing. This can probably work with or without your new attribute but if you only want to support it for your new attribute that's fine with me. d) add some tests to the Activities test suite in org.eclipse.ui.tests to verify this new notation works.
Created attachment 86545 [details] Implementation of nonregular expression strings - v2 Should include all the requested changes from comment #2.
Unfortunately we can't use this as is - Pattern.quote (used in ActivityPatternBinding) is part of Java 1.5 and we have to be Java Foundation compliant (with some exceptions in Activities for 1.4). Pattern.quote will need to be implemented using <=1.4 APIs
Created attachment 86579 [details] Implementation of nonregular expression strings - v3 Exasperatingly I had a wrong configured jre setting, with an compliance level of Java 1.4 but the library setting was on version 1.5 I corrected it and enhanced additionally the junit test a little bit. But I'm not 100% sure if the way I corrected it is OK, I took the "quote()" code from Java 1.5 and copied it into the Eclipse code (with slight modifications). Since both sources are open source it should be no problem. Although the licenses aren't 100% compatible (Eclipse's EPL and Sun's GPL v2), I don't see where they bite each other. Additionally I gave credit in the javadoc and wrote where the source was taken from. I hope this is no problem, since it are only a dozen lines of code, and any implementation would end almost in the same solution.
Ouch! Jan, I can't even look at that patch if it has SDK code in it no matter how little it is. We cannot risk this whatsoever. I'm not sure how to proceed now. McQ, given that Jan's already looked at the code what should happen here? If he reimplements it from scratch will his implementation be tainted by association to this admission? Denis, what should happen here? There's no chance of that code ever making it into Eclipse and even having it as a patch makes me uncomfortable. Should that attachment be deleted?
(In reply to comment #6) > Denis, what should happen here? There's no chance of that code ever making it > into Eclipse and even having it as a patch makes me uncomfortable. Should that > attachment be deleted? > I don't mind deleting it -- it leaves a nice trail here anyway as to why it was deleted. Just say 'go', and confirm you want me to delete attachment 86579 [details] cc'ing Janet in case there is other legal stuff.
(In reply to comment #7) > > > I don't mind deleting it -- it leaves a nice trail here anyway as to why it was > deleted. Just say 'go', and confirm you want me to delete attachment 86579 [details] > > cc'ing Janet in case there is other legal stuff. > Provided Janet has no objections I'd like to see it deleted.
The content of attachment 86579 [details] has been deleted by Denis Roy <denis.roy@eclipse.org> who provided the following reason: As requested by Kim Horne. The token used to delete this attachment was generated at 2008-01-10 15:35:44 -0400.
Janet concurred in an IM message.
And now? The actions are understandable, should I rewrite the code (replacing the StringBuilder with "+" - for example)? Although: The code would look almost the same, and so everybody can come up with: "Hey, that's JDK 5.0 code, you just tricked a little bit to obfuscate us". (It's now already a little bit changed!) In the end this means: because of these license problems nobody is ever allowed to convert a plain string into a regular expression string. At least not by himself with his own code. If he uses something below JDK 5, he must life with it and will never have the allowance to convert his strings! Aren't there some IT law experts (at IBM maybe) who can take a short look at the code? Although I know (e.g.) the US law is very strict on that, I didn't know that the restrictions were so hard, especially since all licenses are open source. If this is the case I have to pay better more attention what I write, when I expand JDK classes, maybe it has already been implemented in almost the same way in a later JDK class - not unlikely if the methods are small (~12 lines). That makes me of course a little bit nervous.
Jan, different kinds of open source don't always go together well. We cannot just take some open source code and include it in the Eclipse codebase. One possible start for reading more about this would be: http://en.wikipedia.org/wiki/Eclipse_Public_License#Compatibility (Bjorn, does the Eclipse Foundation have a web page that explains this in plain English? I would rather point to that rather than a Wikipedia article.) In the concrete case, I would recommend that we get someone other than you to write the required functionality. Could you write us a JUnit test case for the hypothetical method PatternUtil.quotePattern(String)? Something like: public class PatternUtilTest extends TestCase { public void testQuotePattern() { assertEquals("", PatternUtil.quotePattern("")); assertEquals("some string", PatternUtil.quotePattern("some quoted string")); // ... and so on... } }
(In reply to comment #12) > In the concrete case, I would recommend that we get someone other than you to > write the required functionality. Am I already so "spoiled" (from the view of law) that I can't write this function anymore? I often run through the JRE code while debugging and stepping into it, I saw it, I understood it, I saved it in my memory - and maybe I used knowledge from that here and there in my own methods which work almost identical. Doesn't that mean I'm never allowed again to program any code with a license which interferes with the GPLv2 without proofreading it against the JDK? And what if the new code from someone else comes out too be in the principle identical, would it be lawyer proof? Can't this person then be sued too? I mean: can't I rewrite it, and maybe totally "screw it up" (but still working of course)? Wouldn't that be enough? I'm really not firm with this law stuff, it would be great if someone could give me an elaborate answer.
(In reply to comment #12) > (Bjorn, does the Eclipse Foundation have a web page that explains this in plain > English? I would rather point to that rather than a Wikipedia article.) I wish we did, but no. (In reply to comment #13) > I'm really not firm with this law stuff, it would be great if someone could > give me an elaborate answer. I'm sorry, but the Foundation cannot provide legal advice. However, we can say that if you say "I took the "quote()" code from Java 1.5 and copied it into the Eclipse code", then you are violating the licenses.
Created attachment 86927 [details] Non regular expression string activity pattern bindings - with self-coded quote() I hope this patch will pass. It's a real different programmed "quote()" function, which has also been moved to PatternUtil.quotePattern(). The quotePattern() method got also a few unit tests.
Created attachment 87051 [details] Patch against org.eclipse.ui, org.eclipse.ui.tests, and org.eclipse.ui.workbench Same as the last patch, with the following changes: - noRegularExpression has been renamed to isEqualityPattern to remove negative logic. - isNonRegExp is similarly changed to isEqualityPattern for the same reason. Additionally, javadoc in IActivityPatternBinding has been cleaned up (removed reference to Pattern.quote and added since tags) - ActivityPatternBinding.getString() now returns pattern.pattern() rather than toString() (which is unspec'd for Pattern). Also, the ActivityPatternBinding .toString() method has been updated to reflect the new fields
This patch is just at the cusp of being too large to commit without going through the IP process. Given the scare we had with JDK code I'm going to air on the side of caution and run it through the process. An IPZilla entry has been created at https://dev.eclipse.org/ipzilla/show_bug.cgi?id=1994.
Jan: before we commit this we need you to confirm the following in this bug report: a. Confirm you authored 100% of the code b. Confirm you have the rights to donate the code to Eclipse c. Confirm you are submitting the code for inclusion in future Eclipse releases under the Eclipse Public License
Created attachment 87138 [details] Patch against org.eclipse.ui, org.eclipse.ui.tests, and org.eclipse.ui.workbench > a. Confirm you authored 100% of the code > b. Confirm you have the rights to donate the code to Eclipse > c. Confirm you are submitting the code for inclusion in future Eclipse > releases under the Eclipse Public License 1.) I confirm all of this. I think I don't have to fax you a manually signed letter/contract or whatever, do I? Does a forum thread entry count? 2.) After this should be the final patch I wanted to polish it first, before I let it loose on humanity: - Polish the comments. What has already been done by you, thanks. - Finish the job. That includes updates of methods which must reflect the changes that happened to their classes. The "toString()" of "ActivtyPatternBindingDefinition", and all the "equal()" and "compareTo()" methods. - Update the example in activities.exsd with the new attribute name. I hope that in this case you don't have to re-transmit and re-evaluate the IP of this patch, do you?
*sigh* The last patch has already been put through the IP process. Janet: is it too late to attach another patch? Or could we instead proceed with the last version and later create new bugs with separate patches (all less than 250 lines) and put them in without going through IPZilla?
The current attachment has not yet been reviewed. Feel free to obsolete that attachment and give us your latest. Janet (In reply to comment #20) > *sigh* The last patch has already been put through the IP process. > > Janet: is it too late to attach another patch? Or could we instead proceed > with the last version and later create new bugs with separate patches (all less > than 250 lines) and put them in without going through IPZilla? >
Jan: do you have an ETA on when the the updated patch will be ready?
(In reply to comment #22) > Jan: do you have an ETA on when the the updated patch will be ready? Big misunderstanding. Sorry, sorry, sorry. Should have made that clearer: My patch was included in "Comment #19" where I said that I had to polish the patch first. comment #19: -- Comment #19 From Jan-Hendrik Diederich 2008-01-17 04:35:49 -0400 [reply] Created an attachment (id=87138) [details] Patch against org.eclipse.ui, org.eclipse.ui.tests, and org.eclipse.ui.workbench Sorry. But the patch I mentioned in comment #19 was the patch in comment #19 itself. Please transmit it, thanks in advance.
Oh geez, I'm sorry Jan! I didn't notice that attachment at all. Last round of comments: * I didn't notice last time that PatternUtil was in an API package. Does this class need to be API? Can this be moved to the internal activities package? * ActivityPatternBinding.compare is broken. You compare isEqualityPattern to itself. If you can spin a patch that either addresses or justifies the first one and fixes the second one I think we're done.
Created attachment 87540 [details] Patch against org.eclipse.ui, org.eclipse.ui.tests, and org.eclipse.ui.workbench Everything should now be fixed. I fixed the bug and moved PatternUtil to activities.internal. After that I rechecked everything and it should be OK now. But I have one big wish: it's OK to have PatternUtil in activities.internal for _now_, but generally I would like to share it with the "the whole world". Everybody should be able to use it. It's an absolute basic static function, without any requirements (except Java 1.4). You should be able to access it from everywhere at every time. It could really enrich the Eclipse environment, since you can use it everywhere where you have regular expression strings, it would be really great if it were in such a basic package like "org.eclipse.core.helpers" or whatever (this package doesn't really exist, it's just an example name). So please, make this function and functions like these public. I'm absolute sure I will need this in the future, and it may have nothing to do with the UI or similar things.
(In reply to comment #25) > Created an attachment (id=87540) [details] > Patch against org.eclipse.ui, org.eclipse.ui.tests, and > org.eclipse.ui.workbench > > Everything should now be fixed. > I fixed the bug and moved PatternUtil to activities.internal. After that I > rechecked everything and it should be OK now. > But I have one big wish: it's OK to have PatternUtil in activities.internal for > _now_, but generally I would like to share it with the "the whole world". > Everybody should be able to use it. It's an absolute basic static function, > without any requirements (except Java 1.4). You should be able to access it > from everywhere at every time. It could really enrich the Eclipse environment, > since you can use it everywhere where you have regular expression strings, it > would be really great if it were in such a basic package like > "org.eclipse.core.helpers" or whatever (this package doesn't really exist, it's > just an example name). > So please, make this function and functions like these public. I'm absolute > sure I will need this in the future, and it may have nothing to do with the UI > or similar things. > We've never been particularly good at finding appropriate homes for utility code but that should be on our minds for 4.0. For the immediate future this patch should be put through the IP system.
(In reply to comment #26) > For the immediate future this patch should be put through the IP system. Full ACK.
Fix in HEAD. Thanks Jan!
Verified this works in I20080205-0010 by the test cases and a manual test scenario.
