Community
Participate
Working Groups
See discussion in bug 522286 comment #7. It should be possible to drag and drop entries between modulepath and classpath nodes in Java Build Path dialog. The corresponding module properties of the entries being d&d should be handled accordingly.
This needs to be fixed for M3 and 4.7.2.
Ping!
New Gerrit change created: https://git.eclipse.org/r/111803
I've implemented drag and drop on Library page as well as Project page. The corresponding module properties is changed accordingly.
Gerrit change https://git.eclipse.org/r/111803 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=5d3d25f1d3af15f7c02b735fb616c929bc15c5b4
Putting in master for faster testing. Keeping bug open for any polish items. We need to evaluate if this is to be back-ported in 472RC3. Noopur, can you please have a look?
(In reply to Vikas Chandra from comment #6) > Putting in master for faster testing. Keeping bug open for any polish items. > > We need to evaluate if this is to be back-ported in 472RC3. > > Noopur, can you please have a look? I am in a training this week. I will try to have a look for RC3.
Looks good in general while testing. Found some issues: - DnD JRE from modulepath to classpath. Apply and Close. Reopen the dialog. => JRE is still on modulepath even though .classpath doesn't have the module attribute set to true. The correct fix would be to disallow DnD of JRE to classpath so that above issues are avoided. - DnD a JUnit container from classpath to modulpath. Apply and Close. Reopen the dialog. => JUnit container is still seen in classpath. The .classpath file is also not updated.
Kalyan, can you please do some testing after Vikas provides the fix? I will review the code tomorrow night.
(In reply to Noopur Gupta from comment #9) > Kalyan, can you please do some testing after Vikas provides the fix? I will > review the code tomorrow night. Sure
New Gerrit change created: https://git.eclipse.org/r/111944
Gerrit change https://git.eclipse.org/r/111944 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=fc37380d9a2968fbdc1ad29f66909d36dc94d8c3
Thanks for the feedback Noopur ! >>DnD JRE from modulepath to classpath >> disallow DnD of JRE to classpath Makes sense ! Fixed by comment#12. >>DnD a JUnit container from classpath to modulpath I am looking into this case.
New Gerrit change created: https://git.eclipse.org/r/111946
Gerrit change https://git.eclipse.org/r/111946 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=7e24e982336b10d9bb8dc44e179a5bdbeb1d0816
>>I am looking into this case. Fixed by comment#15
I will update with the corresponding 4.7.2 gerrit change soon.
New Gerrit change created: https://git.eclipse.org/r/111955
(In reply to Kalyan Prasad Tatavarthi from comment #10) > (In reply to Noopur Gupta from comment #9) > > Kalyan, can you please do some testing after Vikas provides the fix? I will > > review the code tomorrow night. > > Sure I have done some testing with vikas's new fix and everything looks good during testing.
Few observations while testing with the latest code in master: 1) Can be moved to a different bug - DnD JUnit container from modulepath to classpath, expand it and you can see "Is not modular" node. Apply and Close. Reopen the dialog and expand JUnit container in classpath. The "Is not modular" node cannot be seen now. 2) In a plug-in project with JRE 9, add some plug-in dependencies so that it has a "Plug-in Dependencies" container under classpath node in Java Build Path dialog. It is not possible to DnD "Plug-in Dependencies" container. Not sure if it makes sense for plug-in dependencies but the same should be checked for other containers like Maven dependencies in a maven project etc. 3) Minor issue which can be handled post 4.7.2 - DnD of an entry on to its parent node should not be allowed and it should show the "not allowed" icon. Vikas, can you please comment on the above points after opening new bugs where required?
(In reply to Noopur Gupta from comment #20) > Few observations while testing with the latest code in master: 4) Press Ctrl while DnD. We get the copy icon (with an extra '+') while moving instead of the move icon. This gives a wrong impression of copying an object with DnD.
(In reply to Noopur Gupta from comment #21) Please see the review comments on Gerrit. In particular, the changes w.r.t. DND.DROP_COPY and DND.DROP_DEFAULT, which are related to point number 4 above. In general, it looks good. +1 for RC3.
(In reply to Noopur Gupta from comment #21) > (In reply to Noopur Gupta from comment #20) > > Few observations while testing with the latest code in master: > > 4) Press Ctrl while DnD. We get the copy icon (with an extra '+') while > moving instead of the move icon. This gives a wrong impression of copying an > object with DnD. Vikas, at least 4) we need to address with 4.7.2, but I think this is already covered by the review comments in the Gerrit change.
New Gerrit change created: https://git.eclipse.org/r/112033
Gerrit change https://git.eclipse.org/r/112033 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=ee8c997a9b3bf6c34588d2e6ba52b1f86f00674e
Review comments incorporated in master stream via comment#25
New Gerrit change created: https://git.eclipse.org/r/112034
Patch candidate for 472RC3 is https://git.eclipse.org/r/112034 The review comments has been incorporated and it addresses 4). I will look at 1), 2) and 3) and open bugs if required.
Gerrit change https://git.eclipse.org/r/112034 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=898ac9642c1f289d2ef17477f83898796dadd461
>> The "Is not modular" node cannot be seen now. Is there a loss of functionality? I would think that both scenarios would be equivalent. However, I opened (Bug 527609 - added you there) and we can discuss there. >>same should be checked for other containers like Maven dependencies in a maven project etc. If you can give example of maven project with Maven dependencies, I can investigate this scenario. You can create a bug and add me on the bug. >>DnD of an entry on to its parent node should not be allowed Opened (Bug 527611) for this but I think this is really a minor issue.
verified comment#0, comment#8 and comment#21 on Version: Oxygen.2 (4.7.2) Build id: M20171122-0400
(In reply to Vikas Chandra from comment #26) > Review comments incorporated in master stream via comment#25 This does not handle the following review comments: - DND.DROP_COPY: We don't support copy with DnD so this constant should not be used anywhere in the code. Please check. Here, you have just removed DROP_COPY from the location quoted in the review comment. But it is still used at other places e.g. in #addDragSupport. - DND.DROP_DEFAULT: This should also not be required. This is still present. Is there any reason why it is required? Also, when we don't support and remove DND.DROP_COPY from all the locations then we don't need to override the methods #dragEnter, #dragOperationChanged, and #dragOver. And the method #determineLocation doesn't add any value in our case in addition to its default implementation. So even this can be removed.
(In reply to Vikas Chandra from comment #30) > >> The "Is not modular" node cannot be seen now. > > Is there a loss of functionality? I would think that both scenarios would > be > equivalent. However, I opened (Bug 527609 - added you there) and we can > discuss > there. Yes, without this attribute, you can't open the "Module properties" dialog and hence you can't check the checkbox to It should be consistent for all the entries shown under classpath node. > >>same should be checked for other containers like Maven dependencies in a maven project etc. > > If you can give example of maven project with Maven dependencies, I can > investigate this scenario. You can create a bug and add me on the bug. > > >>DnD of an entry on to its parent node should not be allowed > > Opened (Bug 527611) for this but I think this is really a minor issue.
We have already done lot of testing on this code. At this point, I just got the UI icon removed. I won't recommend removing redundant code at this point of time because it is already too late in RC3 ( early 473 is a good time). >>Without this attribute, you can't open the "Module properties" dialog When you add Junit5 in Classpath, you don't see it anyway even without drag and drop support. Anyways, Bug 527609 will track this.
Created attachment 271588 [details] Sample project where container DnD doesn't work Please ignore my previous comment. (In reply to Vikas Chandra from comment #30) > >> The "Is not modular" node cannot be seen now. > > Is there a loss of functionality? I would think that both scenarios would > be > equivalent. However, I opened (Bug 527609 - added you there) and we can > discuss > there. Yes, without this attribute, you can't open the "Module properties" dialog and hence you can't check the checkbox to move it to the modulepath node. So, moving it to the modulepath node via keyboard is not possible. Also, it should be consistent for all the entries shown under classpath node. > >>same should be checked for other containers like Maven dependencies in a maven project etc. > > If you can give example of maven project with Maven dependencies, I can > investigate this scenario. Attached project reproduces the issue, where DnD on the container is not possible. Also, did you check this for Plug-in dependencies container? > >>DnD of an entry on to its parent node should not be allowed > > Opened (Bug 527611) for this but I think this is really a minor issue.
(In reply to Vikas Chandra from comment #34) > We have already done lot of testing on this code. At this point, I just got > the UI icon removed. I won't recommend removing redundant code at this point > of time because it is already too late in RC3 ( early 473 is a good time). It can be done for master now so that we can get more testing and then it can be backported to 4.7.3 with the remaining issues.
(In reply to Noopur Gupta from comment #35) > > >>same should be checked for other containers like Maven dependencies in a maven project etc. > > > > If you can give example of maven project with Maven dependencies, I can > > investigate this scenario. > > Attached project reproduces the issue, where DnD on the container is not > possible. Also, did you check this for Plug-in dependencies container? Opened bug 527621.
Verified on Version: Photon (4.8) Build id: I20171122-2000 "See also" bugs tracks the rest of the issues. The cleanup will be done along with them.
