Add magnet protocol integration and default-handler status #138
Conversation
mayswind
left a comment
mayswind
left a comment
There was a problem hiding this comment.
Great Job!
I have reviewed all your code and have submitted a few suggestions to this PR.
| stop.description=Zastaví aplikaci po uplynutí SEC sekund. Pokud je zadáno 0, tato funkce je zakázána. | ||
| truncate-console-readout.name=Oříznout výstup v konzoli | ||
| truncate-console-readout.description=Ořízne výstup do konzole tak, aby se vešel na jeden řádek. | ||
| [global] |
There was a problem hiding this comment.
If you don't learn a language, please do not submit translation file for it, as inaccurate translations may mislead other users.
You may leave these entries empty in these languages.
| stop.description=在此选项设置的时间(秒)后关闭应用. 如果设置为 0, 此功能将禁用. | ||
| truncate-console-readout.name=缩短控制台输出内容 | ||
| truncate-console-readout.description=缩短控制台输出的内容在一行中. | ||
| [global] |
There was a problem hiding this comment.
This file already has a "global" section. You should place newly added entries near the related adjacent entries in the UI similar to what you did in the app/scripts/config/defaultLanguage.js file.
| config.execCommandOptionsOnStartup = 'as-detached-process'; | ||
| } | ||
|
|
||
| config.enableMagnetProtocol = originalConfig.enableMagnetProtocol !== false; |
There was a problem hiding this comment.
config.enableMagnetProtocol = !!originalConfig.enableMagnetProtocol
| }); | ||
| }; | ||
|
|
||
| ariaNgNativeElectronService.getMagnetProtocolStatusAsync().then(function (status) { |
There was a problem hiding this comment.
The promise returned by ariaNgNativeElectronService.getMagnetProtocolStatusAsync() can be combined with the default loadingPromise (L649) using $q.all. This provides a better user experience by ensuring that this status is fully loaded when the user opens the interface.
There was a problem hiding this comment.
In addition, the surrounding code in this area is all assigning values to $scope, so this code should be placed at the end of the file, together with the loadingPromise logic.
| </select> | ||
| <span class="input-group-addon"> | ||
| <span translate>Is default magnet handler</span>: | ||
| <span ng-if="context.magnetProtocolStatus.isDefault" translate>Yes</span> |
There was a problem hiding this comment.
Would using bootstrap label here provide a better user experience?
There was a problem hiding this comment.
Also, I suggest placing the "Yes" and "No" entries earlier in the language file, for example near "Enabled" and "Disabled".
| shell.openExternal(url); | ||
| }); | ||
|
|
||
| ipcMain.handle('render-open-system-default-apps-setting', async () => { |
There was a problem hiding this comment.
This project does not use the async / await keywords consistently.
| return (tray.isEnabled() || os.platform() === 'darwin') && config.minimizedToTray; | ||
| }; | ||
|
|
||
| let pendingMagnetUrl = null; |
There was a problem hiding this comment.
notifyRenderProcessNewNewTaskFromTextAfterViewLoaded already provides the ability to execute after the view has finished loading, so there should be no need to control this via pendingMagnetUrl.
| let pendingMagnetUrl = null; | ||
|
|
||
| let isMagnetUrl = function (value) { | ||
| return typeof value === 'string' && /^magnet:\?/i.test(value); |
There was a problem hiding this comment.
Would using string matching function here be more efficient than using a regular expression?
| }); | ||
| }); | ||
|
|
||
| app.on('open-url', (event, url) => { |
There was a problem hiding this comment.
open-url is macOS-specific. I recommended to place it after app.on('open-file') and also within app.on('will-finish-launching') to make the code clearer.
| } | ||
| } | ||
|
|
||
| const secondInstanceMagnetUrl = getMagnetUrlFromArgv(argv); |
There was a problem hiding this comment.
This code is better placed in the else branch of if (secondInstanceArgv && secondInstanceArgv.file && file.isContainsSupportedFileArg(secondInstanceArgv.file)) { so that the control flow is clearer.
| 'Yes': 'Yes', | ||
| 'No': 'No', | ||
| 'Magnet protocol is enabled but this app is not the default handler. Please update system settings.': 'Magnet protocol is enabled but this app is not the default handler. Please update system settings.', | ||
| 'Open default apps settings': 'Open default apps settings', |
There was a problem hiding this comment.
The above entries are missing:
"Default magnet app"
"This feature is not supported on your system."
"Failed to open system settings."
2 participants
Summary
I’ve been using AriaNg Native for a long time and it has been great. The one thing that kept bothering me was having to copy and paste magnet links manually every time. Since the native app can register protocol handlers, I decided to add first-class
magnet:support.This PR adds:
magnet:protocol handling across platformsNotes
Testing