8000 feat: continue-activity event is extended to support webpageURL property by 1akshat1 · Pull Request #30042 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: continue-activity event is extended to support webpageURL property #30042

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
Merged
merged 3 commits into from
Jul 13, 2021
Merged

feat: continue-activity event is extended to support webpageURL property #30042

merged 3 commits into from
Jul 13, 2021

Conversation

1akshat1
Copy link
Contributor
@1akshat1 1akshat1 commented Jul 7, 2021 •
edited
Loading

Description of Change

This PR aims to extend the existing continue-activity event params to resolve issue #29926.
Primarily a new details object is added as a parameter to the event, and webpageURL is added inside that details object if the userActivity contains webpageURL.

Checklist

Release Notes

Notes: Extended continue-activity event API to support webpageURL property from NSUserActivity.

@welcome
Copy link
welcome bot commented Jul 7, 2021

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jul 7, 2021
@1akshat1
Copy link
Contributor Author
1akshat1 commented Jul 7, 2021

@nornagon I tried testing the changes locally but I'm getting the details object as undefined in my app. I don't have experience with objective C so figured I'll request your input. To test I copied the Electron.app generated by e build to my App's electron node module folder. (Not sure if that would work or not).

I'm testing the changes manually by invoking Apple's handoff mechanism.

Notes on Apple's handoff mechanism for universal linking:

  1. Context can be passed from the Apple's browser to a native App.
  2. To achieve this, com.apple.developer.associated-domains entitlement must be placed on the server for the webpageURL we'd like universally link.
  3. The web domain which is to be linked needs to be added to the app's entitlement.plist.
  4. Once everything is in place and the packaged app is in Applications folder on Mac, macOs would be able to open the app when a user clicks the universal link rather than opening the link on browser.

More info:
https://developer.apple.com/library/archive/documentation/General/Conceptual/AppSearch/UniversalLinks.html
https://developer.apple.com/library/archive/documentation/UserExperience/Conceptual/Handoff/AdoptingHandoff/AdoptingHandoff.html#//apple_ref/doc/uid/TP40014338-CH2-SW10

@nornagon
Copy link
Contributor
nornagon commented Jul 7, 2021

Are you using electron-builder / electron-forge or something like it to create the .app bundle for your app to place it in /Applications? I think that will ignore whatever Electron.app is in your node_modules folder and download Electron itself...

@1akshat1
Copy link
Contributor Author
1akshat1 commented Jul 8, 2021

Are you using electron-builder / electron-forge or something like it to create the .app bundle for your app to place it in /Applications? I think that will ignore whatever Electron.app is in your node_modules folder and download Electron itself...

Yeah, that seems to be the case. I tried using electron-builder's property electronDist to point it to the custom build but it is throwing corrupted Electron dist error. Probably has to do with plist issue I think.

@1akshat1
Copy link
Contributor Author

@nornagon I was able to test this out using a boilerplate electron app and using electron-builder's electronDist property to use the custom Electron build. It is working as expected. I used an existing Apple provisioning profile to sign the build and an existing apple-app-site-association file on the server to complete Apple universal linking setup. I'm not sure how we could achieve this in an automated manner though.

@1akshat1 1akshat1 force-pushed the 1akshat1-ExtendContinueUserActivityAPI branch from b406c41 to 31bad10 Compare July 13, 2021 15:42
@1akshat1 1akshat1 marked this pull request as ready for review July 13, 2021 16:02
nornagon
nornagon reviewed Jul 13, 2021
View reviewed changes
nornagon
nornagon approved these changes Jul 13, 2021
View reviewed changes
Copy link
Contributor
@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

LGTM after docs update 👍

I'm OK without an automated test for this.

@1akshat1 @nornagon
docs: Mark webpageURL parameter as optional in docs
dc6d717 
Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
@1akshat1
Copy link
Contributor Author

@nornagon Seems like the build is failing on linux.

@nornagon nornagon added the semver/minor backwards-compatible functionality label Jul 13, 2021
@jkleinsc
Copy link
Member

API LGTM

@nornagon nornagon merged commit d267f97 into electron:main Jul 13, 2021
@welcome
Copy link
welcome bot commented Jul 13, 2021

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link
release-clerk bot commented Jul 13, 2021

Release Notes Persisted

Extended continue-activity event API to support webpageURL property from NSUserActivity.

BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
@1akshat1 @nornagon @BlackHole1
feat: continue-activity event is extended to support webpageURL prope…
2fffd31 
…rty (electron#30042)

Co-authored-by: Akshat Malik <amalik@microstrategy.com>
Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
@RaisinTen RaisinTen mentioned this pull request Jun 27, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

Assignees
No one assigned
Labels
api-review/requested 🗳 new-pr 🌱 PR opened recently no-backport semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@1akshat1 @nornagon @jkleinsc
0