URL handling #6243

mwu-tow · 2023-04-11T19:16:38Z

Pull Request Description

This PR fixes #5239 by supporting the Windows-style of URL handling to support deep linking.

Windows spawns a new process for each URL, rather than sending a 'open-url' event to the existing process. Now the differences between the two platforms should be abstracted away.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

# Conflicts: # app/ide-desktop/lib/client/src/authentication.ts

app/ide-desktop/lib/client/src/url-associations.ts

app/ide-desktop/lib/client/src/project-management.ts

somebody1234 · 2023-04-17T12:58:45Z

not explicitly approving since it'd be good to have another pair of eyes on this. also not sure this will work on linux (since it uses appimages) so i might try QAing this?

app/ide-desktop/lib/client/src/url-associations.ts

somebody1234 · 2023-04-17T14:05:50Z

encountered a bug while testing so this took a while.
this seems to work on linux however:

opens a second instance (correct) but that second instance opens a window which flashes before disappearing (bad!). we may want to consider not opening a window if electron detects that it is the second instance (if possible)
i clicked "login with google". deep link was received, however ⚠️nothing happened⚠️. logs are below although likely doesn't explain the error

Got data from 'second-instance' event: '/tmp/.mount_enso-l26b4zj/enso,--allow-file-access-from-files,enso://auth?code=REDACTED&state=REDACTED'.
URL callback completed, releasing the lock.
Checking if 'enso://auth?code=REDACTED&state=REDACTED' denote a URL to open.
Parsed 'enso://auth?code=REDACTED&state=REDACTED' as URL: enso://auth?code=REDACTED&state=REDACTED. Protocol: enso:.
Got URL from second instance: 'enso://auth?code=REDACTED&state=REDACTEDf'.
Received 'open-url' event for 'enso://auth?code=REDACTED&state=REDACTED'.

side note: may want to redact codes and/or states, manually with regex replacement, and/or by not printing out the link 6 separate times
- alternatively, if the link is safe to post (e.g. time-sensitive and does not contain sensitive info and cannot be used to identify the user), then it might be good to add a log line explaining that

somebody1234 · 2023-04-17T15:05:46Z

there were also two zombie enso processes, not sure if that's even related to this issue, or whether it's unavoidable linux weirdness

mwu-tow · 2023-04-17T15:10:09Z

@somebody1234
Sorry for not making this clear before, the deep linking is not supported yet on Linux, it is expected to work on Windows and macOS only for now.

wdanilo · 2023-04-18T10:10:08Z

app/ide-desktop/lib/client/src/authentication.ts

