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.

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

proof of concept: close Web UI window instead of hiding #1929

Closed
wants to merge 1 commit into from

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 19, 2021

This is a proof of concept for #1893. In this PR, the Web UI window is only created on demand, i.e., when we launch some URL on the Web UI or if the user explicitly set openWebUIAtLaunch to true. When the window is closed, it is actually closed and not hidden.

Electron runs 4 processes: Electron, Electron Helper, Electron Helper (GPU) and Electron Helper (Renderer). Only the renderer process is killed by the window closing. The GPU process is still on.

Problems with this approach:

  • OSes that do not have a menubar functionality cannot close IFPS Desktop.

@lidel what do you think?

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

My understanding is that nothing changes for OS that does not have a menubar functionality: closing the window keeps the daemon running in the background, just like it was before.

I am supportive for this change, however there is a regression on the Status screen:

The bandwidth graph it no longer gets populated when the window does not exist, so the user does not have correct bandwidth utility usage (it gets "paused" until window is opened again).

To solve this, ipfs-desktop would have to keep polling the API and storing stats, and when a window is created inject them into historical data ipfs-webui uses for plotting the graph.

Not sure how feasible this is, thoughts?

@hacdias
Copy link
Member Author

hacdias commented Nov 22, 2021

My understanding is that nothing changes for OS that does not have a menubar functionality: closing the window keeps the daemon running in the background, just like it was before.

You're right indeed.

To solve this, ipfs-desktop would have to keep polling the API and storing stats, and when a window is created inject them into historical data ipfs-webui uses for plotting the graph.

I think it is possible to do. I will take a look later today!

@hacdias
Copy link
Member Author

hacdias commented Nov 22, 2021

@lidel I also want to add here that I don't think I can fix the E2E tests easily: Spectron runs through the active windows so that means we cannot control the app if there's no active windows. In addition, Spectron is being deprecated. The most sensible alternative seems to be Playwright however I wonder if it'd suffer from the same issue since Electron apps were not built to be... menu bar apps.

Do you think I should go ahead with collecting with metrics in the background? If we follow that route, we need to see what to do with the tests.

@lidel
Copy link
Member

lidel commented Dec 3, 2021

I am afraid we don't have bandwidth for tackling those in parallel – i dont want you to invest time into feature which cant ship due to lack of tests.

We should solve spectron problem (tracked in #1902) and when we have e2e test situation resolved, get back to this PR and figure out how we can make this change while keeping confidence provided by currently existing e2e tests (refactor them etc).

@lidel lidel added the status/blocked Unable to be worked further until needs are met label Dec 3, 2021
@lidel lidel removed the status/blocked Unable to be worked further until needs are met label Feb 4, 2022
@lidel
Copy link
Member

lidel commented Feb 4, 2022

This is no longer blocked – we fixed e2e tests in #1937.
@hacdias mind rebasing?

@SgtPooki
Copy link
Member

SgtPooki commented May 6, 2022

If the attempt for this PR is to speed up the application, we can probably disregard it. There are multiple changes in flight that would make this change unnecessary, or at least cumbersome to do in parallel with.

@hacdias
Copy link
Member Author

hacdias commented May 6, 2022

@SgtPooki the main concern, mentioned in #1893, is actually the resources that the rendered process consumes in background. This PR would actually make the app slightly slower, as the Web UI Window would need to be completely rebuilt every time we open it. Right now, we just hide the window when the user closes, instead of actually closing it.

@hacdias hacdias changed the title PoC of actually closing window proof of concept: close Web UI window instead of kidding Jun 2, 2022
@hacdias hacdias changed the title proof of concept: close Web UI window instead of kidding proof of concept: close Web UI window instead of hiding Jun 2, 2022
@SgtPooki
Copy link
Member

@hacdias I think after #2125, "rebuilding the webui" everytime becomes a non-issue. So I'm completely fine with this PR and can take this over in the future if it's not something you can finish up

@SgtPooki
Copy link
Member

closing this since there is a large rebase required and we don't have the bandwidth to focus on this right now

@SgtPooki SgtPooki closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants