ipfs-desktop icon indicating copy to clipboard operation
ipfs-desktop copied to clipboard

proof of concept: close Web UI window instead of hiding

Open hacdias opened this issue 4 years ago • 6 comments

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?

hacdias avatar Nov 19 '21 13:11 hacdias

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 avatar Nov 22 '21 13:11 hacdias

@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.

hacdias avatar Nov 22 '21 16:11 hacdias

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 avatar Dec 03 '21 19:12 lidel

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

lidel avatar Feb 04 '22 02:02 lidel

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.

  • #2114 (ctx is changing)
  • #2125 - I've already got load time to an unobservable delay - load times to come with #2126
  • #2126 - we need metrics showing the problem before we should accept any PRs claiming to fix them.

SgtPooki avatar May 06 '22 18:05 SgtPooki

@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 avatar May 06 '22 19:05 hacdias

@hacdias I think after https://github.com/ipfs/ipfs-desktop/pull/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 avatar Nov 17 '22 22:11 SgtPooki

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

SgtPooki avatar Sep 20 '23 23:09 SgtPooki