Conversation
|
I just woke up, so I haven't had time to fully review it, but some initial concerns from me:
Regardless, the implementation here is really clever, well done! Also, feel free to ignore the Nix CI, I can fix that for you. |
|
Regarding 1, moonlight is actually already explicitly detected by Discord (and it's not for malicious reasons) so I don't think this makes much of a difference. It's already disabled by default, and I'm not sure how to add a Danger Zone marking given that it's toggled in moonbase, though an extra warning box is probably reasonable |
That detection doesn't actually trigger since we don't load libdiscore's separate Webpack entrypoint immediately - I've been meaning to solve it in a more pleasant way in #301.
I'm pretty sure having it as a dependency of Moonbase will enable it, given that we don't have a mechanism to do soft dependency resolution ATM. We should probably fix that... Alternatively, we could not integrate it with Moonbase at all and just have a separate settings tab, I guess?
|
NotNite
left a comment
There was a problem hiding this comment.
Sorry for the wait on reviewing this!
I'm pretty happy with how this is implemented overall, but I still think we should split this off from Moonbase proper into its own settings panel. This fits better as a standalone extension anyways, because we have the relevant config events already exposed in moonlight core. Beyond that, this seems very clean; well done!
| version: string; | ||
| branch: MoonlightBranch; | ||
|
|
||
| getFullConfig: () => Config; |
There was a problem hiding this comment.
Is there a particular reason we need getFullConfig over the config getter here?
| "indentStyle": "space", | ||
| "lineWidth": 120 | ||
| "lineWidth": 120, | ||
| "lineEnding": "auto" |
There was a problem hiding this comment.
Why was this changed? We should ideally be using LF everywhere (though I'm not actually sure if we have any CRLF files in the repository).
| "authors": ["Cynosphere", "NotNite", "redstonekasi"] | ||
| }, | ||
| "dependencies": ["spacepack", "settings", "common", "notices", "contextMenu"], | ||
| "dependencies": ["spacepack", "settings", "common", "notices", "contextMenu", "cloudSync"], |
There was a problem hiding this comment.
I'm of the opinion that we shouldn't directly tie this UI into Moonbase. Because it has cloudSync as a dependency, it'll be implicitly enabled by the dependency resolver and thus always enabled for every user; this is a weird quirk we do for libraries inherited from before we had proper dependency management in Moonbase.
I think the Cloud Sync extension should register a settings pane that's next to Moonbase on the UI:
. Integrating it into Moonbase itself is a bad idea IMO, as that should stay purely for extension management and moonlight settings.| import Margins from "@moonlight-mod/wp/discord/styles/shared/Margins.css"; | ||
| import Flex from "@moonlight-mod/wp/discord/uikit/Flex"; | ||
| import { Button } from "@moonlight-mod/wp/discord/uikit/legacy/Button"; | ||
| import { MoonbaseSettingsStore } from "@moonlight-mod/wp/moonbase_stores"; |
There was a problem hiding this comment.
We shouldn't be using MoonbaseSettingsStore here; just use the setConfigOption function on moonlight. The settings store should automatically pick up the change anyways:
| } | ||
|
|
||
| setExtensionConfig(id: string, key: string, value: any) { | ||
| setExtensionConfig(id: string, key: string, value: any, permanent: boolean = false) { |
There was a problem hiding this comment.
Remove this; this code is here so that config changes can be rolled back in Moonbase, so the concept of "permanent" here is unnecessary.
| } | ||
|
|
||
| export default function CloudSyncPage() { | ||
| const enabled = MoonbaseSettingsStore.getExtensionConfigRaw<boolean>("cloudSync", "enabled", false) ?? false; |
There was a problem hiding this comment.
If we're going to decouple this from Moonbase fully, I think we can remove this "enabled" switch, as the user can just disable the extension. The accountId option won't be displayed in Moonbase as long as it's not defined in the manifest, so we can keep that.
Settings are synced directly to Discord, no remote server required :)