-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
Merge ElectronDisplayType
and ElectronDisplayConfig
?
#21
Comments
In general I like it, but I think it would be nice to still be able to change the global config, right? But we could just make the default So for example change this line to: Base.Multimedia.pushdisplay(ElectronDisplayType(CONFIG )) etc? Then all the code that access the config could entirely avoid the global variable, but one could still use the existing API to set configs globally? |
Yes, I think it makes sense to keep Base.@kwdef mutable struct ElectronDisplayType <: Base.AbstractDisplay
showable = electron_showable
single_window::Bool = false
focus::Bool = true
end
const CONFIG = ElectronDisplayType()
function __init__()
Base.Multimedia.pushdisplay(CONFIG)
end or struct ElectronDisplayType <: Base.AbstractDisplay
config::ElectronDisplayConfig
end
ElectronDisplayType() = ElectronDisplayType(CONFIG)
function __init__()
Base.Multimedia.pushdisplay(ElectronDisplayType()) # same as what we have now
end |
Yep, I think that would work great. The second option, where we keep |
While writing #20, I started feel a bit uneasy that global state
CONFIG
is changing the code path in many locations now. It would be nice to refactor ElectronDisplay to make the core code paths reference only local data.So, how about moving configurations into
ElectronDisplayType
like thisor, alternatively, store
ElectronDisplayConfig
inElectronDisplayType
:One benefit would be that this lets us run
electrondisplay
with specific configurations only for one call without touching the global configurationElectronDisplay.CONFIG
. For example, 2-argelectrondisplay(mime, x)
can be changed to something like this:1-arg
electrondisplay(x)
is a bit more non-trivial but I think a similar idea can be applied. Another benefit is that the tests can invoke different code paths without mutatingCONFIG
.What do you think? Is it a reasonable way to refactor ElectronDisplay?
The text was updated successfully, but these errors were encountered: