Bug 521666

Comment 1 Stephan Herrmann 2017-08-31 08:07:42 EDT

While we're at it: how about --patch-module?

Comment 2 Sarika Sinha 2017-08-31 23:36:34 EDT

(In reply to Stephan Herrmann from comment #1)
> While we're at it: how about --patch-module?

Yes :)

Comment 3 Eclipse Genie 2017-09-03 14:57:31 EDT

New Gerrit change created: https://git.eclipse.org/r/104237

Comment 4 Stephan Herrmann 2017-09-03 14:59:23 EDT

(In reply to Eclipse Genie from comment #3)
> New Gerrit change created: https://git.eclipse.org/r/104237

Here patch set #1 only refactors the previous solution in preparation for different kinds of ModuleEncapsulationDetails (of which ModuleAddExports still is the only concrete subclass).

Next I'll try to fit --add-reads into this frame.

Comment 5 Stephan Herrmann 2017-09-03 19:18:58 EDT

(In reply to Stephan Herrmann from comment #4)
> Next I'll try to fit --add-reads into this frame.

Done in patch set #2.

The main FIXME also here: clarify icons, add help (-context)


The remaining options --patch-module and --limit-modules have slightly different structure. For both I could imagine working with simple checkboxes in a table. (Also Core support is still missing for those).

patch-module comes with the additional constraint, that each project can patch at most one module (I assume). Is it possible to place radio buttons in a table (column)?

When adding more details to the dialogs, I wonder if the detail dialog should use a tab folder to switch between the lists, rather then try to show all at once (as I'm doing so far).

Comment 6 Stephan Herrmann 2017-09-07 15:36:16 EDT

Patch set #3 adds support for patch-module, which requires the Core change from https://git.eclipse.org/r/#/c/104660/

For patch-module I decided to simply add a checkbox and a subordinate text field. Validation & content assist are all in place. Icons & help text are not.

So far no need for multiple tabs. Perhaps in the end we want two tabs, like:

[x] Defines one or more modules

[ Encapsulation Details ]  [ Contents ]

I.e., the first check box dominates all. 

Then Encapsulation Details is the tab containing everything we have so far: patch-module, add-exports, add-reads.

Contents would handle the --limit-modules options. I like the pattern of two lists  "available" and "selected". I don't see an example of this in JDT (for stealing some code :) ). Is this used somewhere in JDT? If not, would anyone mind if I introduce this pattern?

Comment 7 Stephan Herrmann 2017-09-14 17:31:57 EDT

Created attachment 270204 [details]
new Contents tab

This shows the new Contents tab in action.

Comment 8 Stephan Herrmann 2017-09-14 17:33:35 EDT

Created attachment 270205 [details]
new Details tab

Previous widgets from add-exports plus patch-module & add-reads one a new tab "Details".

Comment 9 Stephan Herrmann 2017-09-14 17:55:08 EDT

In patch set #4 I consider the new UI feature complete.

I'd appreciate if people could start testing this, with two caveats in mind:

- opening the dialog on the JDK library is prohibitively slow (and still fails to read necessary information if source attachment is missing, this is bug 522293 in JDT/Core).

- once you saved a configuration with limit-modules (i.e., you made modifications on the Contents tab) the next invocation of the dialog will no longer see the excluded modules, where actually they should be seen in "Available". Need to figure out, if new API from core is needed.

=> workaround during testing: manually remove limit-modules from .classpath, then close & re-open the project :-/

Other than that, the design of the dialogs is complete, and a lot of content assist and validations have been added, even tooltips where considered useful...

I intentionally do not give more explanation as of now, to give people a chance to test if everything is intuitively clear, once you start using it :)


Additionally, playing with Till's suite of bug 522326, bug 522327, bug 522328, bug 522330 indicates, that builder & reconciler don't yet agree in some of these situations. This means the effect of all the new options on compilation is still unstable as of today (some implementations of INameEnvironment are probably lagging in functionality). Those core issues I hope to wrap up over the weekend.

Comment 10 Eclipse Genie 2017-09-16 08:18:07 EDT

Gerrit change https://git.eclipse.org/r/104237 was merged to [BETA_JAVA9].
Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=8362220c2bf1e277c0bf682fdd1d0d3624bb63ec

Comment 11 Stephan Herrmann 2017-09-16 08:24:45 EDT

(In reply to Eclipse Genie from comment #10)
> Gerrit change https://git.eclipse.org/r/104237 was merged to [BETA_JAVA9].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=8362220c2bf1e277c0bf682fdd1d0d3624bb63ec
> 

I pushed my implementation to encourage everybody to test the new/updated dialog.

I'll leave the bug open for two more days, to handle immediate follow-up.

Notable change wrt patch set #4: I removed my ad-hoc icon from Bug 519444 and instead use the new one from bug 521662. Also now consume the provisional API from bug 522293 to resolve the performance issue and fixed a few logical issues.

Comment 12 Stephan Herrmann 2017-09-17 12:50:58 EDT

As of commit http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?h=BETA_JAVA9&id=cd3a5636d6fef999a2566de32bba8762a47c7cac the "Contents" tab of a JRE container with no --limit-modules is populated following the rules for root modules from JEP 261.

This is done by consuming new provisional API from bug 522328

Comment 13 Stephan Herrmann 2017-09-19 19:18:49 EDT

Per commit https://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?h=BETA_JAVA9&id=14a07e573299dcad5bb197a2c99f1dd80a8cd80b I fixed an icon (disabled vs. enabled versions).

Resolving.

Comment 14 Vikas Chandra 2017-09-20 11:52:26 EDT

With this change, the user is not able to click "OK"  after setting "Define one or more modules"  ( check the checkbox ) on Module Properties dialog of non-modular external jars. It has an error "Error exists on Contents tab" and OK is disabled. Is this design intentional ?

Comment 15 Stephan Herrmann 2017-09-20 17:15:04 EDT

(In reply to Vikas Chandra from comment #14)
> With this change, the user is not able to click "OK"  after setting "Define
> one or more modules"  ( check the checkbox ) on Module Properties dialog of
> non-modular external jars. It has an error "Error exists on Contents tab"
> and OK is disabled. Is this design intentional ?

Does it show a specific error on the Contents tab?
I imagine that no module was selected in the "Included..." section, but perhaps no module was actually found in the jar? Maybe we fail to recognize automatic modules.

Comment 16 Vikas Chandra 2017-09-21 00:03:25 EDT

>>Does it show a specific error on the Contents tab?
>> no module was actually found in the jar?

There was no error on the Contents tab and the jar doesn't have modules. Should OK be enabled or disabled for such cases?

Comment 17 Sarika Sinha 2017-09-21 01:35:18 EDT

(In reply to Stephan Herrmann from comment #15)
> (In reply to Vikas Chandra from comment #14)
> > With this change, the user is not able to click "OK"  after setting "Define
> > one or more modules"  ( check the checkbox ) on Module Properties dialog of
> > non-modular external jars. It has an error "Error exists on Contents tab"
> > and OK is disabled. Is this design intentional ?
> 
> Does it show a specific error on the Contents tab?
> I imagine that no module was selected in the "Included..." section, but
> perhaps no module was actually found in the jar? Maybe we fail to recognize
> automatic modules.

Yes, It's the case of automatic modules!!

Comment 18 Stephan Herrmann 2017-10-06 18:06:38 EDT

To close the loop:

(In reply to Sarika Sinha from comment #17)
> (In reply to Stephan Herrmann from comment #15)
> > (In reply to Vikas Chandra from comment #14)
> > > With this change, the user is not able to click "OK"  after setting "Define
> > > one or more modules"  ( check the checkbox ) on Module Properties dialog of
> > > non-modular external jars. It has an error "Error exists on Contents tab"
> > > and OK is disabled. Is this design intentional ?
> > 
> > Does it show a specific error on the Contents tab?
> > I imagine that no module was selected in the "Included..." section, but
> > perhaps no module was actually found in the jar? Maybe we fail to recognize
> > automatic modules.
> 
> Yes, It's the case of automatic modules!!

This should now be fixed via bug 525213.