Community
Participate
Working Groups
The existing Module Dialog of the Build Path page has its limits as witnessed by bug 526831 and friends. I had sketched a new, comprehensive UI in https://wiki.eclipse.org/Java9/ModularityOptions/UIProposal_SH
Today I made a shy start at implementing this, hence tentatively scheduling for 4.12.
New Gerrit change created: https://git.eclipse.org/r/140882
(In reply to Eclipse Genie from comment #2) > New Gerrit change created: https://git.eclipse.org/r/140882 A few preparations in JDT/Core, see gerrit for a few details.
New Gerrit change created: https://git.eclipse.org/r/140893
(In reply to Eclipse Genie from comment #4) > New Gerrit change created: https://git.eclipse.org/r/140893 This change implements a new Tab, that mostly features the same functionality that is already available via the "Is Modular" node, but based on our discussion the new Tab can be smoothly extended for more features and more flexibility. The design closely follows what was previously captured in https://wiki.eclipse.org/Java9/ModularityOptions/UIProposal_SH My current goal is to bring a feature equivalent version into M2 (SimRel) to give users a chance for testing. A few more details should still be added until M3. The change already contains one addition: ability to specify add-opens (via radio buttons in the Expose Package dialog). I suggest, we actually keep both UIs redundantly for a grace period. Perhaps the old dialog should show a deprecation warning, perhaps with a link to the new Tab. I also added new icon decorations A, E, O, R, S, to denote automatic, exports, opens, reads and system respectively. I don't consider myself an icon expert, so these should just be seen as strawmen. I'd be very happy, if s.o. would contribute more profesional versions :) (no fancy art work required, just letters - btw. I chose the same blue as used by the J in several locations). Class ModuleDependenciesPage has a comment listing some TODOs, of which the biggest is the topic of patch-module, where I have a few questions in the TODO, but I see no fundamental problems. Patching defined by the old UI can already be seen also in the new UI. Let me call out that we only have one "Remove" button, which can, however, remove different things: - if only one or more modules in the LHS list are selected, those module(s) will be removed from the module graph (limit-modules) - if any configured details are selected in the RHS tree, than those are deleted, not the module. - if unmodifiable elements are selected in the RHS tree, then Remove is disabled - to avoid bad surprises the removing modules is guarded by a confirmation dialog. Does anyone have issues with this? That confirmation dialog already shows a new approach to what the old UI captured in those three lists: "available", "explicitly included", "implicitly included": - when removing a module that is required by other modules still in the list, removal will actually cover the transitive closure of modules affected by the initial removal. The confirmation dialog will show a list comprising this closure. - when adding a system module the selection dialog automatically extends the selection to the transitive closure in the opposite direction: all modules required by the selected module(s). The asymmetry is owed to the fact, that for removing we use the normal selection in the LHS list, where auto-selection would conflict with normal use of this list as the master for the RHS tree. While the implementation isn't yet bullet-proof, it already covers plenty of used cases. I'd already appreciate if people try it (you need both gerrits, jdt.core & jdt.ui) and give feedback.
(In reply to Stephan Herrmann from comment #5) > (you need both gerrits, jdt.core & jdt.ui) @Jay, @Manoj, the core gerrit contains stuff that is only indirectly related to this new feature. LMK if you prefer individual bugs, in particular for the search issue.
(In reply to Stephan Herrmann from comment #5) > (In reply to Eclipse Genie from comment #4) > > New Gerrit change created: https://git.eclipse.org/r/140893 > > My current goal is to bring a feature equivalent version into M2 (SimRel) to > give users a chance for testing. A few more details should still be added > until M3. > > I suggest, we actually keep both UIs redundantly for a grace period. Perhaps > the old dialog should show a deprecation warning, perhaps with a link to the > new Tab. Sounds good to me. The old UI can be removed in M3 if the new one is stable by that time. > Let me call out that we only have one "Remove" button, which can, however, > remove different things: > Does anyone have issues with this? No issues from my end.
- Few comments on UI Gerrit patch. - Some icons should be added for "Declared" and "Configured" nodes also. - Found this in the log while trying the patch. Not sure which button I pressed: java.lang.IllegalArgumentException: Non-existent button index 5 at org.eclipse.jdt.internal.ui.wizards.buildpaths.ModuleDependenciesAdapter.customButtonPressed(ModuleDependenciesAdapter.java:540) ...
(In reply to Stephan Herrmann from comment #5) > Let me call out that we only have one "Remove" button, which can, however, > remove different things: > - if only one or more modules in the LHS list are selected, those module(s) > will be removed from the module graph (limit-modules) > - if any configured details are selected in the RHS tree, than those are > deleted, not the module. > - if unmodifiable elements are selected in the RHS tree, then Remove is > disabled > - to avoid bad surprises the removing modules is guarded by a confirmation > dialog. > Does anyone have issues with this? One question: Do we ask the users the customary "Do you want to ...?" and if so, does the question specify what is going to be removed?
(In reply to Stephan Herrmann from comment #6) > the search issue. @Stephan: Given there is an API addition for IMD, I think it would be good to have a separate bug.
(In reply to Noopur Gupta from comment #8) > - Few comments on UI Gerrit patch. Thanks, will attend to them shortly. > - Some icons should be added for "Declared" and "Configured" nodes also. Any ideas? The "Configured" node could perhaps re-use the module+gear icon (module_attrib.png). Thinking more about it, maybe "Declared" could actually be merged with the main module node. See https://wiki.eclipse.org/Java9/ModularityOptions/UIProposal_SH where under Details the node java.corba would adopt all the details from "Declared" as its direct children (viz: requires,exports,opens from its module-info). WDYT? Next, what do people prefer for representing patch-module? (a) Listing additional source folders as children of "Configured", source folders decorated with P (this is how the current patch visualizes existing patch-modules), or (b) Adding another toplevel node "Patched" (using a module+patch icon :) ). > - Found this in the log while trying the patch. Not sure which button I > pressed: > java.lang.IllegalArgumentException: Non-existent button index 5 > at > org.eclipse.jdt.internal.ui.wizards.buildpaths.ModuleDependenciesAdapter. > customButtonPressed(ModuleDependenciesAdapter.java:540) > ... 5 is IDX_PATCH, the implementation for which is still in the works (i.e., the current patch can only visualize, but not create/edit this). I hope to have a patch ready later today (patch-module being the most tricky of these attributes).
(In reply to Jay Arthanareeswaran from comment #9) > (In reply to Stephan Herrmann from comment #5) > > Let me call out that we only have one "Remove" button, which can, however, > > remove different things: > > - if only one or more modules in the LHS list are selected, those module(s) > > will be removed from the module graph (limit-modules) > > - if any configured details are selected in the RHS tree, than those are > > deleted, not the module. > > - if unmodifiable elements are selected in the RHS tree, then Remove is > > disabled > > - to avoid bad surprises the removing modules is guarded by a confirmation > > dialog. > > Does anyone have issues with this? > > One question: Do we ask the users the customary "Do you want to ...?" and > if so, does the question specify what is going to be removed? Currently, we only ask when removing modules from the module graph (which may include removing more modules than selected). I think, when you select any details (exports/opens/reads nodes), it should be clear that this is exactly what is being removed :) Given we support multi - selection on the RHS, a confirmation dialog could look more confusing than the original selection. LMK if you think differently. BTW, in the existing approach, children of "Is Modular" can be similarly removed without a confirmation.
(In reply to Manoj Palat from comment #10) > (In reply to Stephan Herrmann from comment #6) > > the search issue. > > @Stephan: Given there is an API addition for IMD, I think it would be good > to have a separate bug. Created *two* separate child bugs: bug 546649 and bug 546653. While separate gerrits are still pending, I already assigned reviewers :) Let me specifically call out, that the PATCH_MODULE part in bug 546653 proposes a modified / enhanced interpretation of an existing classpath attribute. I hope people agree that this enhancement can be seen as a compatible change, not requiring a new classpath attribute (e.g., PATCH_MODULE2). With this, the existing gerrit will be reduced to just the change in ModularClassFileMatchLocator (https://git.eclipse.org/r/#/c/140882/1/org.eclipse.jdt.core/search/org/eclipse/jdt/internal/core/search/matching/ModularClassFileMatchLocator.java)
(In reply to Eclipse Genie from comment #4) > New Gerrit change created: https://git.eclipse.org/r/140893 Patch set #4 is much closer already. Down to one must-have TODO (qualified exports & opens). (In reply to Stephan Herrmann from comment #11) > > - Some icons should be added for "Declared" and "Configured" nodes also. > > Any ideas? > The "Configured" node could perhaps re-use the module+gear icon > (module_attrib.png). > Thinking more about it, maybe "Declared" could actually be merged with the > main module node. See > https://wiki.eclipse.org/Java9/ModularityOptions/UIProposal_SH where under > Details the node java.corba would adopt all the details from "Declared" as > its direct children (viz: requires,exports,opens from its module-info). WDYT? I implemented this approach, to the effect that we have only two toplevel nodes in the RHS pane: - (module) Declared details of {0} - (module+gear) Configured > Next, what do people prefer for representing patch-module? > > (a) Listing additional source folders as children of "Configured", source > folders decorated with P (this is how the current patch visualizes existing > patch-modules), or > > (b) Adding another toplevel node "Patched" (using a module+patch icon :) ). I implemented a variant of (a), but used a 'real' patch decoration (actual svg extracted from /org.eclipse.images/eclipse-svg/org.eclipse.equinox.p2.ui/icons/obj/iu_patch_obj.svg). > 5 is IDX_PATCH, the implementation for which is still in the works (i.e., > the current patch can only visualize, but not create/edit this). I hope to > have a patch ready later today (patch-module being the most tricky of these > attributes). Implemented. For testing please first pull https://git.eclipse.org/r/#/c/140973/ (which is the top of now three JDT/Core gerrits).
Looks good. Few more comments: - The dialog for "Patch with..." needs a title. - It will be good if can get some help in icon designing. If not, the current icons are fine with little changes. I think the letters 'S', 'R' etc. on the module icon can be moved closer to the 'M' icon. Currently, they stick closer to the element name instead of the module icon. For example, see entries in Details pane. - Not sure how the patch icon looks to everyone. Probably, a 'P' will be more consistent. - Need to check about the consistency of the configuration between "Is modular" and the new tab so that existing user configurations are not impacted when "Is modular" is removed. - For projects below Java 9, "Add System Module..." button can be disabled. Does it need an explicit message stating that this tab is not applicable or just disabling all content is fine? - When JRE is changed from non-modular to modular and vice-versa on the Libraries tab, the new tab should update accordingly. @All: Please test and provide your feedback.
(In reply to Noopur Gupta from comment #15) > Looks good. Few more comments: Thanks a lot. I will look into details shortly. > - It will be good if can get some help in icon designing. If not, the > current icons are fine with little changes. I think the letters 'S', 'R' > etc. on the module icon can be moved closer to the 'M' icon. Currently, they > stick closer to the element name instead of the module icon. For example, > see entries in Details pane. I believe this is (in part) an effect of ModuleDependenciesPage.DecoratedImageDescriptor.getSize() returning JavaElementImageProvider.BIG_SIZE. The overlay themselves are based on existing overlays. Do you suggest to shift the content withing the overlay (there's a 1 pixel transparent border at the left)? > - Not sure how the patch icon looks to everyone. Probably, a 'P' will be > more consistent. I believe a visual distinction from the other elements is good, because patching pulls in more source folders into an existing module (thus affecting their compilation), whereas the other options only modify the API / wiring of a module. If you prefer consistency of child nodes, then I'd revive my proposal to create a new (third) toplevel node ("Patched with") - using module+patch as its icon :) > @All: Please test and provide your feedback. Yes please :)
Gerrit change https://git.eclipse.org/r/140882 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=a5bb33f31600c5673590e85111231334faef5bd6
(In reply to Eclipse Genie from comment #17) > Gerrit change https://git.eclipse.org/r/140882 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=a5bb33f31600c5673590e85111231334faef5bd6 The required changes in JDT/Core are all released now. For testing you only need to pull the JDT/UI gerrit: https://git.eclipse.org/r/140893
(In reply to Noopur Gupta from comment #15) > - The dialog for "Patch with..." needs a title. Fixed locally, will be part of the next patch set. > - Need to check about the consistency of the configuration between "Is > modular" and the new tab so that existing user configurations are not > impacted when "Is modular" is removed. > > - For projects below Java 9, "Add System Module..." button can be disabled. > Does it need an explicit message stating that this tab is not applicable or > just disabling all content is fine? > > - When JRE is changed from non-modular to modular and vice-versa on the > Libraries tab, the new tab should update accordingly. Very good, but tricky points. For now I will focus on properly re-initializing the new page when Apply has been clicked on another tab. In particular, if the current project changes compliance to 8- all controls will be disabled, and the All Modules list will show a single, disabled entry: "Non-modular Project". I've created bug 546790 as a follow-up task for detailed coordination between pages / tabs.
I have finally some time to try this. When the current project already has a module-info.java, it appears in the "All Modules"-List in the "Module Dependencies"-Tab and you can select it. Then there is no configured details node visible on the right, but the "Read Module", "Expose Package" and "Patch with..." buttons are enabled (but don't do anything when clicked). Also, the "Remove" button is enabled for the current module and for java.base, even tough you are not allowed to remove them.
The "Add System Modules..." button only makes sense, if you have previously used "Remove" to remove modules, so these button really belong together. Maybe they should be placed next to each other?
You can currently patch a module with a project that has a module-info.java (including the current project).
Also, you can currently select source folders for patching other modules. This would mean that not all sources within a project belong to the same module. I suspect we would have to change many places in the code that currently assume that all sources in a project belongs to one module. ( I think we already discussed this at EclipseCon Europe. )
It is currently hard to see which modules have non-empty "Configured details"
It would be cool if there was a button that shows the javac command line options corresponding to the combined configured details. It would be even cooler if there was another button that sets the configured details from javac command line options in the clipboard.
(In reply to Till Brychcy from comment #25) > It would be even cooler if there was another button that sets the configured > details from javac command line options in the clipboard. Related: I just thought that m2e will want to set the configured details from command line options configured in the pom.xml, which would be a very similar functionality, so some jdt.core-api that helps with this would probably be a good idea.
Thanks a lot, Till! I'll respond to your comments one by one (and add detail numbering for easy referral). == comment 20 (a) == > When the current project already has a module-info.java, it appears in the > "All Modules"-List in the "Module Dependencies"-Tab and you can select it. > Then there is no configured details node visible on the right, but the "Read > Module", "Expose Package" and "Patch with..." buttons are enabled (but don't > do anything when clicked). I intentionally did not create the "Configured" node for the focus module, but realize now, that this was done based on premature assumptions. I have this fixed locally, i.e., also the current project's module can be configured in this dialog. This change implies that now also source folders can have all of the module-related classpath attributes. In the case of multiple source folders, the one holding module-info.java will be used to naturally represent the module. We'll need to make sure we properly evaluate these attributes. Over to bug 546794 == comment 20 (b) == > Also, the "Remove" button is enabled for the current module and for > java.base, even tough you are not allowed to remove them. Also done by intention, with the following rationale: think specifically of a multi-selection in the module list. User finds "Remove" disabled, but asks: why can't I remove those? If we keep the button enabled we have a chance to tell the reason, like: "Cannot remove module 'java.base'". User may answer: "oh, I didn't mean to remove java.base, thanks". Full list of explanations currently given this way: - "Cannot remove the current module" - "Cannot remove module 'java.base'" - "Cannot remove module 'foo.bar' Only system modules can be removed here. To remove other modules please remove them from the ModulePath (tab Projects or Libraries)." In particular the last scenario calls for some user guidance beyond just disabling a button, IMHO. What do others prefer? - Aggressively disable the button? - Let the user try, and tell when & why not possible? - Disable only on single selection, if illegal to remove? If you vote for disable, please suggest how the user can be guided to other tabs for removal of dependencies.
(In reply to Till Brychcy from comment #21) > The "Add System Modules..." button only makes sense, if you have previously > used "Remove" to remove modules, This is not true if the current projects lacks module-info. In that case only the set of root modules defined in JEP 261 is contained in the module graph by default. Here the user can start adding system modules right away. > so these button really belong together. > Maybe they should be placed next to each other? Please see this section from comment 5: > Let me call out that we only have one "Remove" button > [...] > Does anyone have issues with this? In comment 7 Noopur already replied: > No issues from my end. You seem to be suggesting to split this into two buttons?
(In reply to Till Brychcy from comment #22) > You can currently patch a module with a project that has a module-info.java > (including the current project). Good point. Fixed locally. While patch-module + module-info is illegal per current content of JEP 261, we need to watch the space below https://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-April/014203.html where an update regarding patch-module is indicated. We know that JEP 261 is already outdated, and still I'm not aware of any channel how it could possibly be updated, so I'm curious to see if anything will come of that discussion.
(In reply to Till Brychcy from comment #23) > Also, you can currently select source folders for patching other modules. > This would mean that not all sources within a project belong to the same > module. I suspect we would have to change many places in the code that > currently assume that all sources in a project belongs to one module. ( I > think we already discussed this at EclipseCon Europe. ) I did incorporate your suggestion to allow patch-module per project. Remaining question is, whether this should be the only option. For folders *of the current project* I basically agree to your point: this would conflict with our fundamental design wrt projects & modules. Aside from patching several modules in one project, wouldn't it make a lot of sense to support one project to have one regular and one patching source folder? Here's a sketch of another (crude) situation, which IMHO would be possible: P0 defines sources "src1" and "src2" P1 reads modules MA & MB (via some other dependencies) P1 depends on P0 P1 wants to see P0 such that src1 patches MA src2 patches MB What I'm trying to say: compilation of P0 is not affected by any patch-module, only when compiling P1 we see several modules as patched. This situation may not look plausible right now, but the whole point in developing a more comprehensive UI is to avoid premature assumptions about what users will / will not do. Starting from how JEP 261 defines patch-module, we should support all filesystem locations that could plausibly contribute to an existing module. IMHO, source folders naturally map to this definition. Which solution do people prefer: (a) only support patching with a project (a.1) wait if users request support for referring to individual source folders (b) figure out the conflicting situations, and disallow only those. (b.1) just prevent any "real damage" now, and file follow-up tasks for exploring how far we can go.
(In reply to Stephan Herrmann from comment #27) > == comment 20 (a) == > > I intentionally did not create the "Configured" node for the focus module, > but realize now, that this was done based on premature assumptions. I have > this fixed locally, i.e., also the current project's module can be > configured in this dialog. > > This change implies that now also source folders can have all of the > module-related classpath attributes. In the case of multiple source folders, > the one holding module-info.java will be used to naturally represent the > module. We'll need to make sure we properly evaluate these attributes. Over > to bug 546794 I didn't really understand yet why this change implies this, but one issue with anything related to source folders is that multiple source folders may use the same output folder. I guess conceptually we always have to enforce that the same settings apply to all source folders with the same output folder. > > == comment 20 (b) == > > > Also, the "Remove" button is enabled for the current module and for > > java.base, even tough you are not allowed to remove them. > > Also done by intention, with the following rationale: think specifically of > a multi-selection in the module list. User finds "Remove" disabled, but > asks: why can't I remove those? If we keep the button enabled we have a > chance to tell the reason, like: "Cannot remove module 'java.base'". User > may answer: "oh, I didn't mean to remove java.base, thanks". Full list of > explanations currently given this way: > - "Cannot remove the current module" > - "Cannot remove module 'java.base'" > - "Cannot remove module 'foo.bar' > Only system modules can be removed here. > To remove other modules please remove them from the ModulePath (tab > Projects or Libraries)." > In particular the last scenario calls for some user guidance beyond just > disabling a button, IMHO. > > What do others prefer? > - Aggressively disable the button? > - Let the user try, and tell when & why not possible? > - Disable only on single selection, if illegal to remove? > My preferred: Enable it only if at least one removable item is selected (and when remove is clicked, and some non-removable ones are selected, remove the removable ones but give a message as written above for the others) > If you vote for disable, please suggest how the user can be guided to other > tabs for removal of dependencies. Explain it in the help?
(In reply to Stephan Herrmann from comment #28) > (In reply to Till Brychcy from comment #21) > > The "Add System Modules..." button only makes sense, if you have previously > > used "Remove" to remove modules, > > This is not true if the current projects lacks module-info. In that case > only the set of root modules defined in JEP 261 is contained in the module > graph by default. Here the user can start adding system modules right away. Hmm currently I see the same list whether the current project has a module-info or not. > > > so these button really belong together. > > Maybe they should be placed next to each other? > > Please see this section from comment 5: > > Let me call out that we only have one "Remove" button > > [...] > > Does anyone have issues with this? > > In comment 7 Noopur already replied: > > No issues from my end. > > You seem to be suggesting to split this into two buttons? Only later I understood that "remove" is used both for modules and for configured details. I don't have a better suggestion yet. Maybe moving the "Add System Modules.." button to the other buttons on the right would help? But I'm not convinced myself. Maybe the button title could dynamically change if removable items are selected to either "Remove system module" or "Remove configured detail" (or the corresponding plural forms). In the other comment you asked how to guide users how to remove non-system modules. Now I actually wondered if the question shouldn't be: How to guide users how to *add* non-system modules here (Removing them the same way would then be natural) Regarding buttons: I think all buttons that open a dialog before performing an action (so almost all) should end with "...", e.g. "Read Module..." instead of "Read Module"
(In reply to Till Brychcy from comment #24) > It is currently hard to see which modules have non-empty "Configured details" I had this on my "later" list, but implemented this just now: The list will show a "> " prefix for 'modified' modules, borrowing this idiom from team/scm support :) ---------------- I've pushed my current state to gerrit (patch set #5), to make the following fixes available for testing: - comment 15 (The dialog for "Patch with..." needs a title (& a message)) - comment 15 (disable at compliance 8-, enable at 9+, see comment 19) - comment 20 (a) (configurabillity for the current project's module, see comment 27) - comment 22 (don't offer locations with module-info for patching, see comment 29) - comment 24 (detail indicator "> ", see this comment) Comment 25 and comment 26 will probably go into the nice-to-have follow-up bucket.
(In reply to Till Brychcy from comment #31) > (In reply to Stephan Herrmann from comment #27) > > == comment 20 (a) == > > > > I intentionally did not create the "Configured" node for the focus module, > > but realize now, that this was done based on premature assumptions. I have > > this fixed locally, i.e., also the current project's module can be > > configured in this dialog. > > > > This change implies that now also source folders can have all of the > > module-related classpath attributes. In the case of multiple source folders, > > the one holding module-info.java will be used to naturally represent the > > module. We'll need to make sure we properly evaluate these attributes. Over > > to bug 546794 > > I didn't really understand yet why this change implies this, but one issue > with anything related to source folders is that multiple source folders may > use the same output folder. I guess conceptually we always have to enforce > that the same settings apply to all source folders with the same output > folder. A project doesn't have classpath attributes, only classpath entries have. For the issue of modifying the current project's module, we already have the guarantee that at most one source folder has module-info.java. Only that source folder is offered for modification via the new UI. > > If you vote for disable, please suggest how the user can be guided to other > > tabs for removal of dependencies. > > Explain it in the help? Who reads help? :) In fact there are several issues calling for a readily available help page, notably the icon decorations. Actually, I was wondering if we could afford some space to include a caption/explanation for those letters.
(In reply to Till Brychcy from comment #32) > (In reply to Stephan Herrmann from comment #28) > > (In reply to Till Brychcy from comment #21) > > > The "Add System Modules..." button only makes sense, if you have previously > > > used "Remove" to remove modules, > > > > This is not true if the current projects lacks module-info. In that case > > only the set of root modules defined in JEP 261 is contained in the module > > graph by default. Here the user can start adding system modules right away. > > Hmm currently I see the same list whether the current project has a > module-info or not. Please check for these modules: [jdk.policytool, jdk.charsets, jdk.pack, jdk.deploy, java.jnlp, jdk.snmp, jdk.deploy.controlpanel, jdk.zipfs, jdk.crypto.ec, jdk.editpad, java.smartcardio, java.activation, jdk.internal.vm.ci, jdk.naming.rmi, jdk.internal.vm.compiler, jdk.javaws, jdk.rmic, jdk.jstatd, java.se.ee, jdk.jcmd, jdk.xml.ws, jdk.scripting.nashorn.shell, java.xml.bind, jdk.plugin, jdk.hotspot.agent, javafx.deploy, jdk.plugin.server, jdk.crypto.cryptoki, java.transaction, java.xml.ws, java.xml.ws.annotation, jdk.xml.bind, jdk.aot, java.corba, jdk.naming.dns, jdk.localedata] Those are in the list if you have module-info, but not when compiling for the unnamed module, no?
(In reply to Stephan Herrmann from comment #30) > Starting from how JEP 261 defines patch-module, we should support all > filesystem locations that could plausibly contribute to an existing module. > IMHO, source folders naturally map to this definition. Hmm in JEP 261 patch-module is used both for specifying that source to be compiled (=source folders) should be part of another module and that other jars or .class-folders should be treated like that (=output folders of projects). I haven't tried, but I wonder: can you actually compile both patching sources and non-patching sources in a differed folders in a single javac invocation? I guess not in single-module mode, but probably in multi-module mode? But as written above in another comment: I think at least we'd have to enforce that source folders have the same settings if they are compiled to the same output folder. Or we directly think in terms of the output folders instead of the source folders (I mean the UI here) Of course, only for the default output folder we have a separate .classpath entry. For other output folders we'd still have to store the info with the first corresponding source folder (or store it in all of them, but use just the first one?) > > Which solution do people prefer: > > (a) only support patching with a project > (a.1) wait if users request support for referring to individual source > folders > > (b) figure out the conflicting situations, and disallow only those. > (b.1) just prevent any "real damage" now, and file follow-up tasks for > exploring how far we can go. I'm not arguing it wouldn't be useful for users, I just said it might break many things in the current implementation. I just guess (a.1) is the path of least effort and still shouldn't stop us from supporting other options later, but if other options can be made to work now, I'm also sure there will be happy users. Actually allowing multiple module-info.java per project is something that later maven-compiler-plugin versions now does for test sources: You can have a separate module-info.java for test sources and if its module name is different, the test sources are treated as being in a separate module (but if it is the same as in the main sources, the main sources are treated as patch for the test sources during test source compilation)
(In reply to Stephan Herrmann from comment #35) > [jdk.policytool, jdk.charsets, jdk.pack, jdk.deploy, java.jnlp, jdk.snmp, > jdk.deploy.controlpanel, jdk.zipfs, jdk.crypto.ec, jdk.editpad, > java.smartcardio, java.activation, jdk.internal.vm.ci, jdk.naming.rmi, > jdk.internal.vm.compiler, jdk.javaws, jdk.rmic, jdk.jstatd, java.se.ee, > jdk.jcmd, jdk.xml.ws, jdk.scripting.nashorn.shell, java.xml.bind, > jdk.plugin, jdk.hotspot.agent, javafx.deploy, jdk.plugin.server, > jdk.crypto.cryptoki, java.transaction, java.xml.ws, java.xml.ws.annotation, > jdk.xml.bind, jdk.aot, java.corba, jdk.naming.dns, jdk.localedata] > > Those are in the list if you have module-info, but not when compiling for > the unnamed module, no? With patch set 4, I couldn't see a difference and the "add" pop was always empty, but with patch set 5, I can. Either something has changed in the patch, or there is some kind of caching issue and it was fixed by restarting (which I did for patch set 5).
(In reply to Stephan Herrmann from comment #33) > The list will show a "> " prefix for 'modified' modules, borrowing this > idiom from team/scm support :) > > - comment 24 (detail indicator "> ", see this comment) Thanks, much butter. I just note it currently doesn't work for jdk-modules. Also it is only updated if the dialog is reopened.
(In reply to Till Brychcy from comment #36) > Hmm in JEP 261 patch-module is used both for specifying that source to be > compiled (=source folders) should be part of another module and that other > jars or .class-folders should be treated like that (=output folders of > projects). Or: IPackageFragmentRoot (which can be binary or source). > I haven't tried, but I wonder: can you actually compile both > patching sources and non-patching sources in a differed folders in a single > javac invocation? I guess not in single-module mode, but probably in > multi-module mode? Some combinations with patch-module in multi-module mode are currently being discussed, e.g., patching a module currently being compiled. See https://mail.openjdk.java.net/pipermail/jigsaw-dev/2019-April/014206.html and below (from the same thread mentioned above). > But as written above in another comment: I think at least we'd have to > enforce that source folders have the same settings if they are compiled to > the same output folder. Yes. > Or we directly think in terms of the output folders instead of the source > folders (I mean the UI here) > Of course, only for the default output folder we have a separate .classpath > entry. For other output folders we'd still have to store the info with the > first corresponding source folder (or store it in all of them, but use just > the first one?) I prefer the other option above. (In reply to Till Brychcy from comment #38) > (In reply to Stephan Herrmann from comment #33) > > The list will show a "> " prefix for 'modified' modules, borrowing this > > idiom from team/scm support :) > > > > - comment 24 (detail indicator "> ", see this comment) > > Thanks, much butter. > I just note it currently doesn't work for jdk-modules. Also it is only > updated if the dialog is reopened. Both issues fixed locally.
New in patch set #6: We can now define, edit, view, remove target modules of add-exports and add-opens. This resolves my last must-have item from the TODO list in ModuleDependenciesPage.java. Wrt to defining multiple target modules for one add-exports, where are not better than the old UI: content assist is offered only for a single target module. Editing can optionally be triggered by double-click (in addition to button Edit). Both fixes regarding detail indicator "> " (see comment 38, comment 39).
*** Bug 525716 has been marked as a duplicate of this bug. ***
*** Bug 531127 has been marked as a duplicate of this bug. ***
Trivial issue fixed just now (patch set #7). (In reply to Till Brychcy from comment #32) > Regarding buttons: I think all buttons that open a dialog before performing > an action (so almost all) should end with "...", e.g. "Read Module..." > instead of "Read Module" Summarizing the status right now: From above we already have bug 546790. I extracted various non-blocking tasks from discussions into new bug 546797. As I don't see any blocking issues, my idea is to - let it sink in for two more days - push what we have to master to enable first field testing - continue improving until M3 via the other bugs. Any objections?
See also updates in https://wiki.eclipse.org/Java9/ModularityOptions/UIProposal_SH
(In reply to Stephan Herrmann from comment #43) > From above we already have bug 546790. Something similar is done in Launch configuration tabs when making a change in the JRE tab and moving to Classpath/Dependencies tab without clicking Apply. With patch set 7, changing EE from 12 to 8 in Libraries tab, clicking Apply and moving to the new tab has the buttons "Read Module..." etc. still enabled and clicking on that results in: java.lang.IllegalStateException: detail list has no ConfiguredDetails element at org.eclipse.jdt.internal.ui.wizards.buildpaths.ModuleDependenciesAdapter.getConfiguredDetails(ModuleDependenciesAdapter.java:814) ... These buttons should also be disabled. > I extracted various non-blocking tasks from discussions into new bug 546797. > > As I don't see any blocking issues, my idea is to > - let it sink in for two more days > - push what we have to master to enable first field testing > - continue improving until M3 via the other bugs. > > Any objections? Fine for me as this will allow everyone to test easily. (In reply to Stephan Herrmann from comment #16) > > Currently, they > > stick closer to the element name instead of the module icon. For example, > > see entries in Details pane. > > I believe this is (in part) an effect of > ModuleDependenciesPage.DecoratedImageDescriptor.getSize() returning > JavaElementImageProvider.BIG_SIZE. The overlay themselves are based on > existing overlays. Do you suggest to shift the content withing the overlay > (there's a 1 pixel transparent border at the left)? Can we try that and see if it looks better? > > - Not sure how the patch icon looks to everyone. Probably, a 'P' will be > > more consistent. > > I believe a visual distinction from the other elements is good, because > patching pulls in more source folders into an existing module (thus > affecting their compilation), whereas the other options only modify the API > / wiring of a module. > > If you prefer consistency of child nodes, then I'd revive my proposal to > create a new (third) toplevel node ("Patched with") - using module+patch as > its icon :) I don't think a third node is required. My concern with the current icon is that the red overlay is somewhat big and again sticks to the text. Not sure if reducing the overlay icon's size will look better.
Adding a class folder to modulepath on Libraries tab doesn't show the module name in the new tab. It shows only the module icon but no name.
Created attachment 278427 [details] Screenshot - Java Compiler page Regarding the 'Remove' button, the current approach looks fine. If it needs to be disabled in some cases then it can be done when all the selected entries are non-removable. The reason/guidance can be shown as an error message at the top/bottom of the tab as done in 'Java Compiler' page (see screenshot).
Add system module dialog has the 'Add' button enabled without any selection also when opened. Clicking on it gives this error: java.lang.NullPointerException at org.eclipse.jdt.internal.ui.wizards.buildpaths.ModuleSelectionDialog.getResult(ModuleSelectionDialog.java:312) ...
I addressed simple issues from comments 45 through 48 like so: (In reply to Noopur Gupta from comment #45) > With patch set 7, changing EE from 12 to 8 in Libraries tab, clicking Apply > and moving to the new tab has the buttons "Read Module..." etc. still > enabled and clicking on that results in: > java.lang.IllegalStateException: detail list has no ConfiguredDetails element > at > org.eclipse.jdt.internal.ui.wizards.buildpaths.ModuleDependenciesAdapter. > getConfiguredDetails(ModuleDependenciesAdapter.java:814) > ... > These buttons should also be disabled. done > (In reply to Stephan Herrmann from comment #16) > > I believe this is (in part) an effect of > > ModuleDependenciesPage.DecoratedImageDescriptor.getSize() returning > > JavaElementImageProvider.BIG_SIZE. The overlay themselves are based on > > existing overlays. Do you suggest to shift the content withing the overlay > > (there's a 1 pixel transparent border at the left)? > Can we try that and see if it looks better? > [..] > I don't think a third node is required. My concern with the current icon is > that the red overlay is somewhat big and again sticks to the text. Not sure > if reducing the overlay icon's size will look better. I reduced the bounding size, thus moving the overlays 2 pixels closer to the main icon (was easier than updating all icons :) ). While playing with this I noticed the 'P' would actually conflict with the 'J' from Java projects as patch source. OTOH, I do see that the patch overlay is bigger then necessary. I'll try to find the time to reduce it a bit. (In reply to Noopur Gupta from comment #46) > Adding a class folder to modulepath on Libraries tab doesn't show the module > name in the new tab. It shows only the module icon but no name. This I couldn't reproduce, can you elaborate more, what exactly you did? A class folder with module-info.class correctly shows the module name (albeit only after close-reopening the dialog - just apply didn't bring it into the list at all). A class folder without module-info.class (should actually be illegal on the module path??) shows the folder name as the module name, and decorates it as an automatic module. (In reply to Noopur Gupta from comment #48) > Add system module dialog has the 'Add' button enabled without any selection > also when opened. Clicking on it gives this error: > java.lang.NullPointerException > at > org.eclipse.jdt.internal.ui.wizards.buildpaths.ModuleSelectionDialog. > getResult(ModuleSelectionDialog.java:312) > ... Fixed for ModuleSelectionDialog and also for ModulePatchSourceSelectionDialog. Additionally: - make sure "Edit..." is disabled if nothing is selected - remove details when switching to 8-
Thanks, Stephan. The icons look better now. (In reply to Stephan Herrmann from comment #49) > (In reply to Noopur Gupta from comment #46) > > Adding a class folder to modulepath on Libraries tab doesn't show the module > > name in the new tab. It shows only the module icon but no name. > > This I couldn't reproduce, can you elaborate more, what exactly you did? - Project A having module-info.java. - Project B having module-info.java. In project B's build path Libraries tab, click 'Add Class Folder...' and select project A. - Apply, close and reopen the dialog and go to the new tab. The module icon with 'A' overlay is displayed but the name is missing. Let me know if this doesn't reproduce the issue. I will upload the projects as an attachment. Found some more issues: 1) In the new tab, select a module to remove e.g. jdk.sctp. Click Remove button in the dialog. I get the following exception in error log: java.lang.IllegalStateException: detail list has no ConfiguredDetails element at org.eclipse.jdt.internal.ui.wizards.buildpaths.ModuleDependenciesAdapter.getConfiguredDetails(ModuleDependenciesAdapter.java:814) ... 2) Also, for a project having module-info.java, initially all system modules are included so we can disable the 'Add System Module...' button. 3) For elements added via read and patch, the 'Edit...' button doesn't do anything so it should be disabled. I suggest that we move the remaining problems to separate bugs and close this bug for M2 today for better testing.
(In reply to Noopur Gupta from comment #50) > - Project A having module-info.java. > - Project B having module-info.java. In project B's build path Libraries > tab, click 'Add Class Folder...' and select project A. > - Apply, close and reopen the dialog and go to the new tab. The module icon > with 'A' overlay is displayed but the name is missing. > Let me know if this doesn't reproduce the issue. I will upload the projects > as an attachment. I was going to say: "I didn't know you can do that", then found: it actually doesn't work at all. Over to bug 546877 :) But, if it doesn't work at all, why did we show a name-less module? We asked JavaCore.getAutomaticModuleDescription(jProject), which always returns something. We still need to ask exists() before processing that handle.
(In reply to Stephan Herrmann from comment #51) > But, if it doesn't work at all, why did we show a name-less module? We asked > JavaCore.getAutomaticModuleDescription(jProject), which always returns > something. We still need to ask exists() before processing that handle. Nope, exists() doesn't work here, either (-> bug 546879).
Fixed in patch set #9: (In reply to Noopur Gupta from comment #50) > - Project A having module-info.java. > - Project B having module-info.java. In project B's build path Libraries > tab, click 'Add Class Folder...' and select project A. > - Apply, close and reopen the dialog and go to the new tab. The module icon > with 'A' overlay is displayed but the name is missing. > Let me know if this doesn't reproduce the issue. I will upload the projects > as an attachment. Reject modules with empty name (since exists() doesn't help, see bug 546879) > Found some more issues: > 1) In the new tab, select a module to remove e.g. jdk.sctp. Click Remove > button in the dialog. I get the following exception in error log: > java.lang.IllegalStateException: detail list has no ConfiguredDetails element > at > org.eclipse.jdt.internal.ui.wizards.buildpaths.ModuleDependenciesAdapter. > getConfiguredDetails(ModuleDependenciesAdapter.java:814) > ... fixed (need to expect that remove may wipe the details list) > 2) Also, for a project having module-info.java, initially all system modules > are included so we can disable the 'Add System Module...' button. done > 3) For elements added via read and patch, the 'Edit...' button doesn't do > anything so it should be disabled. done (for now Edit is enabled only for add-exports, add-opens).
Gerrit change https://git.eclipse.org/r/140893 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=facb46830a9f580c6eb62ba30ecc57bc30d6833b
(In reply to Noopur Gupta from comment #50) > I suggest that we move the remaining problems to separate bugs and close > this bug for M2 today for better testing. Thanks. I found the two test failures to be bug 545831. So I released (In reply to Eclipse Genie from comment #54) > Gerrit change https://git.eclipse.org/r/140893 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=facb46830a9f580c6eb62ba30ecc57bc30d6833b for 4.12 M3. Bonus: I even have "@2x" versions of the new overlays :) The saga will continue in bug 546797. Please add your items there, if I forgot anything or if you identify more issues. Thanks!
Initial N&N entry can be found here: https://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=2b3fd8e4a1fc7ba43a35ce1af16b745f779f7366 and here https://www.eclipse.org/eclipse/news/4.12/jdt.php#buildpath-module-dependencies
Stephan, you did check in PNG files (including @2x PNGs) but did not also commit the SVGs into the platform images repo. You you still have the SVGs (or what ever file format you used) you used to render these PNGs?
New Gerrit change created: https://git.eclipse.org/r/149110
(In reply to Matthias Becker from comment #57) > Stephan, > > you did check in PNG files (including @2x PNGs) but did not also commit the > SVGs into the platform images repo. You you still have the SVGs (or what > ever file format you used) you used to render these PNGs? Sorry for the delay, here you go: (In reply to Eclipse Genie from comment #58) > New Gerrit change created: https://git.eclipse.org/r/149110
Gerrit change https://git.eclipse.org/r/149110 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.images.git/commit/?id=4151a968ce468446ccfdcb82e4109467d81bfc1a
