Fix(pager): Resolve background color before staring pager #650
Conversation
|
@zemzale kindly rebase the branch |
|
@profclems Will do. There seem to be some issues about this with tests anyways. So gotta fix that first |
zemzale
force-pushed
the
fix-pager-issue
branch
from
da87c2e
to
ed7eda5
Compare
Codecov Report
@@ Coverage Diff @@
## trunk #650 +/- ##
==========================================
- Coverage 60.18% 60.05% -0.13%
==========================================
Files 90 90
Lines 6414 6431 +17
==========================================
+ Hits 3860 3862 +2
- Misses 2191 2206 +15
Partials 363 363
Continue to review full report at Codecov.
|
|
Rebased and tests working |
There was a problem hiding this comment.
Thanks for working on this.
I have added a few suggestions in the inline review comments: we allow users set the glamour_style in the config so we should consider that.
So in the mr_view and issue_view, we get the glamour_style from the config and pass to the ResolveBackgroundColor(style string).
pkg/iostreams/iostreams.go
Outdated
| func (s *IOStreams) ResolveBackgroundColor() string { | ||
| style := os.Getenv("GLAMOUR_STYLE") |
There was a problem hiding this comment.
| func (s *IOStreams) ResolveBackgroundColor() string { | |
| style := os.Getenv("GLAMOUR_STYLE") | |
| func (s *IOStreams) ResolveBackgroundColor(style string) string { | |
| if(style == "") { | |
| style = os.Getenv("GLAMOUR_STYLE") | |
| } |
commands/issue/view/issue_view.go
Outdated
| @@ -93,6 +93,7 @@ func NewCmdView(f *cmdutils.Factory) *cobra.Command { | |||
| } | |||
| } | |||
|
|
|||
| f.IO.ResolveBackgroundColor() | |||
There was a problem hiding this comment.
| f.IO.ResolveBackgroundColor() | |
| f.IO.ResolveBackgroundColor(opts.GlamourStyle) |
commands/mr/view/mr_view.go
Outdated
| @@ -77,6 +77,7 @@ func NewCmdView(f *cmdutils.Factory) *cobra.Command { | |||
| } | |||
| } | |||
|
|
|||
| f.IO.ResolveBackgroundColor() | |||
There was a problem hiding this comment.
| f.IO.ResolveBackgroundColor() | |
| f.IO.ResolveBackgroundColor(glamourStyle) |
So far we have started the pager and then got background color glamour style from enviorment. If enviorment value is not set it defaults to auto. Which in turn uses termenv to resolve background color using some terminal escape code sequences. The problem with this is that doing this when a pager like less is running, just doesn't work and leaves less in a broken state. To fix this we are now checking the background color manually and before we start the pager. Issue profclems#636
zemzale
force-pushed
the
fix-pager-issue
branch
from
ed7eda5
to
be467fa
Compare
|
Fixed the issue with not config-defined styles. I also removed the |
Description
So far we have started the pager and then got background color glamour
style from enviorment.
If enviorment value is not set it defaults to auto. Which in turn uses
termenv to resolve background color using some terminal escape code
sequences. The problem with this is that doing this when a pager like
less is running, just doesn't work and leaves less in a broken state.
To fix this we are now checking the background color manually and before
we start the pager.
Related Issue
Resolves #636
How Has This Been Tested?
Fixes hangs for less on my machine.
Types of changes