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

Replace internal remote module usage #1148

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

Conversation

timothyis
Copy link
Member

@timothyis timothyis commented Oct 27, 2022

The intended purpose of this pull request is to replace all built-in remote module usage in our own code.

There are multiple instances of remote being used in external modules, but we should address those in a further PR.

This is a work in progress.

Fixes #1089

@timothyis timothyis marked this pull request as ready for review November 9, 2022 19:02
@timothyis timothyis mentioned this pull request Nov 9, 2022
@sindresorhus
Copy link
Member

Very nice work! It works great for me in dev mode.

@sindresorhus
Copy link
Member

I did run into an issue when in the built version though (npm run list). I cannot get past the screen recording permission prompt no matter how many times I give permission and restart. I cannot reproduce this issue in Kap 3.6.0.

.gitignore Outdated Show resolved Hide resolved
@timothyis
Copy link
Member Author

Ah, perhaps I'm not up-to-date with main. Let me see what I can do here 🙏🏼

@timothyis timothyis force-pushed the update/replace-internal-remote-usage branch from 6ef4d35 to d168c40 Compare November 10, 2022 10:52
@timothyis
Copy link
Member Author

Looks like this still happens after rebasing with main. For me the following occurs after rebasing:

  1. Double click Kap in the dist directory and a blank space appears in the tray where Kap is meant to be. Nothing else happens. (Kap shows up in the activity monitor however). I think this is when the "Move to applications folder required" message should pop up.
  2. Force quit Kap from the activity monitor (won't just quit normally)
  3. Open Kap from the applications folder (it moved itself there automatically), the Kap icon appears in the dock and tray and I get a dialog asking for permissions. No combination or disabling or re-enabling works to solve this issue. Kap still won't quit normally and needs force-quitting.

I think perhaps these parts still use Electron remote upstream somewhere and I need to check that.

@sindresorhus
Copy link
Member

I think perhaps these parts still use Electron remote upstream somewhere and I need to check that.

Most likely, yes.

@timothyis
Copy link
Member Author

Just to note on the first issue; the dialog for moving Kap to the apps directory isn't a remote issue. I completely ripped remote out and it still didn't work. In my minimal test app, I upgraded Electron and electron-builder to the latest versions and it worked as expected. I'll try updating Electron until that first part works and then move on to the permissions issue.

@wojtiku
Copy link

wojtiku commented Mar 8, 2023

Sorry for intruding here. I love Kap a lot but is has been unusable for the past few months because of a ~30s delay before being able to record. This drove me to try different things including building locally and trying out this branch. To the point though:

After upgrading to MacOS Ventura (Intel), I also had a permission issue with Kap where I was unable to grant the permission no matter how many times I would agree - the dialog just kept appearing. This includes this branch.

What I found out to always work though was selecting kap on the list and clicking the minus (1) button and then clicking the plus button (2) and selecting the binary I just installed. This fixes the permission issue every time for me.
image

Hope this makes it easier for you to debug.

P.S.

Regarding issue #1125, even though this branch doesn't fix it for me as I hoped it would, it changes things - on 3.6.0 I need to wait about 20-30s after clicking the Kap icon for anything to show up and to record. On this branch, the UI (grey box with controls and the red record button) is shown immediately after clicking Kap icon but it is frozen for 20-30s before I can do anything.

I love Kap and I can't find any alternatives since it stopped working for me so I really cheer to your efforts.

@timothyis
Copy link
Member Author

Hi @wojtiku, thank you for commenting. I too found that fix worked for me, but this sadly also stopped me from debugging it! I wish I could reproduce it.

I'm very busy with work right now, but I'm hoping I can find some time to finish this PR in the next few weeks.

I spoke with Sindre privately about the best method forward and hoping that this can be closed soon and we can move onto other issues.

Just to note that this branch does not yet include the Electron updates to latest, or switching to IPC instead of remote, which should bring a lot of performance improvements.

Really appreciate your kind words and patience 🙏🏼

@wojtiku
Copy link

wojtiku commented Mar 8, 2023

Regarding reproduction, I was able to consistently get to a permission-related problem with the following steps on this branch:

  1. use minus button to remove Kap from the permission list
  2. do not add it manually
  3. open Kap built on this branch (I did not verify with current prod)
  4. you will get the permission dialog (go to system prefs or deny)
  5. click go to system prefs
  6. allow kap to record the screen
  7. click quit and reopen in the dialog
  8. after reopening click kap in the top toolbar
  9. everything is frozen indefinitely, seems like there is invisible overlay on the screen preventing any interaction
    fortunately I have terminal under a keyboard shortcut and killall Kap solves the issue

Hope this helps. I don't think I could contribute with such a complex issue not knowing the project but if you need a tester feel free to mention me and I will test on my side.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to @electron/remote
3 participants