tkf commented 5 years ago

Without this patch, when ElectronDisplay is imported before REPL is initialized, it looses to REPL's display. We can prioritize ElectronDisplay over REPL by calling pushdisplay in atreplinit.

Before:

$ cat ~/.julia/config/startup.jl
using ElectronDisplay

$ julia -q
julia> typeof.(Base.Multimedia.displays)
3-element Array{DataType,1}:
 TextDisplay
 ElectronDisplay.ElectronDisplayType
 REPL.REPLDisplay{REPL.LineEditREPL}

After:

$ julia -q
julia> typeof.(Base.Multimedia.displays)
3-element Array{DataType,1}:
 TextDisplay
 REPL.REPLDisplay{REPL.LineEditREPL}
 ElectronDisplay.ElectronDisplayType

davidanthoff commented 5 years ago

Hm, that seems to break the tests? Maybe we need to make sure to only do this if we are in interactive REPL mode?

tkf commented 5 years ago

Ah, I have forgotten that using atreplinit is tricky.

In non-interactive mode, we need to call pushdisplay manually (7959a7f652a927e7887e20b371abfcebb83e8137). This is a breaking change for people using ElectronDisplay in non-interactive script. I don't think there is a good way to solve this problem. If we call pushdisplay when isinteractive() is false, this voids the very purpose of this PR since it is false inside startup.jl.

Even in interactive mode, I just noticed this PR broke the case where ElectronDisplay was imported in the middle of REPL session. This can be fixed by manually checking isdefined(Base, :active_repl) 7959a7f652a927e7887e20b371abfcebb83e8137. Ideally, we can use Base.afterreplinit https://github.com/JuliaLang/julia/pull/29896 at some point in the future...

I thought this PR was going to be a simple change but it turned out that it was a rather hairy problem. Maybe it's better to wait for the priority system https://github.com/JuliaLang/julia/pull/30922 so that it can be solved cleanly?

codecov-io commented 5 years ago

Codecov Report

Merging #24 into master will increase coverage by 0.05%. The diff coverage is 60%.

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   48.83%   48.88%   +0.05%     
==========================================
  Files           1        1              
  Lines          86       90       +4     
==========================================
+ Hits           42       44       +2     
- Misses         44       46       +2

Impacted Files	Coverage Δ
src/ElectronDisplay.jl	`48.88% <60%> (+0.05%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4ef1713...41ff4d9. Read the comment docs.

davidanthoff commented 5 years ago

Ah, bugger, yes, that is complicated. I think I agree, maybe lets wait with this until the priority system for the display stack is in place...

tkf commented 5 years ago

Sounds good. I'll close this PR for now. I hope that the priority system hits Julia master soon.

queryverse / ElectronDisplay.jl

Prioritize ElectronDisplay over REPL #24

Codecov Report