Community
Participate
Working Groups
Unfortunately the addSelectionListener method in Button uses a non-functional interface. To use lambdas with SWT Button as event handler for Button I suggest to add a new method to Button called addOnClickListener similar to the following public void addOnClickListener (OnClickListener listener) { checkWidget (); if (listener == null) error (SWT.ERROR_NULL_ARGUMENT); TypedListener typedListener = new TypedListener (listener); addListener (SWT.Selection,typedListener); } public interface OnClickListener extends SWTEventListener { public void onClick(SelectionEvent e); } Some links for more information: http://docs.oracle.com/javase/8/docs/api/java/util/function/package-summary.html http://developer.android.com/reference/android/view/View.OnClickListener.html
This looks like a good path forward but in order to keep consistency it should be introduced as reliable pattern through whole SWT api(e.g. CollapseListener, ExpandListener, ControlMoved, ControlResized, KeyPressed, KeyReleased and etc.). I understand that many listeners are functional interfaces now but still coordinated effort might be better.
(In reply to Alexander Kurtakov from comment #1) > but still coordinated effort might be better. Sounds good to me, but I think implementation wise I think we should address it peace-wise, most common cases (like Button) first, afterwards "not so common" cases, like ControlMoved. Maybe we can have one umbrella bug and bugs per listener.
The idea sounds good, but it can only really be assessed once we have a complete proposed implementation for at least one of the typed listener interfaces. With API design, there's often a bunch of problems that you only see when the first version is done. In Eclipse, the policy is that we don't release this first version, but we first polish it and only release it when we're confident about it. Breaking up a well-structured API with random "shortcuts" is not the way to go.
(In reply to Markus Keller from comment #3) > The idea sounds good, but it can only really be assessed once we have a > complete proposed implementation for at least one of the typed listener > interfaces. Sounds like my plan (start with one addOnClickListener implementation for Button and validate if it works). I just wanted to validate before doing any implementation work if others see issues with the idea.
We already have the untyped Listener, which is a functional interface. I generally prefer the untyped listeners to the typed ones because they are simple and low-level, as SWT is. The same pattern `addListener(type, callback)` is used in web APIs (e.g. jQuery) as well and it doesn't seem too hard to understand. And I don't recall a lot of cases where the type safety of typed events would have been a real benefit. I guess the proposed listeners would eventually make the existing typed listeners and listener adapters obsolete. However, the existing listeners can't be removed, so it's just yet another way to do the same thing. While the additional listener and event types may not hurt, the additional add methods would further bloat the public interface of every widget, which is already huge. I image that we'd end up with addKeyUpListener, addKeyDownListener, addKeyPressListener, ...
> public void addOnClickListener (OnClickListener listener) { The names would follow SWT conventions, e.g.: public void addWidgetSelectedListener (WidgetSelectedListener listener) { .. } And the combined interfaces would extend the functional interfaces, e.g: public interface SelectionListener extends WidgetSelectedListener, WidgetDefaultSelectedListener, SWTEventListener { .. } -- Another variant to explore is to declare functional interfaces as subtypes of existing listener interfaces: @FunctionalInterface interface WidgetSelectedListener extends SelectionListener { @Override default void widgetDefaultSelected(SelectionEvent e) { // do nothing } } Clients can already do this today and then use the interface like this: button.addSelectionListener((WidgetSelectedListener) e -> { System.out.println("selected " + e); });
(In reply to Markus Keller from comment #6) > The names would follow SWT conventions, e.g.: > public void addWidgetSelectedListener (WidgetSelectedListener listener) { .. > } > And the combined interfaces would extend the functional interfaces, e.g: > > public interface SelectionListener extends WidgetSelectedListener, > WidgetDefaultSelectedListener, SWTEventListener { .. } > Thanks for the suggestion: https://git.eclipse.org/r/35703 I only adjust the implementation of the Button in /org.eclipse.swt/Eclipse SWT/gtk folder. Where can I find the implementations of the other platforms for Button?
I have also written a test for this, but it looks to me that I first need to change SWT and than then add the test, as tests do not see the new interfaces.
There are /org.eclipse.swt/Eclipse SWT/cocoa and /org.eclipse.swt/Eclipse SWT/win32 folders with org.eclipse.swt.widgets.Button implementations for the other two supported UI toolkits. You have to add the new method in them too.
https://git.eclipse.org/r/#/c/35703/ is now ready for review. Would be nice to get this in for M4 or to get feedback what should be changed. THanks Alexander for the information.
Lars, please respect the component lead's scheduling. And as I already said in comment 3, this is not ready for release. We'd need examples that use the new APIs, and we'd need to be sure this is the right way to go. The concerns in comment 5 have not even been answered. The removal of .classpath_gtk is definitely wrong. I haven't looked closely at the API filters, but they also look wrong (or at least we'd need a bug report if there's something wrong with the tooling). It's also strange that you find time to remove the initial copyright holder for code that you merely moved into a new file.
Thanks Markus for the feedback (In reply to Markus Keller from comment #11) > Lars, please respect the component lead's scheduling. I assumed the target was moved because the patch was not ready. After finishing the patch this morning I moved it back to bring it again to the attention of the project lead. I think it would help if a reason is given why a bug is rescheduled to avoid this. > And as I already said in comment 3, this is not ready for release. We'd need > examples that use the new APIs, and we'd need to be sure this is the right > way to go. I tried to address the comment 3, by providing a complete proposed implementation for at least one of the typed listener interfaces. Examples in the platform are hard, as this would require to update a part of the platform to Java 8. Or is the proposal to use the new API with anonymous classes? > The removal of .classpath_gtk is definitely wrong. I haven't looked closely > at the API filters, but they also look wrong (or at least we'd need a bug > report if there's something wrong with the tooling). Thanks, I fix these. > It's also strange that you find time to remove the initial copyright holder > for code that you merely moved into a new file. This seems to be handled inconsistently within platform, in the past I did not modify copyright in similar cases and people complained, now you complain about the modification. :-) I'm happy to change that back, please add a comment to the Gerrit review, if that is the right thing to do, I would like to avoid ping-pong here.
> (In reply to Markus Keller from comment #11) > The removal of .classpath_gtk is definitely wrong. Fixed in Gerrit > I haven't looked closely at the API filters, but they also look wrong API tooling wants to generate them, please let me know what is wrong with them, so that I can open a bug for the API tools.
As said before, we won't release a partial fix for just one listener (SelectionListener) and just one observable class (Button). So the Gerrit may be ready for review, but not for release. I agree that using lambdas in the SDK is not feasible at the moment, but you could show the new APIs in use e.g. by updating the ControlExample or a few Snippets. As explained in comment 5, API bloat is a real concern. The updated client code must be clearly smaller and easier to understand, and we also have to explore all the proposed alternatives and see how they compare to your proposed solution.
(In reply to Markus Keller from comment #14) > As said before, we won't release a partial fix for just one listener > (SelectionListener) and just one observable class (Button). So the Gerrit > may be ready for review, but not for release. If I understand that correct in the context of what you said before (Quote: "it can only really be assessed once we have a complete proposed implementation for at least one of the typed listener interfaces") I need to to the rest of the SelectionListener, e.g. TextView. I do this as soon as possible. > I agree that using lambdas in the SDK is not feasible at the moment, but you > could show the new APIs in use e.g. by updating the ControlExample or a few > Snippets. Sounds good to me, if I understand this correctly, it is OK to update ControlExample to Java 8 and use the new API. > and we > also have to explore all the proposed alternatives and see how they compare > to your proposed solution. +1 Just to clarify the solution proposal come from you in comment 6. Thanks again for your help. I try to get this done early M5, I don't think there is any rush required for M4.
(In reply to Lars Vogel from comment #15) > (In reply to Markus Keller from comment #14) > > As said before, we won't release a partial fix for just one listener > > (SelectionListener) and just one observable class (Button). So the Gerrit > > may be ready for review, but not for release. > > If I understand that correct in the context of what you said before (Quote: > "it can only really be assessed once we have a complete proposed > implementation for at least one of the typed listener interfaces") I need to > to the rest of the SelectionListener, e.g. TextView. I do this as soon as > possible. > > > I agree that using lambdas in the SDK is not feasible at the moment, but you > > could show the new APIs in use e.g. by updating the ControlExample or a few > > Snippets. > > Sounds good to me, if I understand this correctly, it is OK to update > ControlExample to Java 8 and use the new API. This is a change which when we are going to adopt for lambdas must happen. I actually consider more gradual path - Java 1.6, 1.7... to not end up with tons of new warnings in my workspace at once. > > > and we > > also have to explore all the proposed alternatives and see how they compare > > to your proposed solution. > > +1 Just to clarify the solution proposal come from you in comment 6. > > Thanks again for your help. I try to get this done early M5, I don't think > there is any rush required for M4.
I have now Button and Text in included in the change: https://git.eclipse.org/r/#/c/35703/ Now I have to replicate this work for the other widgets: http://help.eclipse.org/juno/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fapi%2Forg%2Feclipse%2Fswt%2Fevents%2Fclass-use%2FSelectionListener.html This is a time consuming task but will not introduce any new concept. Can someone have a first look at the current patch and tell me if this looks like the correct direction? I would like to avoid spending now hours and hours for the rest of the widgets only to hear that we have a blocker and to get a "wontfix". Tonight I upload a change for the ControlExample as requested by Markus, this will be for demo purpose and not necessary to get integrated therefore I do this in a new Gerrit review.
(In reply to Markus Keller from comment #14) > I agree that using lambdas in the SDK is not feasible at the moment, but you > could show the new APIs in use e.g. by updating the ControlExample or a few > Snippets. I created a few examples in the SWT snippets. I think the new API looks awesome and elegant. Please let me know what you think.
(In reply to Lars Vogel from comment #18) > I created a few examples in the SWT snippets. I think the new API looks > awesome and elegant. Please let me know what you think. https://git.eclipse.org/r/38384
What is the advantage of cancel.addWidgetSelectedListener(e -> System.out.println("Cancel")); compared to cancel.addListener(SWT.Selection, e -> System.out.println("Cancel")); ?
(In reply to Ralf Sternberg from comment #20) > What is the advantage of > cancel.addWidgetSelectedListener(e -> System.out.println("Cancel")); > compared to > cancel.addListener(SWT.Selection, e -> System.out.println("Cancel")); Isn't that a general question, why we have typed listeners? IMHO opinion typed listeners are useful because they provide better Javadoc and a fittercontent assists. If you look at platform.ui code the untyped listener is very rarely used.
fittercontent assists -> better content assists
Can someone from SWT team give feedback on the current implementation? I think the approach is clear now, what is left is "only" to do the same change to the remaining widget. I can do this but would like to get first some feedback if that approach is OK for the SWT team.
I don't think this development blocks Bug 455399 or 455403. IMHO their is no strong dependency, I would be nice (but not blocking) to upgrade the snippets / examples to Java 8 to have a better test ground for this. But upgrading the examples to Java 8 is IMHO the correct way to demonstrate customers the best usage of SWT API together with Java 8.
(In reply to Lars Vogel from comment #23) > Can someone from SWT team give feedback on the current implementation? > > I think the approach is clear now, what is left is "only" to do the same > change to the remaining widget. I can do this but would like to get first > some feedback if that approach is OK for the SWT team. Hi Lars, thank you for the effort and sorry for the delay in responding. We’ve tried to evaluate the number of new APIs (methods and interfaces) that have to be added. For completeness, we have to address all the event handlers notified by the Control class and its subclasses, i.e, we can’t do this only for SelectionListener or FocusListener. This would mean adding about 15+ new interfaces and adding the necessary add*Listener methods to the classes. This API bloat in SWT is a real concern. The typed listeners in SWT grouped the related events in a single interface. The adapter classes were added to provide default implementations for these interfaces. By adding the new set of functional interfaces we would be breaking up the well grouped typed listeners, for a specific purpose and making the adapters obsolete. Having 2 sets of typed interfaces for the same task could lead to confusion. I don't think it is correct to recommend using one interface against the other to the client, as mentioned in the javadoc. I think we should discuss the above concerns and the pros-cons of the implementation before moving forward.
Too late for 4.5 I assume, maybe we can discuss that again for 4.6.
See also http://stackoverflow.com/questions/21833537/java-8-lambda-expressions-what-about-multiple-methods-in-nested-class/23373921#23373921 , where Brian Goetz says the Lambda EG recommends static helper methods like this: static SelectionListener widgetSelected(final Consumer<SelectionEvent> c) { return new SelectionAdapter() { @Override public void widgetSelected(SelectionEvent e) { c.accept(e); } }; }
(In reply to Markus Keller from comment #27) > See also > http://stackoverflow.com/questions/21833537/java-8-lambda-expressions-what- > about-multiple-methods-in-nested-class/23373921#23373921 , where Brian Goetz > says the Lambda EG recommends static helper methods like this: > > static SelectionListener widgetSelected(final Consumer<SelectionEvent> c) { > return new SelectionAdapter() { > @Override > public void widgetSelected(SelectionEvent e) { > c.accept(e); > } > }; > } Thanks for sharing this, Markus! As far as I understand, we can't have these helper methods in the SWT project (at least as of now) as we are at Java compliance level 1.5 and these would require Java 1.8. Is it right?
(In reply to Lakshmi Shanmugam from comment #28) > (In reply to Markus Keller from comment #27) > > See also > > http://stackoverflow.com/questions/21833537/java-8-lambda-expressions-what- > > about-multiple-methods-in-nested-class/23373921#23373921 , where Brian Goetz > > says the Lambda EG recommends static helper methods like this: > > > > static SelectionListener widgetSelected(final Consumer<SelectionEvent> c) { > > return new SelectionAdapter() { > > @Override > > public void widgetSelected(SelectionEvent e) { > > c.accept(e); > > } > > }; > > } > > Thanks for sharing this, Markus! > As far as I understand, we can't have these helper methods in the SWT > project (at least as of now) as we are at Java compliance level 1.5 and > these would require Java 1.8. Is it right? Unless we bump the min version to Java 1.8, which is fair option nowadays I would say.
Already now, the typed listeners are not easy to use because the documentation is spread across 3 places: 1. Javadoc of the add*Listener(*Listener) method (e.g. MenuItem#addSelectionListener(SelectionListener) tells which methods are called and what event fields will be valid) 2. Javadoc of the *Listener#*(*Event) methods 3. Javadoc of the *Event class fields (and such field of superclasses, see e.g. DragDetectEvent) We can also take this bug as a trigger to clean up the Javadocs. The connections from the SWT#* event type constants to the typed listeners are mostly available, but the reverse links from the add*Listener(*Listener) methods to the SWT#* event type constants are missing. Once the event type constants are properly documented (see also bug 81334), I don't see much advantage in adding a second layer of typed listeners with FunctionalInterface types.
(In reply to Lakshmi Shanmugam from comment #28) > As far as I understand, we can't have these helper methods in the SWT > project (at least as of now) as we are at Java compliance level 1.5 and > these would require Java 1.8. Is it right? Yes, we can't just release such helper methods right now. We could apply tricks and compile against a 1.5-compatible variant of java.util.function.Consumer<T>, but that would be awkward, and the class holding the helper methods would only be loadable on 1.8.
(In reply to Markus Keller from comment #31) > (In reply to Lakshmi Shanmugam from comment #28) > > As far as I understand, we can't have these helper methods in the SWT > > project (at least as of now) as we are at Java compliance level 1.5 and > > these would require Java 1.8. Is it right? Plug-in can upgrade to Java 8 if they require Java 8 functionality. This sounds like a good reason. I opened Bug 481195 for an update of SWT to Java 1.8.
Resetting target as there is nothing to be done here for 4.6.
(In reply to Markus Keller -- away till June 25 from comment #27) > See also > http://stackoverflow.com/questions/21833537/java-8-lambda-expressions-what- > about-multiple-methods-in-nested-class/23373921#23373921 , where Brian Goetz > says the Lambda EG recommends static helper methods like this: > > static SelectionListener widgetSelected(final Consumer<SelectionEvent> c) { > return new SelectionAdapter() { > @Override > public void widgetSelected(SelectionEvent e) { > c.accept(e); > } > }; > } I believe consuming code would look like the following: Button buttonWithLambdaListener = new Button(parent, SWT.PUSH); buttonWithLambdaListener.setLayoutData(new GridData(SWT.BEGINNING, SWT.CENTER, false, false)); buttonWithLambdaListener.setText("Lambda"); buttonWithLambdaListener.addSelectionListener(SelectionListener.widgetSelected(e -> { System.out.println("Button has been clicked!"); })); Definitely an improvement to the current situation, but I personally would prefer splitting the SelectionInterface in separate interfaces, especially as in most cases the widgets only supports either widgetSelected or widgetDefaultSelected. SWT team, which option do you prefer?
+1! I happened upon the implementation in the second half of comment 6, and it works great for me, but I understand the concern about API bloat. In that case, I vote for the suggestion in comment 29. In any case, I do think this is really important since it makes SWT less functional which is becoming increasingly more important as OO and functional programming paradigms intersect in JVM languages. Scala will soon have bridges for Java 8 classes like Consumer which will make the implementation suggested in comment 29 very Scala-friendly.
(In reply to John Dimeo from comment #35) > +1! > > I happened upon the implementation in the second half of comment 6, and it > works great for me, but I understand the concern about API bloat. > > In that case, I vote for the suggestion in comment 29. > > In any case, I do think this is really important since it makes SWT less > functional which is becoming increasingly more important as OO and > functional programming paradigms intersect in JVM languages. Scala will > soon have bridges for Java 8 classes like Consumer which will make the > implementation suggested in comment 29 very Scala-friendly. We have just moved SWT to Java 8 for Oxygen M1. The road for patches using static helpers is open. If someone disagrees time to speak now before someone sends patch for review.
(In reply to Alexander Kurtakov from comment #36) > (In reply to John Dimeo from comment #35) > > +1! > > > > I happened upon the implementation in the second half of comment 6, and it > > works great for me, but I understand the concern about API bloat. > > > > In that case, I vote for the suggestion in comment 29. > > > > In any case, I do think this is really important since it makes SWT less > > functional which is becoming increasingly more important as OO and > > functional programming paradigms intersect in JVM languages. Scala will > > soon have bridges for Java 8 classes like Consumer which will make the > > implementation suggested in comment 29 very Scala-friendly. > > We have just moved SWT to Java 8 for Oxygen M1. The road for patches using > static helpers is open. If someone disagrees time to speak now before > someone sends patch for review. In that case, the one change that would solve this for the vast majority of use cases is to add a default interface implementation of widgetDefaultSelected that either does nothing or calls widgetSelected for SelectionListener. That would allow "direct" lambda usage for selection, which I feel like (at least in my code) the place where it hurts the most to not have lambdas (since others like paint are already functional or like mouse where I'm overriding more than one callback)
New Gerrit change created: https://git.eclipse.org/r/79618
(In reply to Eclipse Genie from comment #38) > New Gerrit change created: https://git.eclipse.org/r/79618 This patch follows Markus suggestion from https://bugs.eclipse.org/bugs/show_bug.cgi?id=441116#c27
The overloaded SelectionListener#widgetSelected(Consumer<SelectionEvent>) will cause compile errors in some client code that still compiles against JavaSE-1.7. I got compile errors in JDT and EGit with the dreaded message "The type java.util.function.Consumer cannot be resolved. It is indirectly referenced from required .class files". The problems in JDT's AbstractConfigurationBlock and MarkOccurrencesConfigurationBlock can be fixed with an explicit cast like this: listener.widgetSelected((SelectionEvent) null); For the problem in EGit's org.eclipse.egit.ui.UIUtils#addExpansionItems(ToolBar, AbstractTreeViewer), I didn't see a solution other than bumping EGit to 1.8, which they probably won't do because they want to stay compatible with older Eclipse releases. However, I can't explain right away why org.eclipse.jdt.internal.ui.callhierarchy.ExpandWithConstructorsConfigurationBlock#createContents(Composite) doesn't suffer from the same problem -- the code looks very similar to the EGit case.
Gerrit change https://git.eclipse.org/r/79618 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=c566830d764d4719a4578c99d1f8d6b624b753e2
Merged into master.
Fixed the JDT UI compile errors with: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=89da2ef66b1fced4c1d4ff81ea9622ada9841256 (In reply to Eclipse Genie from comment #41) > Gerrit change https://git.eclipse.org/r/79618 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=c566830d764d4719a4578c99d1f8d6b624b753e2 The @since tags are missing with the new APIs and the formatting of the new #widgetDefaultSelected is bad (extra indentation on the first line of method declaration).
New Gerrit change created: https://git.eclipse.org/r/80859
Gerrit change https://git.eclipse.org/r/80859 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=e3f9ade94753ec7978dd4f00a69442e4acec0e00
New Gerrit change created: https://git.eclipse.org/r/80890
Gerrit change https://git.eclipse.org/r/80890 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=5a63e95697b5d2f6f6cc6c3bb474987c2c78e5fa
(In reply to Alexander Kurtakov from comment #42) > Merged into master. Did you inform EGit and provide them with a solution or workaround?
(In reply to Dani Megert from comment #48) > Did you inform EGit and provide them with a solution or workaround? Email send to EGit-dev and opened Bug 501220 for follow-up action.
(In reply to Markus Keller from comment #40) > For the problem in EGit's > org.eclipse.egit.ui.UIUtils#addExpansionItems(ToolBar, AbstractTreeViewer) I cannot reproduce with N20160911-0220, created Bug 501220 for discussion.
(In reply to Lars Vogel from comment #50) > (In reply to Markus Keller from comment #40) > > For the problem in EGit's > > org.eclipse.egit.ui.UIUtils#addExpansionItems(ToolBar, AbstractTreeViewer) > > I cannot reproduce with N20160911-0220, created Bug 501220 for discussion. It depends on what you have in your workspace and how the compiler compiles things. JDT UI also compiles if all other projects are taken from the target platform. Markus is currently taking a look at the compiler issue. Depending on the outcome we will decide today whether to revert the SWT fix or not. Revert would be done if an impact on many projects is expected.
This change will be in M2. The problems with the compile errors issued by JDT Core are captured in bug bug 501220.
I suspect that re-using existing method names unnecessarily forces the compiler into resolving things it should better not try to resolve. I'm not yet convinced that bug 501220 is indeed a bug in the compiler (partly due to my inability to reproduce). Could SWT consider using fresh method names instead, to avoid complications for clients?
I pushed a compiler fix, but I don't think it's prudent to expect all SWT clients to upgrade to an unreleased compiler build right now. I have no indications for a compiler regression, whereas I can't think of a good reason for introducing the dangerous situation in SWT in the first place.
Carried over from the other bug: (In reply to Lars Vogel from bug 501220 comment #19) > (In reply to Stephan Herrmann from bug 501220 comment #16) > > > Thanks, but mind you that this is not solved by SWT moving to a new > > compiler. All SWT *clients* will (potentially, viz. when using null > > annotations) have to upgrade their compiler. > > If I understand the statements from Markus correctly, this only affects > clients using Java 7 and SWT in 4.7 requires Java 8. From my understanding for those clients it is a source-incompatible change: they will have to insert a cast or such to make existing code compile again. Additionally, clients using null annotations (even at Java 8) will need to upgrade their compiler. I see no reason for lightly dismissing these problems.
Thanks Stephan for insisting on a good coding style. I had mentioned that ruling out such problems is a prerequisite for adding new APIs, but that has been ignored. Reopening this bug. Since the static factory methods don't do anything close to the existing abstract methods, overloading is indeed bad style. The methods should be renamed. The first idea that came to my mind was "widgetSelectedAdapter". Another idea would be something like "forWidgetSelected", which would look even better in client code that doesn't use static imports, e.g.: button.addSelectionListener(SelectionListener.forWidgetSelected(e -> { System.out.print("Hello"); })); However, with static imports, the first variant looks better: button.addSelectionListener(widgetSelectedAdapter(e -> { System.out.print("Hello"); })); Furthermore, there are no tests and not a single usage example in the SWT repo. This change should be reverted for M2 and only released after contributor duties have been fulfilled.
(In reply to Markus Keller from comment #40) > The overloaded SelectionListener#widgetSelected(Consumer<SelectionEvent>) > will cause compile errors in some client code that still compiles against > JavaSE-1.7. I have seen the compile problem too on a simple preference page. It appears that anyone who uses "SelectionListener" would have to update their source code to use 1.8 for the "Consumer" interface and then (I assume) implement the 'accept' method. I think that is probably just about everyone. Is that intended, or considered an ok thing to do? (I would not think so, but, I would be the last to know). If that is inteneded and considered "ok", I would suggest a general announcment about it and giving people time to adjust.
Regarding a rename - what about: button.addSelectionListener(selected(e -> { System.out.print("Hello"); })); Does selected/defaultSelected sounds fine? That would be even better from my POV and will not expose the bug.
(In reply to Alexander Kurtakov from comment > > Does selected/defaultSelected sounds fine? That would be even better from my > POV and will not expose the bug. +1 for this rename
New Gerrit change created: https://git.eclipse.org/r/81020
(In reply to Eclipse Genie from comment #60) > New Gerrit change created: https://git.eclipse.org/r/81020 That's the proposed change, let's have some time hearing from others about their opinion before pushing it. Arun, would you please state your opinion as project lead?
And ... we're back to field one, quickly proposing and supporting unreflected new variants and letting others spend time on evaluating your "ideas". Did you go over the other listener interfaces to see if that approach can be applied consistently? Did you realize that code like ... > button.addSelectionListener(selected(e -> { ... requires clients to use a static import for that method name, and that this only works once per CU and that it would cause conflicts with other methods called "selected"? Maybe it will work out, maybe not. I don't know without investigating more. But I do know that making API decisions in that way is exactly the opposite of what made Eclipse successful.
(In reply to David Williams from comment #57) > I have seen the compile problem too on a simple preference page. It appears > that anyone who uses "SelectionListener" would have to update their source > code to use 1.8 for the "Consumer" interface and then (I assume) implement > the 'accept' method. No, these are static helper methods that cannot / don't have to be implemented by subclasses. The only known harm to existing code is due to the overloading, which will be avoided in the final API. If your preference page is based on a copy of JDT UI's AbstractConfigurationBlock, then the workaround from comment 43 will not be necessary any more after the methods have been renamed. (And to be honest, that we passed null there is an API violation that is only acceptable because we have total control over the listeners on which we call that method.)
(In reply to Markus Keller from comment #62) > ... requires clients to use a static import for that method name, and that > this only works once per CU and that it would cause conflicts with other > methods called "selected"? Valid point. I prepare a change with Markus proposal (widgetSelectedAdapter and widgetDefaultSelectedAdapter) and let the SWT team decide.
New Gerrit change created: https://git.eclipse.org/r/81024
(In reply to Markus Keller from comment #63) > > If your preference page is based on a copy of JDT UI's > AbstractConfigurationBlock, then the workaround from comment 43 will not be > necessary any more after the methods have been renamed. I don't think based on that, but not sure. Maybe I was seeing something less (or more?) that others. It was a "1.7" bundle, and I was seeing a red-X error of The type java.util.function.Consumer cannot be resolved. It is indirectly referenced from required .class files And this was in the file: http://git.eclipse.org/c/platform/eclipse.platform.releng.buildtools.git/tree/bundles/org.eclipse.test.performance.ui/src/org/eclipse/test/internal/performance/results/ui/PerformanceResultsPreferencePage.java I did not actually try to build it. Perhaps it can be solved with some "setting" or something and not actually be a problem at build time. But, I started to look for "what's changed" and ran into this bug and perhaps jumped to conclusions.
(In reply to Markus Keller from comment #40) > I got compile errors in JDT and EGit with the dreaded message > "The type java.util.function.Consumer cannot be resolved. It is indirectly > referenced from required .class files". Yep, now I've updated SWT and see those errors too. Additionally to that, it's only me who gets the API errors on SWT HEAD (using Eclipse 4.7 Build I20160823-1359): Description Resource Path Location Type The default method org.eclipse.swt.events.SelectionListener.widgetDefaultSelected(Consumer<SelectionEvent>) in an interface has been added SelectionListener.java /org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/events line 35 Compatibility Problem The default method org.eclipse.swt.events.SelectionListener.widgetSelected(Consumer<SelectionEvent>) in an interface has been added SelectionListener.java /org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/events line 35 Compatibility Problem The major version should be incremented in version 3.106.0, since API breakage occurred since version 3.105.0 MANIFEST.MF /org.eclipse.swt/META-INF line 5 Version Numbering Problem I think someone should silence those API errors, since we don't want to bump SWT version to 4.0.0? Or do we? Or is it a PDE bug?
(In reply to David Williams from comment #66) I can confirm that this is the same problem as in EGit (1.7 project with null annotations enabled), and it's fixed by bug 501250 (e.g. in I20160914-0430) and/or by avoiding the overloaded methods in SWT. (In reply to Andrey Loskutov from comment #67) You shouldn't see such errors at this phase of the project. During test days like yesterday, committers must use the latest I-build, and they are expected to self-host on the latest I-build anyway. There were some API Tools problems in older I-builds.
Sorry for chiming in a bit late on this but I agree with most of the concerns expressed by Markus in comment 56 and comment 62 and think it is better to revert this API addition for M2 as its too late to iron out all the outstanding issues. I also think the approach suggested by John in comment 37 is worth considering even though it proposes to address the case of SelectionListener only. Markus, do you see any obvious problems with this suggestion?
(In reply to Arun Thondapu from comment #69) > Sorry for chiming in a bit late on this but I agree with most of the > concerns expressed by Markus in comment 56 and comment 62 and think it is > better to revert this API addition for M2 as its too late to iron out all > the outstanding issues. I think https://git.eclipse.org/r/#/c/81024/ follows Markus suggestions from comment 56 and comment 62.
(In reply to Lars Vogel from comment #70) > I think https://git.eclipse.org/r/#/c/81024/ follows Markus suggestions from > comment 56 and comment 62. I didn't make a concrete suggestion; I just proposed things to explore and said we should not rush in things that have not been thought through. Comment 37 indeed looks like a very good solution. I don't see problems with it right away. Something to explore in M3.
(In reply to Markus Keller from comment #71) > Comment 37 indeed looks like a very good solution. I don't see problems with > it right away. Something to explore in M3. +1 for the revert for M2. I still think the original proposal from Markus (routing back to Brian) from https://bugs.eclipse.org/bugs/show_bug.cgi?id=441116#c27 is the right approach as it allows you later to use the same approach for interfaces with more than one "important" method. But definitely something we should explorer early M3 to have enough time to find and solve issues. @Alex, can you revert the commit for M2?
Will do it now.
New Gerrit change created: https://git.eclipse.org/r/81117
Gerrit change https://git.eclipse.org/r/81117 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=2beeb567ac5ceebc065f66f8c8bb970b4cdd8445
(In reply to Eclipse Genie from comment #75) > Gerrit change https://git.eclipse.org/r/81117 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=2beeb567ac5ceebc065f66f8c8bb970b4cdd8445 Patches reverted. To be reconsidered for M3.
I evaluated https://bugs.eclipse.org/bugs/show_bug.cgi?id=441116#c37 and it is not a approach with we should apply as the general solution. Reason is, that we cannot apply this solution consistently to other listeners, e.g., MouseListener, FocusListener. We desired we also do this change in addition to the new helper methods but the helper methods are still necessary to allow lambda usage for all methods of an interface. The approach to define helper methods is also suggested by Brian Goetz and agreed approach before we discovered the JDT core bug. I updated https://git.eclipse.org/r/#/c/81024/ with collision free names to avoid the compiler bug. I suggest to merge it early in M3 to avoid the discovery of more "last minute" bugs, this time for M3.
Tests will be adjusted with Bug 501703.
(In reply to Lars Vogel from comment #77) > The approach to define helper methods is also suggested by Brian Goetz and > agreed approach before we discovered the JDT core bug. > > I updated https://git.eclipse.org/r/#/c/81024/ with collision free names to > avoid the compiler bug. Good, please just stop calling it a compiler bug. (from bug 501220 comment #22): > The change in this bugzilla improves resilience of the compiler in an > unspecified area. Even before the change the compiler was behaving correctly > (wrt the spec). > Good design, OTOH, stays clear of this unspecified area.
(In reply to Stephan Herrmann from comment #79) > Good, please just stop calling it a compiler bug. Will do. :-)
(In reply to Markus Keller from comment #40) > The overloaded SelectionListener#widgetSelected(Consumer<SelectionEvent>) > will cause compile errors in some client code that still compiles against > JavaSE-1.7. > > I got compile errors in JDT and EGit with the dreaded message > "The type java.util.function.Consumer cannot be resolved. It is indirectly > referenced from required .class files". > > The problems in JDT's AbstractConfigurationBlock and > MarkOccurrencesConfigurationBlock can be fixed with an explicit cast like > this: > > listener.widgetSelected((SelectionEvent) null); > > For the problem in EGit's > org.eclipse.egit.ui.UIUtils#addExpansionItems(ToolBar, AbstractTreeViewer), > I didn't see a solution other than bumping EGit to 1.8, which they probably > won't do because they want to stay compatible with older Eclipse releases. EGit 4.6.0-SNAPSHOT (now on master) has raised minimum BREE to Java 8
Gerrit change https://git.eclipse.org/r/81024 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=806829c269af091f858f3f4f7be3f6212583d81c
Merged now. Tests and further usage of the API can be applied only after N-build contains the API.
New Gerrit change created: https://git.eclipse.org/r/81605
Gerrit change https://git.eclipse.org/r/81605 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=69b56edf7d9a357a2257073a2158c96d5ae894db
Javadocs now include method references, and SelectionAdapter mentions the new helper methods: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=ea61ed53377c57cbd3b912cf5975bab94c7167a3
