Merge `ElectronDisplayType` and `ElectronDisplayConfig`?

[tkf]

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 this

mutable struct ElectronDisplayType <: Base.AbstractDisplay
    showable
    single_window::Bool
    focus::Bool
end

or, alternatively, store ElectronDisplayConfig in ElectronDisplayType:

mutable struct ElectronDisplayConfig
    ...
end

struct ElectronDisplayType <: Base.AbstractDisplay
    config::ElectronDisplayConfig
end

One benefit would be that this lets us run electrondisplay with specific configurations only for one call without touching the global configuration ElectronDisplay.CONFIG. For example, 2-arg electrondisplay(mime, x) can be changed to something like this:

electrondisplay(mime, x; options...) =
    display(ElectronDisplayType(; options...), mime, x)

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 mutating CONFIG.

What do you think? Is it a reasonable way to refactor ElectronDisplay?

[davidanthoff]

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 ElectronDisplayType pick up the existing global CONFIG object, right?

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?

[tkf]

Yes, I think it makes sense to keep ElectronDisplay.CONFIG. I think we can do it without breaking the public API. Maybe something like

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

[davidanthoff]

Yep, I think that would work great. The second option, where we keep ElectronDisplayConfig seems slightly cleaner to me.