-
-
Notifications
You must be signed in to change notification settings - Fork 832
Initial Electron Settings - for Auto Launch #920
Conversation
(opens path for Proxy Settings) Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Can one of the admins verify this patch? |
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
return "Not implemented"; | ||
} | ||
|
||
isElectron(): boolean { return false; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels a bit weird to pollute BasePlatform with knowledge of electron - could we not just check the type of PlatformPeg to see if it's a VectorBasePlatform or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you mean? I can't do PlatformPeg.get() instanceof ElectronPlatform
from the react-sdk side of things. How about a generic getFlags()
method on BasePlatform and descendants that would have a prop of isElectron = true on Electron Platform, no idea what else it'd be useful for but it seems like the most generic way of solving this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or since now they have getHumanReadableName could do it based off of that but that feels dirty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could also just be a property and then accessed as PlatformPeg.get().isElectron
that way no other classes have to implement it but is also weird, I guess its all weird with the current settings architecture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i must be missing how instanceof
works - why wouldn't it work from the react-sdk side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would I import ElectronPlatform from riot-web into react-sdk so I can refer to it in an instanceof? Am I missing something obvious :/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only by violating the layering (which already happens in a bunch of places, sadly). for now, stick with the isElectron() method.
lgtm other than query |
… t3chguy/electron_settings
ed57a37
to
7e02977
Compare
const {ipcRenderer} = require('electron'); | ||
|
||
return <div> | ||
<h3>Electron Settings</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users don't know what electron is. "Desktop-specific settings" perhaps?
lgtm other than merge conflicts and the comedy setting label problem |
(oh, and please can haz i18n) |
oh hah, I half i18n'd it |
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
… t3chguy/electron_settings # Conflicts: # src/components/structures/UserSettings.js # src/i18n/strings/en_EN.json First time using JetBrains Merge Tool, MAY HAVE GONE HORRIBLY WRONG Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
lgtm |
added
.isElectron()
to make the detection of electron more readable