+export function onOpenUrl(url: URL, window: () => electron.BrowserWindow) {
+    logger.log(`Received 'open-url' event for '${url.toString()}'.`)
+    if (url.protocol !== `${common.DEEP_LINK_SCHEME}:`) {
+        logger.error(`${url.toString()} is not a deep link, ignoring.`)


${url.toString()} should be quoted.

wdanilo · 2023-04-18T10:29:39Z

app/ide-desktop/lib/client/src/file-associations.ts

@@ -146,6 +148,29 @@ export function handleOpenFile(openedFile: string): string {
        }
        logger.error(e)
        electron.dialog.showErrorBox(common.PRODUCT_NAME, message)
+        // eslint-disable-next-line no-restricted-syntax


@somebody1234 why we still need such strange linter config in code? I remember we were talking about fixing linter config some time ago and making such annotations not needed anymore. What is the status of this topic? :)

the cause of this specific one is bad eslint config, it should be able to be removed once #6267 is merged

app/ide-desktop/lib/client/src/url-associations.ts

PabloBuchu · 2023-04-22T14:39:41Z

QA 🟢

@mwu-tow I was able to make it finally running on my Windows.. We may think on some loader animation or at least disabling the button while user is waiting for the browser to open.. Other than that great work and thank you for making it running

# Conflicts: # app/ide-desktop/lib/client/src/file-associations.ts

app/ide-desktop/lib/client/src/project-management.ts

somebody1234 · 2023-04-24T13:55:40Z

app/ide-desktop/lib/client/src/url-associations.ts

+    if (!electron.app.requestSingleInstanceLock()) {
+        const message = 'Another instance of the application is already running. Exiting.'
+        logger.error(message)
+        // eslint-disable-next-line no-restricted-syntax


this should be redundant too

This one seems to be still required.

ah, yeah, my bad - i think you can suppress it by explicitly doing else { return } though

somebody1234 · 2023-04-24T13:56:38Z

i haven't git checkouted this branch so i'm not 100% sure they can be removed - but it should be quick enough to check

* develop: Turn null into UnexpectedExpression when Union type is incomplete (#6415) Ensure that IO.println does not fail if `to_text` returned a non-Text value. (#6223) Improve inlining of `<|` on (GraalVM EE) (#6384) Feat: update templates styles and feature (#6071) 5127 Add Table.parse_to_columns to parse a single column to a set of columns. (#6383) URL handling (#6243) Exclude comparison operators from modifier logic (#6370) Better cleanup of project management test suite (#6392) Fix all eslint errors (#6267) Infer SQLite types locally (#6381) Vector Editor with List Editor and adding elements. (#6363) Add typechecks to Aggregate and Cross Tab (#6380) Forbid edits in read-only mode (#6342) Add Table.parse_text_to_table to convert Text to a Table. (#6294) Adding typechecks to Column Operations (#6298) Rollback cloud options groups (#6331) Project folder not renamed after project name change (#6369) Add `replace`, `trim` to Column. Better number parsing. (#6253) Read-only mode for controllers (#6259) Prevent default for all events, fixing multiple IDE bugs (#6364)

This PR fixes #5239 by supporting the Windows-style of URL handling to support deep linking. Windows spawns a new process for each URL, rather than sending a 'open-url' event to the existing process. Now the differences between the two platforms should be abstracted away.

[wip]

8fd0145

mwu-tow requested review from MichaelMauderer, wdanilo and kazcw as code owners April 11, 2023 19:16

mwu-tow marked this pull request as draft April 11, 2023 19:16

mwu-tow added 6 commits April 11, 2023 21:25

[wip]

19b71b1

wip

a4d83f2

Merge remote-tracking branch 'origin/develop' into wip/mwu/url-handling

775e689

Merge remote-tracking branch 'origin/develop' into wip/mwu/url-handling

4fe411c

# Conflicts: # app/ide-desktop/lib/client/src/authentication.ts

Merge remote-tracking branch 'origin/develop' into wip/mwu/url-handling

88ddf9c

wip

69dfb06

mwu-tow marked this pull request as ready for review April 17, 2023 12:45

mwu-tow requested review from somebody1234, PabloBuchu and indiv0 as code owners April 17, 2023 12:45

somebody1234 reviewed Apr 17, 2023

View reviewed changes

app/ide-desktop/lib/client/src/url-associations.ts Show resolved Hide resolved

app/ide-desktop/lib/client/src/project-management.ts Outdated Show resolved Hide resolved

somebody1234 reviewed Apr 17, 2023

View reviewed changes

app/ide-desktop/lib/client/src/url-associations.ts Outdated Show resolved Hide resolved

mwu-tow changed the title ~~[wip] URL handling~~ URL handling Apr 17, 2023

wdanilo approved these changes Apr 18, 2023

View reviewed changes

mwu-tow added 3 commits April 18, 2023 23:51

Merge remote-tracking branch 'origin/develop' into wip/mwu/url-handling

25ec667

cr

7c4e350

Merge remote-tracking branch 'origin/develop' into wip/mwu/url-handling

a42ff57

PabloBuchu approved these changes Apr 21, 2023

View reviewed changes

somebody1234 approved these changes Apr 22, 2023

View reviewed changes

Focus on the Window when receiving URL from a second instance.

74357cb

mwu-tow added 4 commits April 23, 2023 21:11

Merge remote-tracking branch 'origin/develop' into wip/mwu/url-handling

c6dc916

fmt

2a20095

Merge remote-tracking branch 'origin/develop' into wip/mwu/url-handling

5bf4393

Merge remote-tracking branch 'origin/develop' into wip/mwu/url-handling

e7b5e1b

# Conflicts: # app/ide-desktop/lib/client/src/file-associations.ts

mwu-tow self-assigned this Apr 24, 2023

mwu-tow added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 24, 2023

somebody1234 approved these changes Apr 24, 2023

View reviewed changes

removed unnecessary lint annotation

b59e32f

mwu-tow merged commit 1920342 into develop Apr 24, 2023

mwu-tow deleted the wip/mwu/url-handling branch April 24, 2023 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

URL handling #6243

URL handling #6243

mwu-tow commented Apr 11, 2023 •

edited

Loading

somebody1234 commented Apr 17, 2023

somebody1234 commented Apr 17, 2023

somebody1234 commented Apr 17, 2023

mwu-tow commented Apr 17, 2023

wdanilo Apr 18, 2023

wdanilo Apr 18, 2023

somebody1234 Apr 18, 2023

PabloBuchu commented Apr 22, 2023

somebody1234 Apr 24, 2023

mwu-tow Apr 24, 2023

somebody1234 Apr 24, 2023

somebody1234 commented Apr 24, 2023

URL handling #6243

URL handling #6243

Conversation

mwu-tow commented Apr 11, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

somebody1234 commented Apr 17, 2023

somebody1234 commented Apr 17, 2023

somebody1234 commented Apr 17, 2023

mwu-tow commented Apr 17, 2023

wdanilo Apr 18, 2023

Choose a reason for hiding this comment

wdanilo Apr 18, 2023

Choose a reason for hiding this comment

somebody1234 Apr 18, 2023

Choose a reason for hiding this comment

PabloBuchu commented Apr 22, 2023

somebody1234 Apr 24, 2023

Choose a reason for hiding this comment

mwu-tow Apr 24, 2023

Choose a reason for hiding this comment

somebody1234 Apr 24, 2023

Choose a reason for hiding this comment

somebody1234 commented Apr 24, 2023

mwu-tow commented Apr 11, 2023 •

edited

Loading