feat: allow extensions to customize icons (#1899) #3131

lstocchi · 2023-07-06T10:48:48Z

What does this PR do?

This PR allow extensions to customize icons in the container list.

As it uses the when clause which has to be supported by #1898 I only added the minimum code to support the in/not in operator. All the rest will be worked in that issue.

Screenshot/screencast of this PR

What issues does this PR fix or reference?

This solves part of #1899

How to test this PR?

go to the container list and check that the image of the kind container is different

N.B: The image is ugly as i was not able to create a good one when converting svg to woff. I asked @mairin to help me. When she has some free time she will make it better. I left my ugly one as a placeholder.

benoitf

I was under the impression that the context/view should be checked on the client side and not on the backend side

If someone update externally the context, how client will be refreshed ?

benoitf · 2023-07-06T11:59:30Z

packages/renderer/src/lib/ContainerList.svelte

+function iconClass(container: ContainerInfoUI): string | undefined {  
+  // handle ${} in icon class
+  // and interpret the value and replace with the class-name
+  let icon;
+  if (container.icon) {
+    const match = container.icon.match(/\$\{(.*)\}/);
+    if (match !== null && match.length === 2) {
+      const className = match[1];
+      icon = container.icon.replace(match[0], `podman-desktop-icon-${className}`);
+      return icon;
+    }
+  }
+  return icon;
+}


probably need to move a separate component if we're having more and more this interpretation on the client side (I think we had that in status bar as well)

packages/renderer/src/lib/ContainerList.svelte

lstocchi · 2023-07-06T15:29:19Z

I was under the impression that the context/view should be checked on the client side and not on the backend side

By looking at the predefined context key used by vscode it would make sense to put it in the client as it has the knowledge of almost everything (active editor, the language used, workspace, etc..) but in our case we could provide something more advanced that the client has no knowledge about?
Maybe if you start executing multiple when evaluation in the list page it gets too slow at rendering?

In this PR it makes sense to have it in the server as the client should just show the icon but if i look the predefined values by vscode i have doubts.

If someone update externally the context, how client will be refreshed ?

What do you mean by that? An example?
I expect that if an extension is added/removed its contributions trigger a refresh and then the context is recalculated.

benoitf · 2023-07-06T15:42:41Z

If someone update externally the context, how client will be refreshed ?

What do you mean by that? An example?
I expect that if an extension is added/removed its contributions trigger a refresh and then the context is recalculated.

context is what an extension do at any time. For example an extension can decide to change a context value if a command is executed by the user, anything.

so if users have for example:

"when": "io.x-k8s.kind.cluster in containerLabelKeys && kindIsActive",

any extensions can update the kindIsActive value.

Then, it should trigger a refresh of the UI element being impacted.

but here, UI won't be refreshed until a container is not added/removed, etc.

lstocchi · 2023-07-06T16:02:37Z

context is what an extension do at any time. For example an extension can decide to change a context value if a command is executed by the user, anything.

so if users have for example:
"when": "io.x-k8s.kind.cluster in containerLabelKeys && kindIsActive",
any extensions can update the kindIsActive value.

Then, it should trigger a refresh of the UI element being impacted.

but here, UI won't be refreshed until a container is not added/removed, etc.

Ok, i got your example but aren't those values (kindIsActive, containerLabelKeys) just predefined values known by PD that are replaced with the right values? E.g in vscode you have the contextKey editorFocus but an external extension doesn't change that value directly. Maybe it can use (just inventing a command here) editor.movefocusto(3) and then vscode update the contextKey accordingly.

The main problem here is that the context contains info regarding a single container (containerLabelKeys) and we need to update it before to evaluate the when for each container. So we cannot really refresh the UI after each context update ... i wonder if the containerLabelKeys should be treated differently.

benoitf · 2023-07-06T16:08:57Z

but an external extension doesn't change that value directly. Maybe it can use (just inventing a command here) editor.movefocusto(3) and then vscode update the contextKey accordingly.

Here is the example: https://code.visualstudio.com/api/extension-guides/command#using-a-custom-when-clause-context

lstocchi · 2023-07-06T16:27:17Z

but an external extension doesn't change that value directly. Maybe it can use (just inventing a command here) editor.movefocusto(3) and then vscode update the contextKey accordingly.

Here is the example: https://code.visualstudio.com/api/extension-guides/command#using-a-custom-when-clause-context

Ah, right. i also used that command in some other project 😅
BTW, going back to the problem (server vs client) i still don't have a clear idea. We can create a context store in the client to notify it... this way if there is no need to update the store for some reason (e.g like when updating the container label list) we prevent the UI from refreshing.

Having everything on the client would not it be too much? It should know that it needs to update the labels in the context for each container ...

benoitf · 2023-07-06T16:35:59Z

for sure it's not all server side and all client side.
Most of the things is on the 'plugin/server side' but we need a 'refresh/trigger to display it again' on the client side and it should update only the item that needs to be refreshed. (for example we should not pull again the list of containers each time context store is updated)
But each time a context is changed and a when clause uses that property, we should refresh the UI

lstocchi · 2023-07-07T13:22:22Z

I've been thinking a bit. In few words it could be described like this

There are 2 different families of context keys. Ones that are pre-calculated and stored on the server (e.g
isWindows, isLinux ...) and ones that are calculated at runtime (terminalIsOpen, containerLabels, ...).

The server keeps track of the contexts (one per extension) When an extension gets installed, a context is initialized with default values (if any, i guess in future we could have a list similar to vscode) and each extension could add its own contextKeys.

The client has a context store which will be updated when a context in the backend is updated. So it will have the knowledge of the pre-filled contextkeys or if an external extension updates something.
The client is responsible for setting contextKey at runtime that it is aware of (terminalIsOpen, containerLabels, ...) but their value will not be actually save on the server.
The client has the logic to evaluate the when clause, so when a context is updated it can perform the evaluation without calling the server again and re-download all the content.

benoitf · 2023-07-07T13:29:26Z

@lstocchi 👍

benoitf · 2023-07-07T14:01:06Z

also if you think some parts should be already be merged, we could merge some elements

lstocchi · 2023-07-07T14:05:37Z

@benoitf unfortunately i need to refactor everything a bit. Even if some classes will not be changed internally they will need to be moved in another place.

lstocchi · 2023-07-11T10:08:49Z

PR refactored following #3131 (comment)

I'll address @benoitf comment #3131 (comment) in another PR.

Once approved, i'll remove the kind icon woff2 before merging as it looks bad. I'm keeping this so you can see something/copy&replicate on your own custom extension.

In other PRs i'll work on supporting other formats because woff2 is not suitable for complex icons and i'll add a new kind icon as well.

cdrage

The code looks great and amazing job! This is a huge change that will add the ability to customize the icon.

I am a tad confused, I see that in your example for kind, we use containerLabelKeys.

I'm assuming that this is the only thing that is supported? Because do set keys here: https://github.com/containers/podman-desktop/pull/3131/files#diff-78630035cd0851326c0b30ac2a140aa5ebe9c71ae419027d9e44b9ade5e56164R99

So that means that we can't do anything else, correct? Example blahblahblah in podLabelKeys, or something like that.

If so, we should add documentation somewhere on how to do this so this information is not lost.

You'll have to update https://github.com/containers/podman-desktop/blob/main/EXTENSIONS.md with instructions on how to use this feature / how to add the icon to package.json, etc.

From understanding the code, basically... we scan and check to see if the container label includes io.x-k8s.kind.cluster. It doesn't check the key however though right? Just makes sure that io.x-k8s.kind.cluster exists.

cdrage · 2023-07-11T18:34:15Z

packages/renderer/src/lib/context/contextKey.ts

+ *
+ * SPDX-License-Identifier: Apache-2.0
+ ***********************************************************************/
+/* eslint-disable @typescript-eslint/no-explicit-any */


All of the below code is from https://github.com/microsoft/vscode/blob/main/src/vs/platform/contextkey/common/contextkey.ts

We are allowed to use it (MIT), but I believe we should attribute / add comments that we have used it from the above part.

Yes this is something i wanted to ask. The core of the context/when evaluation/scanner has been taken from vscode (in accordance with Florent) and i wasn't sure how this should be treated.
Should i copy their copyright label instead of ours? And only add ours where i added custom code?

lstocchi · 2023-07-12T08:06:38Z

The code looks great and amazing job! This is a huge change that will add the ability to customize the icon.

I am a tad confused, I see that in your example for kind, we use containerLabelKeys.

I'm assuming that this is the only thing that is supported? Because do set keys here: https://github.com/containers/podman-desktop/pull/3131/files#diff-78630035cd0851326c0b30ac2a140aa5ebe9c71ae419027d9e44b9ade5e56164R99

So that means that we can't do anything else, correct? Example blahblahblah in podLabelKeys, or something like that.

Correct, this PR only adds support to the containerLabelKeys context key

If so, we should add documentation somewhere on how to do this so this information is not lost.

You'll have to update https://github.com/containers/podman-desktop/blob/main/EXTENSIONS.md with instructions on how to use this feature / how to add the icon to package.json, etc.

I wanted to tackle it in another PR.
This is just the first piece of the puzzle. Once merged, and everyone agree with these changes, I would have to extend it to the pod/image/volume tabs.
Then i want to document how one can customize a view, which are the context keys supported, the when rules that can be used, etc....

From understanding the code, basically... we scan and check to see if the container label includes io.x-k8s.kind.cluster. It doesn't check the key however though right? Just makes sure that io.x-k8s.kind.cluster exists.

Yes this is the way we handle containerLabelKeys, then maybe we can create containerLabelValue to check the value or something else. It depends on what we want to offer/our needs. So these are the context values natively supported by PD.
In future (not supported in this PR) external extensions could add their own context value with the setContext command.

packages/extension-api/src/extension-api.d.ts

cdrage · 2023-07-14T14:24:18Z

The code looks great and amazing job! This is a huge change that will add the ability to customize the icon.
I am a tad confused, I see that in your example for kind, we use containerLabelKeys.
I'm assuming that this is the only thing that is supported? Because do set keys here: https://github.com/containers/podman-desktop/pull/3131/files#diff-78630035cd0851326c0b30ac2a140aa5ebe9c71ae419027d9e44b9ade5e56164R99
So that means that we can't do anything else, correct? Example blahblahblah in podLabelKeys, or something like that.

Correct, this PR only adds support to the containerLabelKeys context key

If so, we should add documentation somewhere on how to do this so this information is not lost.
You'll have to update https://github.com/containers/podman-desktop/blob/main/EXTENSIONS.md with instructions on how to use this feature / how to add the icon to package.json, etc.

I wanted to tackle it in another PR. This is just the first piece of the puzzle. Once merged, and everyone agree with these changes, I would have to extend it to the pod/image/volume tabs. Then i want to document how one can customize a view, which are the context keys supported, the when rules that can be used, etc....

From understanding the code, basically... we scan and check to see if the container label includes io.x-k8s.kind.cluster. It doesn't check the key however though right? Just makes sure that io.x-k8s.kind.cluster exists.

Yes this is the way we handle containerLabelKeys, then maybe we can create containerLabelValue to check the value or something else. It depends on what we want to offer/our needs. So these are the context values natively supported by PD. In future (not supported in this PR) external extensions could add their own context value with the setContext command.

Ah I understand.

The code LGTM although we will have to double-check with regards to licensing. I know it's MIT so we can use it, but it's a kind gesture to reference where we got it from.

This implementation looks good, but yeah, I agree that we need to have ample documentation on how we can use it as it looks quite important from an extension perspective. It would be nice to customize each icon through the extension (even add little easer eggs too!).

packages/main/src/plugin/context-registry.ts

packages/main/src/plugin/view-registry.ts

benoitf · 2023-07-18T14:00:36Z

packages/renderer/src/lib/ContainerList.svelte

+                    {#if iconClass(container)}
+                      <StatusIcon icon="{iconClass(container)}" status="{container.state}" />
+                    {:else}
+                      <StatusIcon icon="{ContainerIcon}" status="{container.state}" />


wondering if it should be part of the container object ?

like container.icon

packages/renderer/src/lib/ContainerList.svelte

packages/main/src/plugin/context/context.ts

packages/renderer/src/stores/contexts.ts

packages/renderer/src/lib/ContainerList.svelte

Signed-off-by: lstocchi <lstocchi@redhat.com>

benoitf · 2023-07-20T19:43:06Z

what about this font ?

font.zip

packages/renderer/src/lib/images/StatusIcon.svelte

Signed-off-by: lstocchi <lstocchi@redhat.com>

lstocchi · 2023-07-21T09:30:22Z

@benoitf refactored the code following your suggestions.

I also used your icon. I don't know where you found it but it's much better than mine.

benoitf

awesome work 🚀

benoitf · 2023-07-21T09:51:07Z

@benoitf refactored the code following your suggestions.
I also used your icon. I don't know where you found it but it's much better than mine.

I changed to be a monochromatic at the SVG level before transforming to a font with my limited image editing skills

@lstocchi I think @mairin icon will be definitely way better as part of #3296 but we're good for this step, it'll be improvement

cdrage

No comments on the code and awesome work! Looking forward to the documentation so we can implement this into the other extensions.

One nitpick is that the icon is very close to the edge compared to our other icons, but I see that #3296 has been created.

LGTM

benoitf · 2023-07-21T13:47:09Z

svg was using no-border, here version with space around the border
a.zip

Signed-off-by: lstocchi <lstocchi@redhat.com>

lstocchi · 2023-07-21T14:40:33Z

svg was using no-border, here version with space around the border a.zip

Thanks @benoitf . Updated

cdrage · 2023-07-21T14:46:11Z

svg was using no-border, here version with space around the border a.zip

Thanks @benoitf . Updated

Much better! Thank you for updating it.

lstocchi requested review from a team and benoitf as code owners July 6, 2023 10:48

lstocchi requested review from jeffmaury and cdrage and removed request for a team July 6, 2023 10:48

benoitf reviewed Jul 6, 2023

View reviewed changes

lstocchi force-pushed the i1899 branch 2 times, most recently from 1bd60b2 to f8b8726 Compare July 11, 2023 10:05

lstocchi requested a review from benoitf July 11, 2023 10:05

cdrage requested changes Jul 11, 2023

View reviewed changes

benoitf reviewed Jul 13, 2023

View reviewed changes

packages/extension-api/src/extension-api.d.ts Outdated Show resolved Hide resolved

lstocchi force-pushed the i1899 branch from 6c95096 to 03de2fb Compare July 18, 2023 04:45

lstocchi requested a review from benoitf July 18, 2023 04:46

benoitf reviewed Jul 18, 2023

View reviewed changes

afbjorklund mentioned this pull request Jul 19, 2023

feat: Show the Lima instance name #2689

Closed

This was referenced Jul 19, 2023

feat: add initial support for contexts #3277

Merged

feat: add support for views contributions #3278

Merged

lstocchi added 4 commits July 20, 2023 18:50

fix: keep storage to server and move evaluation to renderer

cc11fd6

Signed-off-by: lstocchi <lstocchi@redhat.com>

fix: clean and add tests

8102c5e

Signed-off-by: lstocchi <lstocchi@redhat.com>

fix: rename file and fix function signature

7b69d8b

Signed-off-by: lstocchi <lstocchi@redhat.com>

fix: move contextKeyValue type

01136d0

Signed-off-by: lstocchi <lstocchi@redhat.com>

lstocchi force-pushed the i1899 branch from 03de2fb to 01136d0 Compare July 20, 2023 16:59

fix: clean after messed rebase

e2523e4

Signed-off-by: lstocchi <lstocchi@redhat.com>

benoitf reviewed Jul 20, 2023

View reviewed changes

packages/renderer/src/lib/images/StatusIcon.svelte Outdated Show resolved Hide resolved

mairin mentioned this pull request Jul 20, 2023

Design: Single-color icon artwork for kind #3296

Open

lstocchi force-pushed the i1899 branch from 8f7943a to 50657d4 Compare July 21, 2023 09:26

fix: replace icon and refactor

ef8ef3a

Signed-off-by: lstocchi <lstocchi@redhat.com>

lstocchi force-pushed the i1899 branch from 50657d4 to ef8ef3a Compare July 21, 2023 09:28

lstocchi requested review from benoitf and cdrage July 21, 2023 09:30

benoitf approved these changes Jul 21, 2023

View reviewed changes

cdrage approved these changes Jul 21, 2023

View reviewed changes

cdrage mentioned this pull request Jul 21, 2023

Demo: Seal icon for demos / easter egg #3307

Closed

chore: replace icon with one with margin

998a78a

Signed-off-by: lstocchi <lstocchi@redhat.com>

lstocchi merged commit 1ebe4c7 into containers:main Jul 21, 2023
8 checks passed

lstocchi deleted the i1899 branch July 21, 2023 15:02

lstocchi mentioned this pull request Jul 21, 2023

Differentiate the "Kind Container" in the list of container #1951

Closed

podman-desktop-bot added this to the 1.3.0 milestone Jul 21, 2023

afbjorklund mentioned this pull request Jul 21, 2023

Add monochrome version and webfont icon containers/podman-desktop-extension-minikube#8

Closed

lstocchi mentioned this pull request Jul 25, 2023

allow extensions to customize icons in container list (or image list, etc) #1899

Closed

benoitf mentioned this pull request Aug 16, 2023

extensions: adds a ContextKey service #1898

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow extensions to customize icons (#1899) #3131

feat: allow extensions to customize icons (#1899) #3131

lstocchi commented Jul 6, 2023 •

edited

benoitf left a comment

benoitf Jul 6, 2023

lstocchi commented Jul 6, 2023 •

edited

benoitf commented Jul 6, 2023

lstocchi commented Jul 6, 2023

benoitf commented Jul 6, 2023

lstocchi commented Jul 6, 2023

benoitf commented Jul 6, 2023

lstocchi commented Jul 7, 2023

benoitf commented Jul 7, 2023

benoitf commented Jul 7, 2023

lstocchi commented Jul 7, 2023

lstocchi commented Jul 11, 2023 •

edited

cdrage left a comment

cdrage Jul 11, 2023

lstocchi Jul 12, 2023

lstocchi commented Jul 12, 2023

cdrage commented Jul 14, 2023

benoitf Jul 18, 2023

benoitf commented Jul 20, 2023

lstocchi commented Jul 21, 2023

benoitf left a comment

benoitf commented Jul 21, 2023

cdrage left a comment

benoitf commented Jul 21, 2023

lstocchi commented Jul 21, 2023

cdrage commented Jul 21, 2023

feat: allow extensions to customize icons (#1899) #3131

feat: allow extensions to customize icons (#1899) #3131

Conversation

lstocchi commented Jul 6, 2023 • edited

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

benoitf left a comment

Choose a reason for hiding this comment

benoitf Jul 6, 2023

Choose a reason for hiding this comment

lstocchi commented Jul 6, 2023 • edited

benoitf commented Jul 6, 2023

lstocchi commented Jul 6, 2023

benoitf commented Jul 6, 2023

lstocchi commented Jul 6, 2023

benoitf commented Jul 6, 2023

lstocchi commented Jul 7, 2023

benoitf commented Jul 7, 2023

benoitf commented Jul 7, 2023

lstocchi commented Jul 7, 2023

lstocchi commented Jul 11, 2023 • edited

cdrage left a comment

Choose a reason for hiding this comment

cdrage Jul 11, 2023

Choose a reason for hiding this comment

lstocchi Jul 12, 2023

Choose a reason for hiding this comment

lstocchi commented Jul 12, 2023

cdrage commented Jul 14, 2023

benoitf Jul 18, 2023

Choose a reason for hiding this comment

benoitf commented Jul 20, 2023

lstocchi commented Jul 21, 2023

benoitf left a comment

Choose a reason for hiding this comment

benoitf commented Jul 21, 2023

cdrage left a comment

Choose a reason for hiding this comment

benoitf commented Jul 21, 2023

lstocchi commented Jul 21, 2023

cdrage commented Jul 21, 2023

lstocchi commented Jul 6, 2023 •

edited

lstocchi commented Jul 6, 2023 •

edited

lstocchi commented Jul 11, 2023 •

edited