Community
Participate
Working Groups
Extracted from bug 335792. This bug covers the work needed to add generics to the org.eclipse.core.databinding plugin.
Can this be done for M2?
No. I've been putting off a number of important tasks in order to get the observables and properties plugins done. This one needs to wait while I catch up on those other tasks. I'll come back to this, but only after higher-priority issues are finished. Why the urgency? It's been working fine as it is for a long time. (Also: Please don't submit my current work-in-progress patch in its current state. It still needs some careful review and testing.)
Sorry, Lars. I just re-read my last comment and it sounded a bit cold. I didn't mean it to come off like that. I'll definitely come back to this. When I mentioned submitting my changes, I was honestly just talking about the WIP patch I uploaded for this bug and not making a passive-aggressive reference to the last patch you submitted for me. :-)
(In reply to Stefan Xenos from comment #3) > Sorry, Lars. I just re-read my last comment and it sounded a bit cold. I > didn't mean it to come off like that. No worries, all is good. Thanks for working on this.
Mass move to 4.7 as M7 is approaching. Please move back in case you are planning to fix it for Neon.
Any progress here?
I'd like to do some work to hopefully have this issue solved. It seems to me that the necessary changes are mostly finished by Stefan Xenos, but they're sitting in a review branch here: https://git.eclipse.org/r/#/c/50300/ I plan to do the following: * Fix the build problem that the change apparently has. * Go through the comments on that change and update the implementation accordingly. * Rebase the change to the current master. * Update the bug number in the change to instead reference this bug. Would this work be useful? Would it mean that the change could be merged into master? Is something more needed to complete the work for this issue?
(In reply to Jens Lideström from comment #7) > I'd like to do some work to hopefully have this issue solved. > > It seems to me that the necessary changes are mostly finished by Stefan > Xenos, but they're sitting in a review branch here: > > https://git.eclipse.org/r/#/c/50300/ > > I plan to do the following: > > * Fix the build problem that the change apparently has. > * Go through the comments on that change and update the implementation > accordingly. > * Rebase the change to the current master. > * Update the bug number in the change to instead reference this bug. > > Would this work be useful? Would it mean that the change could be merged > into master? Is something more needed to complete the work for this issue? IMO work on this would be very useful. I can assist with code reviews, but I cannot merge it as I'm not a committer. I would suggest to split the work down in several patches, because adding generics is sometimes tricky without changing the API. IMO the Binding class in the mentioned patch should not get type parameters in the first patch and maybe not at all.
Conrad, If you review, I can merge it.
I tried to push the updated code to the old Gerrit change with this command: git push ssh://jlidestrm@git.eclipse.org:29418/platform/eclipse.platform.ui.git HEAD:refs/for/master Using the change ID of the above-mentioned change: I6ea2644d0a2395a16a29d554587d8ccd9de3b9a9 I got this error message: ! [remote rejected] HEAD -> refs/for/master (You must be a committer to push on behalf of others.) It makes sense that not everyone can push to other peoples Gerrit changes. But in this case it would be better to use the old change. Can this be solved somehow? Or should I just create a new Gerrit change for this?
> IMO the Binding class in the mentioned patch should not get type parameters in the first patch and maybe not at all. I agree! Binding still have type parameters in the change I am about to push now, but I'll remove them for an updated change later.
>Or should I just create a new Gerrit change for this? I think you need to push a new Gerrit
(In reply to Jens Lideström from comment #11) > I agree! Binding still have type parameters in the change I am about to push > now, but I'll remove them for an updated change later. You should remove the type parameter in the same Gerrit patch, because once they appear on the master it's hard to remove them again as this could be considered as a breaking change.
I have pushed new patch sets to the old Gerrit change: https://git.eclipse.org/r/50300 The problem I had earlier was that the author of the commit was still set to Stefan Xenos. That was not allowed. When I changed that I could push to the same change. I have removed the type parameters on Binding. It is my impression that this changes is finished. I will go on and work on the addition of type parameters to the org.eclipse.jface.databinding bundle, then the tests and examples.
I got a test failure for the converters that now apply type parameters to their super classes. The problem is that they now fail with a ClassCastException in their generated bridge method, instead of failing with an IllegalArgumentException in code that manually checks the argument type. This breaks backwards (runtime) compatibility, although only in a minor way. I plan to resolve this by using Object as the type argument for those converters. I think it is better to have a non-precise type for them than to break backwards compatibility. I'll add a comment that they have the wrong type for backwards compatibility reasons. Is this the best solution? Or should we accept that ClassCastException is thrown instead of IllegalArgumentException? This is an issue for the following classes: StringToByteConverter StringToNumberConverter StringToShortConverter (Oh, and I should have run the tests locally before committing. But in this case it is good that the test result in on the public build server were everyone can see them.)
(In reply to Jens Lideström from comment #15) > I got a test failure for the converters that now apply type parameters to > their super classes. The problem is that they now fail with a > ClassCastException in their generated bridge method, instead of failing with > an IllegalArgumentException in code that manually checks the argument type. > > This breaks backwards (runtime) compatibility, although only in a minor way. > > I plan to resolve this by using Object as the type argument for those > converters. I think it is better to have a non-precise type for them than to > break backwards compatibility. I'll add a comment that they have the wrong > type for backwards compatibility reasons. > > Is this the best solution? Or should we accept that ClassCastException is > thrown instead of IllegalArgumentException? > > This is an issue for the following classes: > StringToByteConverter > StringToNumberConverter > StringToShortConverter > > (Oh, and I should have run the tests locally before committing. But in this > case it is good that the test result in on the public build server were > everyone can see them.) All the Converter implementations are located in an internal package, so no other project than that described in the MANIFEST.MF as friend is allowed to use these classes. You can delete the test methods, that test wrong types, as this is not possible anymore with typed classes.
(In reply to Conrad Groth from comment #16) > All the Converter implementations are located in an internal package Two converters, StringToNumberConverter and NumberToStringConverter are public. And about the internal converters: It is actually possible for clients to be affected of the *result* of the implementation of the converters, even if they don't have access to the type. This is the case if clients extend for example UpdateValueStrategy and override the convert method. If anyone catches IllegalArgumentException in that method their code will stop working. This is probably not something that is very likely, but it might happen. > that test wrong types, as this is not possible anymore with typed classes. It is *possible* to call the generated bridge method of a converter with an incorrect type. This is the case when an unchecked cast is used, which is necessary for example to get a value from UpdateStrategy#converterMap. Conclusion: I still think it is best to keep the converters as they are. In the latest patch set, nr 13, I have removed a specific type parameters from a few more places. If you really think we should update them anyway I can make a new patch set for that. But I probably will not have time to do it until next weekend, or the one after that.
Ok, you convinced me. The overall state of the patch looks good, some minor comments can be found in Gerrit.
There is a related heap-poisoning bug 26507. A problem there was an omitted application of converters. Fix of that was rejected as backward incompatible. If converters are done in a type-safe way, such fix would have to be applied.
(In reply to Vasili Gulevich from comment #19) > There is a related heap-poisoning bug 26507. Sorry, proper reference is bug 326507
(In reply to Vasili Gulevich from comment #19) > There is a related heap-poisoning bug 326507 (updated). > > A problem there was an omitted application of converters. Fix of that was > rejected as backward incompatible. If converters are done in a type-safe > way, such fix would have to be applied. We will not fix this bug as part of this work. Converters will continue to work the same way as they always have now that we have added type parameters. The only difference will be that converters that apply specific type arguments when implementing IConverter will have ClassCastException thrown by the bridge method of IConverter#convert, instead of having to handle incorrect types manually.
Re: Comment 17 IllegalArgumentException is rarely used as part of a method contract - it's normally just there to indicate programmer errors. I don't have the code in front of me ATM, but does the JavaDoc promise that the implementation will throw an IAE? If not, nobody should be relying on catching the exception and if they are they have a bug. Also, feel free to take the diff from my patch and re-upload it with a new change-id to change the author if you want. I don't mind if you steal the author credit for it - it's just good to see this fixed.
(In reply to Stefan Xenos from comment #22) > but does the JavaDoc promise that the implementation will > throw an IAE? If not, nobody should be relying on catching the exception and > if they are they have a bug. No, the docs don't say this. But it didn't take a big effort to preserve the old behaviour anyway. Maybe we can save someone's old weird code from breaking. > Also, feel free to take the diff from my patch and re-upload it with a new > change-id to change the author if you want. I don't mind if you steal the > author credit for it - it's just good to see this fixed. I went ahead and did this when I discovered it was possible. Good to know that you support it. :)
Change looks good to me. Database tests are still running. Also, I imported the EMF databinding plug-ins and they do not show any errors and the following test plug-ins are still running fine. org.eclipse.emf.test.databinding org.eclipse.emf.test.databinding.edit I plan to merge the change, as soon as the change is rebased onto the current origin/master.
Thanks, Jens for the intensive piece of work. Thanks to Stefan who started the journey and to Conrad for the driving the review.
Jens, could you add an entry to the N&N for 4.9?
Gerrit change https://git.eclipse.org/r/50300 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2220dc6057030ca7d761b4ed6b7debb50eec3452
(In reply to Lars Vogel from comment #26) > Jens, could you add an entry to the N&N for 4.9? I'd be happy to, but I don't know how to do that. How and where do I do that? (I don't find anything on the wiki about this. If you tell me how I'll add a note about that here: https://wiki.eclipse.org/Platform_UI/How_to_Contribute) (I found this N&N ticket for 4.8, but nothing for 4.9: https://bugs.eclipse.org/bugs/show_bug.cgi?id=534241)
I'll add a sentence to N&N about the ListBinding converter bug too: https://bugs.eclipse.org/bugs/show_bug.cgi?id=326507
This introduced compile warnings in nightly builds http://download.eclipse.org/eclipse/downloads/drops4/I20180702-2000/compilelogs/plugins/org.eclipse.e4.tools_4.8.100.v20180605-1328/@dot.html . Probably e4.tools needs modifications or some generification is not done entirely. Please someone involved in the bug to spend some time to fix it.
(In reply to Alexander Kurtakov from comment #30) > This introduced compile warnings in nightly builds I'll have a look at it. @Alexander Kurtakov @Lars Vogel Where do we fix these warnings? In the Gerrit change attached to this bug? Or in a new Gerrit change?
(In reply to Jens Lideström from comment #31) > (In reply to Alexander Kurtakov from comment #30) > > This introduced compile warnings in nightly builds > > I'll have a look at it. Thanks! > > @Alexander Kurtakov > @Lars Vogel > > Where do we fix these warnings? http://git.eclipse.org/c/platform/eclipse.platform.ui.tools.git/tree/bundles/org.eclipse.e4.tools > In the Gerrit change attached to this bug? > Or in a new Gerrit change? As it's in another repo it has to be in a new gerrit change attached to this bug.
(In reply to Jens Lideström from comment #28) > How and where do I do that? I added a small section to the contributor guide https://wiki.eclipse.org/Platform_UI/How_to_Contribute#Adding_a_Note_and_Noteworthy_entry Let me know if that help. Please add me as reviewer for the gerrits once you created them.
Now when I have looked at the warnings in org.eclipse.e4.tools I become unsure whether it is a good idea to merge the updates to org.eclipse.core.databinding before the plugins that are clients are updated: org.eclipse.jface.databinding and org.eclipse.core.databinding.beans I should have looked and some client code before staring this work...
New Gerrit change created: https://git.eclipse.org/r/125407
(In reply to Jens Lideström from comment #34) > Now when I have looked at the warnings in org.eclipse.e4.tools I become > unsure whether it is a good idea to merge the updates to > org.eclipse.core.databinding before the plugins that are clients are > updated: org.eclipse.jface.databinding and org.eclipse.core.databinding.beans Updating the whole databinding framework at once was found as too time-consuming, therefore, the PMC allows to update plug-in by plug-in.
I get an error when I try to push to to the News repo. I have attached an platform_isv.html file with an added note about org.eclipse.core.databinding to this ticket instead. This is the error message I get: $ git push -v ssh://jlidestrm@git.eclipse.org/gitroot/www.eclipse.org/eclipse/news HEAD:refs/for/master Pushing to ssh://jlidestrm@git.eclipse.org/gitroot/www.eclipse.org/eclipse/news ssh: connect to host git.eclipse.org port 22: Connection timed out fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists.
Created attachment 274778 [details] platform_isv.html file with an added note about org.eclipse.core.databinding
(In reply to Jens Lideström from comment #37) > I get an error when I try to push to to the News repo. > > I have attached an platform_isv.html file with an added note about > org.eclipse.core.databinding to this ticket instead. > > This is the error message I get: > > $ git push -v > ssh://jlidestrm@git.eclipse.org/gitroot/www.eclipse.org/eclipse/news > HEAD:refs/for/master > Pushing to > ssh://jlidestrm@git.eclipse.org/gitroot/www.eclipse.org/eclipse/news > ssh: connect to host git.eclipse.org port 22: Connection timed out > fatal: Could not read from remote repository. > > Please make sure you have the correct access rights > and the repository exists. push URL looks wrong, if you clone the repo via Eclipse it will be correct. You should use: ssh://jlidestrm@git.eclipse.org:29418/www.eclipse.org/eclipse/news
New Gerrit change created: https://git.eclipse.org/r/125414
Lars Vogel wrote: > You should use: ssh://jlidestrm@git.eclipse.org:29418/www.eclipse.org/eclipse/news That did it, thank you!
Gerrit change https://git.eclipse.org/r/125414 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=0ea351436469a8499f8493f6766c1fc59d63a8b3
Gerrit change https://git.eclipse.org/r/125407 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.tools.git/commit/?id=85611b010ded9f211293a5f57b9e65e677c0282d
Thanks Jens, would be great if you could work on the generification of the remaining plug-in settings. Please open new bugs if you plan to work on them.
New Gerrit change created: https://git.eclipse.org/r/125708
Gerrit change https://git.eclipse.org/r/125708 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=241d6bf6d69eb1e3e3da141f37c95f27499ea2e4
New Gerrit change created: https://git.eclipse.org/r/125742
Gerrit change https://git.eclipse.org/r/125742 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=1e3c917331736d73f984a8391e72fb9dce98d54d
