Allow user select eap version of JetBrains IDE #7852
Conversation
roboquat
added
release-note-none
team: IDE
team: webapp
iQQBot
force-pushed
the
pd/add-latest-jb
branch
2 times, most recently
from
1416792 to
636130e
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Associated issue: #7023 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## main #7852 +/- ##
=========================================
- Coverage 12.01% 8.04% -3.97%
=========================================
Files 20 31 +11
Lines 1190 2212 +1022
=========================================
+ Hits 143 178 +35
- Misses 1043 2031 +988
+ Partials 4 3 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
/werft run with-helm 👍 started the job as gitpod-build-pd-add-latest-jb.3 |
|
@gtsiolis @loujaybee could you review UI/UX 🙏 please |
components/server/src/ide-config.ts
Outdated
| @@ -190,6 +191,23 @@ export class IDEConfigService { | |||
| } | |||
| } | |||
|
|
|||
| for (const [id, option] of Object.entries(newValue.ideOptions.options).filter(([_, x]) => x.latestImage)) { | |||
| if (option.latestImage == null || option.latestImage === "") { | |||
There was a problem hiding this comment.
This line doesn't really work, the real check is in the previous line, here it's just to remove the typescript error, of course can replace it with @ts-ignore
components/server/src/ide-config.ts
Outdated
| @@ -190,6 +191,23 @@ export class IDEConfigService { | |||
| } | |||
| } | |||
|
|
|||
| for (const [id, option] of Object.entries(newValue.ideOptions.options).filter(([_, x]) => x.latestImage)) { | |||
| if (option.latestImage == null || option.latestImage === "") { | |||
There was a problem hiding this comment.
| if (option.latestImage == null || option.latestImage === "") { | |
| if (option.latestImage === null || option.latestImage === "") { |
Is the shallow comparison on purpose? I find it possibly causing headaches to debug (e.g. undefined == null => true). Alternatively why not if (!option.latestImage) {?
There was a problem hiding this comment.
This line doesn't really work, the real check is in the previous line, here it's just to remove the typescript error, of course can replace it with @ts-ignore
| @@ -190,6 +191,23 @@ export class IDEConfigService { | |||
| } | |||
| } | |||
|
|
|||
| for (const [id, option] of Object.entries(newValue.ideOptions.options).filter(([_, x]) => x.latestImage)) { | |||
There was a problem hiding this comment.
this for loop is fairly complex, with many things happening in a single line - did you consider breaking it down a bit?
|
@iQQBot Let's put a checkbox in less prominent location like under the header or even under all desktop IDEs. Most of users don't need it. Is it possible to show concrete versions as label? Do we have this information in the metadata or can we resolve it somehow from docker image? Can we get it? |
|
@gtsiolis I need some visual design help, can you give me some help? |
|
Looking at this now! 👀 |
|
Thanks for the ping @iQQBot! FWIW, I agree with @akosyakov in #7852 (comment) about moving the checkbox to a less prominent location. This is also something we could use to consolidate all options including VS Code editors but let's leave this out of the scope of this PR. Cc @akosyakov @loujaybee Adding below a design for this. I've removed the rest of the preferences elements to focus on the desktop editors. Feedback is welcome! 🏀
|
Let's think about user stories here. I am not sure that's it all the same. There can be cases when you want to use stable for one, and latest for another. Maybe later we remove these big boxes instead have a drop down and add all versions there, stable and latest. Right now it is just for us to test against next version. |
|
Thanks for @gtsiolis the design plan, @akosyakov Do you think it's OK to follow the above design? If so I'll follow the design and implement it |
Yes, it looks good and enable us to test latest versions. |
iQQBot
force-pushed
the
pd/add-latest-jb
branch
from
ee80801 to
322255f
Compare
|
Already changed based on review suggestions |
No challenges with the proposed design for desktop. However, if we implement as suggested then the preferences page (for desktop) will become inconsistent to the VS Code preferences section above (for browser) where two UX for choosing the latest/EAP versions are provided. If we go with the UX suggested, I'd suggest we also update the VS Code preferences to the same checkbox UX for insiders (or we create a ticket for it). CC: @gtsiolis |
@loujaybee To clarify, the removal was only part of the review comment to focus on the changes in the scope of this . The proposal was not to consolidate browser and desktop editors in this PR, as discussed last week in our meeting with @akosyakov.
Agree. Opening a follow up issue sounds also good. 🍊 🍊 🍊 🍊 Long-term I still think consolidating the editors and using the latest and desktop options below could be better UX. However, probably not worth holding this PR back for this. Less choices, more control[1][2][3]. Happy to also discuss the limitations or benefits in a follow up issue or PR. Here's how this could look like:
|
|
Thanks for the update, @gtsiolis 🙏
I think we're in agreement on the proposal you mentioned. I also agree it doesn't make sense to prevent this PR progressing. I think we don't yet have an issue for what you mentioned, yet. So I'll make one now. In this case, I'm think I'm talking about something related, but different. EAP and insiders are essentially the same concept, i.e the user doesn't get a new IDE, but instead gets a pre-release/early version, so I think should be represented in the same way in the UI for both the desktop and browser. Merging the PR will bring a (non-critical) inconsistency. Happy to move this to another issue, but wanted to raise so that we were aware of the inconsistency before merging. Visual example:
The screenshot you attached would tackle both:
I'll make an issue with the changes you suggested @gtsiolis, then let's follow up as soon as we can after this PR with those changes to simplify/align the browser/desktop preferences. |
|
@loujaybee @gtsiolis @akosyakov |
|
/werft run 👍 started the job as gitpod-build-pd-add-latest-jb.6 |
iQQBot
force-pushed
the
pd/add-latest-jb
branch
from
322255f to
9266dc4
Compare
|
@iQQBot do you know why latest image still use CWM links? We never were able to rebuild latest images since Gateway plugin PR was merged? |
this is the latest image is not "latest", it build failed since Gateway plugin PR was merged |
Could you make sure that they are good? Together with @felladrin I don't want to deploy something what brings old functionality. |
|
Yes, I already ping him |
|
/werft run --with-clean-slate-deployment 👍 started the job as gitpod-build-pd-add-latest-jb.8 |
|
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-pd-add-latest-jb.9 |
|
/assign @gitpod-io/engineering-webapp |
There was a problem hiding this comment.
@iQQBot UX changes look incrementally good, since this will add the checkbox to use the latest EAP version without affecting anything else. Cc @akosyakov @loujaybee
@gitpod-io/engineering-webapp Could someone take a closer look at the code changes?
🍊 🍊 🍊 🍊
In the next iteration, we could probably consider improving
For
- Merge the IDEs into a single section
- Update card selector component and sidebar design #7932
- Use the same modality for the latest VS Code option and drop the extra Insiders option
- Update the Latest Release checkbox control so that this applies to all options
- Surface release version for each editor or IDE
- Introducing a checkbox control to set the preference for the desktop version, including the (un)pairing use cases like when a user selects an editor that is only available in desktop, etc.
- Mark VS Code as the default option so that we can fallback to it when users uncheck the desktop preference
For
- Improve more actions button on the workspace start page when opening in desktop IDEs #6910
|
@gtsiolis It would be cool to have issues for your ideas if we don't have them yet. The team could pull it one by one. |
| @@ -190,6 +191,20 @@ export class IDEConfigService { | |||
| } | |||
| } | |||
|
|
|||
| for (const [id, option] of Object.entries(newValue.ideOptions.options).filter(([_, x]) => x.latestImage)) { | |||
| try { | |||
| value.ideOptions.options[id].latestImage = await this.resolveImageDigest(option.latestImage!); | |||
There was a problem hiding this comment.
Nit: I would find it a bit "cleaner" to use a boolean like option.useLatestImage, and if it's true, set option.latestImage (just like option.resolveImageDigest and option.image above).
Here, we seem to use option.latestImage both as a "boolean" (i.e. filter(([_, x]) => x.latestImage)) above) and a resolved value .latestImage = await .... This looks a little bit dangerous (e.g. we might accidentally resolve the same image multiple times, or maybe forget to resolve it elsewhere, etc).
However, not blocking the review due to this. Please feel free to address in a follow-up PR.
There was a problem hiding this comment.
latestImage is a string not boolean, so filter(([_, x]) => x.latestImage) is equal than filter(([_, x]) => x.latestImage != null && x.latestImage !== '')
it no problem I think
There was a problem hiding this comment.
Thanks @iQQBot! Yes, I understand this, but my point was, I prefer something like this:
type Option = {
resolveLatestImage: boolean;
latestImage?: string;
}
if (option.resolveLatestImage) {
option.latestImage = await resolve();
}where the controlling condition is different from what's actually resolved.
Currently, the code seems to work like this:
type Option = {
latestImage?: string;
}
if (option.latestImage) {
option.latestImage = await resolve();
}This makes it a bit unclear if an image has already been resolved or not, e.g.:
- If the code is called multiple times, you might accidentally resolve this image multiple times
- You cannot do
if (!option.latestImage) { option.latestImage = await resolve(); } - Someone might accidentally "forget" to resolve the image in some cases (i.e. leave it as
option.latestImage === "latest", becauseif (option.latestImage)looks "true")
|
Agree with @gtsiolis that the IDE settings are becoming a little confusing:
But filing issues and iterating on this sounds like a good path forward. 😊 |
Description
Allow user select eap version of JetBrains IDE
Related Issue(s)
Relate #7023
How to test
We can check current Desktop IDE version by: Start a workspace, run
cat /ide-desktop/backend/product-info.json | jqRelease Notes
Documentation