eclipse-ee4j / starter

Eclipse Starter for Jakarta EE
Eclipse Public License 2.0
50 stars 40 forks source link

Enable Jakarta version selection initially in UI #198

Closed scottkurz closed 1 year ago

scottkurz commented 1 year ago

With defaults of Jakarta EE 10 and Core profile, to enable say Jakarta 9 + Web you have to click on the Web profile and then go back up to click on the Jakarta 9 version. If the Jakarta version is selectable initially you can just click on the Jakarta version and proceed from top-to-bottom (and the profile version selection will be updated to Web profile by the underlying model update).

eclipse-starter-bot commented 1 year ago

Can one of the admins verify this patch?

m-reza-rahman commented 1 year ago

We can easily do this if we change the default to the full platform or Web Profile instead of the Core Profile. Otherwise, we basically start with an invalid setting.

scottkurz commented 1 year ago

We can easily do this if we change the default to the full platform or Web Profile instead of the Core Profile. Otherwise, we basically start with an invalid setting.

I was thinking of maybe defaulting to Web or Full... but this seems like a separate point to me.

After this PR, we still start with Core + EE 10. If you change the EE level say to 9.1 we still go through the same org.eclipse.starter.ui.Project.onJakartaVersionChange() logic, so if you change to earlier than 10 the Core profile will be disabled and the button selection will transfer to the next choice (Web)......

(Sorry you can see I don't do a lot of this and am not using correct terminology)

m-reza-rahman commented 1 year ago

The problem is that it is inconsistent with the rest of the validation flow and can result in choosing core profile for an invalid Jakarta EE version (we can account for that, but it’s a tiny bit awkward/inconsistent). It’s the most consistent to just switch the default profile choice. Also, please do adapt the rest of the defaults. For example, enabling GlassFish since it does indeed support the other profiles for EE 10. It’ll be potentially less confusing if the form state always represents currently valid options.

scottkurz commented 1 year ago

The problem is that it is inconsistent with the rest of the validation flow and can result in choosing core profile for an invalid Jakarta EE version

@m-reza-rahman, I'm not following.

And it's not that I'm trying to be insistent on making the change of initially enabling Jakarta version selection without changing the rest of the defaults. It's just without being able to understand your comment I hesitate to make any change at all. I'd feel like I just was lacking a baseline understanding.

As I'm understanding this, the code here: https://github.com/eclipse-ee4j/starter/blob/85bb756dda1e0d401d4f9ff25fb5a062ba800f76/ui/src/main/java/org/eclipse/starter/ui/Project.java#L148-L161

is going to make sure that, if the first change I make is to the Jakarta version, that the 'core' profile setting will be replaced with 'web'. Are you saying this validation flow is clear with the current code (in master) but wouldn't be with my PR here?

I'm confused.

UPDATE: But I added a 2nd commit switching the default to web profile and re-enabling Glassfish initially.

m-reza-rahman commented 1 year ago

Definitely not straightforward so it's OK and understandable. It took me quite a while to try to get the validation logic right. I think you'll see that going through the code. To be honest, I am not really even sure now that it is a 100% right or even consistent.

Where I landed on after a significant amount of trial-and-error is enabling/disabling options proactively on selection, trying to ensure the form options are always valid, and avoiding "automatically" altering form values (that are potentially deliberate user input), which wind up having weird side effects (there are some edge cases where I wound up doing this anyway very carefully). The change you suggest may wind up working fine, but I am not a 100% sure it won't have some other weird side effect since that is not what I had in mind while writing the rest of the logic. My basic assumption is that at least to start with, we have all valid options given a default selection (and with some luck hopefully I have gotten it to a point where that is actually always true).

Hence, I am suggesting if we change the initial defaults, the most reliable thing to do is just to ensure the form is in a valid state to start with (if the profile is Core, EE versions other than 10 are not valid options to allow a user to choose). So the safest change here is changing the default profile choice to probably full and making the rest of the changes accordingly (including enabling all EE versions). You can give it a shot and I will try to double check it is right. Hopefully it's not too hard given you won't need to try to alter the actual validation logic just yet.

As a side note, I have to say overall I am not sure I really think the validation logic is written in the most optimal way. I will keep trying but if possible, help in writing that code is very much appreciated. Getting a reasonable, consistent and readable structure I am having a hard time arriving at without introducing the possibility of allowing the user to enter invalid input or having weird side effects.

Does this help?

m-reza-rahman commented 1 year ago

I also see that part of the issue is that I had some dead code that would not be reached (probably from the aforementioned trial-and-error efforts). I've removed that now: https://github.com/eclipse-ee4j/starter/pull/200. It may make it easier to understand why it adds even more complexity if the form options aren't kept valid for a given selection (and quite possibly user confusion too)?

m-reza-rahman commented 1 year ago

I've gone ahead and changed the default profile to full: https://github.com/eclipse-ee4j/starter/pull/201. That opens up the EE version range and also enables GlassFish.

OndroMih commented 1 year ago

I agree with @m-reza-rahman that it should be enough to set the default to Full profile. Simpler is better here. But we can add some info tooltip to the Jakarta EE version selection that versions that do not support the selected profile are disabled.

It could be useful if people could set Core profile, then Jakarta EE 8, and the profile would automatically change to Web profile. But it could also be confusing, people would expect that if they switch back to Jakarta EE 10, the profile would be hanged back to Core. We would have to implement this too or warn the user that the profile was changed automatically.

So it's easier to just change the default. We can assume that the user will have to select Core profile intentionally and then they will only see Jakarta EE versions that are available.

scottkurz commented 1 year ago

I'm going to close this PR.

Changing the default profile to Full does seem to solve the biggest part of the usability issue I was raising.

Agree that like Ondro suggested, it might be nice to be able to select a different Jakarta version and have the profile selection change, but I'm fine saying someone could tackle that later, possibly.


I think maybe the confusion here btw. Reza and I did center around the #200 code. Still not 100% sure I follow, but I think it might be that Reza had already recognized that code as "dead" in the master branch, and didn't realize that my PR here actually was relying on this very code to switch the profile (from 'core' to 'web') after a Jakarta version switch to <10.

But it's gone now and the immediate problem is gone.