Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: avoid button 'flickering' during extension start #6863

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chore: avoid button 'flickering' during extension start #6863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
285fe7c
chore: avoid button 'flickering' during extension start
deboer-tim Apr 22, 2024
702b651
chore: add tests for lifecycle state
deboer-tim Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
112 changes: 110 additions & 2 deletions packages/renderer/src/lib/extensions/InstalledExtensionCardLeftLifecycle.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ beforeEach(() => {
(window as any).removeExtension = vi.fn();
});

test('Expect to see start and delete on stopped pd Extension', async () => {
test('Expect to see start and delete on stopped pd extension', async () => {
const extension: CombinedExtensionInfoUI = {
type: 'pd',
id: '',
Expand All @@ -53,12 +53,49 @@ test('Expect to see start and delete on stopped pd Extension', async () => {
// should have start button and delete button
const start = screen.getByRole('button', { name: 'Start' });
expect(start).toBeInTheDocument();
expect(start).toBeEnabled();

const deleteButton = screen.getByRole('button', { name: 'Delete' });
expect(deleteButton).toBeInTheDocument();
expect(deleteButton).toBeEnabled();

// should not have stop button
const stop = screen.queryByRole('button', { name: 'Stop' });
expect(stop).not.toBeInTheDocument();
});

test('Expect to see disabled start and delete on starting pd extension', async () => {
const extension: CombinedExtensionInfoUI = {
type: 'pd',
id: '',
name: 'foo',
description: 'my description',
displayName: '',
publisher: '',
removable: true,
version: 'v1.2.3',
state: 'starting',
path: '',
readme: '',
};
render(InstalledExtensionCardLeftLifecycle, { extension });

// should not have stop button
const stop = screen.queryByRole('button', { name: 'Stop' });
expect(stop).not.toBeInTheDocument();

// should have disabled start button
const start = screen.getByRole('button', { name: 'Start' });
expect(start).toBeInTheDocument();
expect(start).toBeDisabled();

// should have disabled delete button
const deleteButton = screen.getByRole('button', { name: 'Delete' });
expect(deleteButton).toBeInTheDocument();
expect(deleteButton).toBeDisabled();
});

test('Expect to see stop on started pd Extension', async () => {
test('Expect to see stop and disabled delete on started pd extension', async () => {
const extension: CombinedExtensionInfoUI = {
type: 'pd',
id: '',
Expand All @@ -81,4 +118,75 @@ test('Expect to see stop on started pd Extension', async () => {
// should have stop button
const stop = screen.getByRole('button', { name: 'Stop' });
expect(stop).toBeInTheDocument();
expect(stop).toBeEnabled();

// should not have start button
const start = screen.queryByRole('button', { name: 'Start' });
expect(start).not.toBeInTheDocument();

const deleteButton = screen.getByRole('button', { name: 'Delete' });
expect(deleteButton).toBeInTheDocument();
expect(deleteButton).not.toBeEnabled();
});

test('Expect to see disabled start and delete on stopping pd extension', async () => {
const extension: CombinedExtensionInfoUI = {
type: 'pd',
id: '',
name: 'foo',
description: 'my description',
displayName: '',
publisher: '',
removable: true,
version: 'v1.2.3',
state: 'stopping',
path: '',
readme: '',
};
render(InstalledExtensionCardLeftLifecycle, { extension });

// should not have stop button
const stop = screen.queryByRole('button', { name: 'Stop' });
expect(stop).not.toBeInTheDocument();

// should have disabled start button
const start = screen.getByRole('button', { name: 'Start' });
expect(start).toBeInTheDocument();
expect(start).toBeDisabled();

// should have disabled delete button
const deleteButton = screen.getByRole('button', { name: 'Delete' });
expect(deleteButton).toBeInTheDocument();
expect(deleteButton).toBeDisabled();
});

test('Expect to see disabled start and enabled delete on unknown state pd extension', async () => {
const extension: CombinedExtensionInfoUI = {
type: 'pd',
id: '',
name: 'foo',
description: 'my description',
displayName: '',
publisher: '',
removable: true,
version: 'v1.2.3',
state: 'unknown',
path: '',
readme: '',
};
render(InstalledExtensionCardLeftLifecycle, { extension });

// should not have stop button
const stop = screen.queryByRole('button', { name: 'Stop' });
expect(stop).not.toBeInTheDocument();

// should have disabled start button
const start = screen.getByRole('button', { name: 'Start' });
expect(start).toBeInTheDocument();
expect(start).toBeDisabled();

// should have delete button
const deleteButton = screen.getByRole('button', { name: 'Delete' });
expect(deleteButton).toBeInTheDocument();
expect(deleteButton).toBeEnabled();
});
2 changes: 1 addition & 1 deletion packages/renderer/src/lib/extensions/InstalledExtensionCardLeftLifecycleStart.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ async function startExtension(): Promise<void> {
}
</script>

{#if extension.state === 'stopped'}
{#if extension.state !== 'started'}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it feels strange to see the start button if the state is not started.
let say state is 'unknown' or 'deleted' or something different we could see the button and this is not expected

probably need to tweak the accepted state including the starting or other intermediate steps

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The start/stop buttons already correctly enable/disable when they are not applicable, i.e. we could show both of them always and the page works fine. It is only our choice to only display one at a time to save space/reduce clutter, and flickering occurs because there are intermediate states where we have no button and so the layout adjusts.

This change is identical to what we did in the containers view, just making sure that the button visibility states are completely opposite so that you always & only see one button:

podman-desktop/packages/renderer/src/lib/container/ContainerActions.svelte

Line 141 in 9477baa

hidden="{container.state === 'RUNNING' || container.state === 'STOPPING'}"
. Neither button is enabled in these states, i.e. we could do the opposite change to the stop button.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is only our choice to only display one at a time to save space/reduce clutter,

to clarify, it's the person that did the mockup choice ;-)

and flickering occurs because there are intermediate states where we have no button and so the layout adjusts.

I understand the problem but I would avoid the usage of saying 'anything that is not started' to display the start button as while it does the trick currently, it does not guarantee it will always work as it can have side effects with the button 'stop' being displayed in some state that should not permit that.

so I would list there the states that are allowed (including the intermediate states) so it's clear that it will always be displayed for the states that we want/list and not something that may work but is fragile

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify, it's the person that did the mockup choice ;-)

;-)

I understand the problem but I would avoid the usage of saying 'anything that is not started' to display the start button as while it does the trick currently, it does not guarantee it will always work as it can have side effects with the button 'stop' being displayed in some state that should not permit that.

so I would list there the states that are allowed (including the intermediate states) so it's clear that it will always be displayed for the states that we want/list and not something that may work but is fragile

We are missing tests for LoadingIconButton, but that is where we should really be verifying correct enablement of the buttons with different states. The Resources page has start/stop/restart all visible so that will help too.

Which means that the fragile part here is mostly just ensuring that if state === x and if state != x are exactly opposite between these two buttons and only one appears. In ContainerActions that's obvious enough because they're 9 lines apart, but not here. So, I've added tests to the lifecycle component that cover both which buttons appear in each state and enablement, let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the condition is still erroneous to me. We can't hide a start button if we're in a state different than 'started'. This is semantically incorrect and it can lead to errors

If you prefer as you're telling that the LoadingIconButton is already smart. It's to add a property to this component so it'll hide if condition is not satisfied as well.

so instead of doing the if there, it's done inside the component that will control its display usage

Either it includes all the valid states where it should be displayed or it's inside the LoadingIconButton component as well but it should still use states semantically valid.

<LoadingIconButton
clickAction="{() => startExtension()}"
action="start"
Expand Down