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

Add `styled` and `styled-segment` builtins #674

Merged
merged 27 commits into from May 28, 2018

Conversation

Projects
None yet
3 participants
@fehnomenal
Contributor

fehnomenal commented May 16, 2018

These commits add two new builtins styled and styled-segment that are modelled after @xiaq's design in #520. It allows nesting, querying for style attributes and defining custom style transformers.

Currently the new functions are not used anywhere. The problem is that ui.Styled and ui.Styles are used to render the editor (completion matching, history, ...). This is done for the individual parts of the UI.
Probably there should be something like a renderer interface that is implemented by shell.Shell and web.Web and allows rendering of more abstract things like a selection list, notifications and so on.

This is probably a rather large refactoring that needs to be coordinated and unfortunately I'm very short on time at the moment. So I decided to publish at least my current progress.

@fehnomenal

This comment has been minimized.

Contributor

fehnomenal commented May 16, 2018

With the last commit $edit:styled~ now calls the new function. It seems to work but I haven't tested it intensively.

@xiaq

This comment has been minimized.

Member

xiaq commented May 16, 2018

Hi! Thank you so much for your contribution. Really impressed by the progress you brought.

I will take a look later this week - a bit occupied lately.

@xiaq

This comment has been minimized.

Member

xiaq commented May 22, 2018

Hi - I haven't been able to look at this last week. I will definitely (TM) take a look this week!

@xiaq

Thanks again for your contribution! Please see review comments.

You said that you wouldn't be able to work on this for a while. Please tell me if that is still the case; if so, it makes most sense for me to merge this into a feature branch, and either you or I can finish the work here.

@@ -1,30 +1,28 @@
package edit

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

Remove edit:styled in favor of builtin:styled you introduced.

I realized that builtin:styled's API is not completely backward-compatible with the old edit:styled. This is OK; Elvish is not maintaining API compatibility at the moment.

// from the supplied options applied to it. If the input is already a Segment its
// attributes are copied and modified.
func styledSegment(options RawOptions, input interface{}) (*styled.Segment, error) {
switch input := input.(type) {

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

There is a lot of common code in the two branches. In fact, they only differ near the end.

The current way does have the advantage of doing the type check early though. Probably you can do an explicit type check first, like:

switch input.(type) {
case string, styled.Segment:
default:
    return nil, errStyledSegmentArgType
}
...
}, nil
}
return nil, errors.New("expected string or styled segment")

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

Declare an error in the global scope and use it:

var errStyledSegmentArgType = errors.New("argument to styled-segment must be string or styled segment")
switch transformedSegment := vs[0].(type) {
case styled.Segment:
text[i] = transformedSegment
case *styled.Segment:

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

Is this branch possible at all?

Having to handle both styled.Segment and *styled.Segment is a code smell; it means that the code is not using one type exclusively.

This comment has been minimized.

@fehnomenal

fehnomenal May 25, 2018

Contributor

This applied to styledSegment as well and was needed because sometimes they would get a pointer and sometimes not. The error was that I was passing the non-pointer segment to the user supplied function. It is gone in the refactored code.

"bg-white": "107",
}
func TestCompatibility(t *testing.T) {

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

This test tests builtin transformers extensively, but doesn't test for other behaviors:

  • Construction of styled segments (either by supplying a string and styles, or an existing styled segment and styles);
  • Field access of styled segments;
  • Applying multiple transformers with styled;
  • Using literal function as transformers with styled.

I don't think we necessarily need to test every builtin transformer: testing one from each category (text style, foreground, background) should be sufficient. This way you can write out the tests manually; which is also desirable because you now avoid having logics in tests.

Note that I changed the test framework API slightly - I renamed runTests to Test and made it take test cases as variadic arguments. Your test will now look like:

Test(t,
  That("print (styled abc bold)").Prints(...),
  That("print (styled abc red)").Prints(...)
)
import "fmt"
type Color uint

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

Type safety and memory compactness aside, it seems to be much more straightforward if we just use string to represent colors and translate them to SGR codes when printing.

It will also be easier to extend more complex color representations (e.g. hex #123456) if we just use string.

)
type TextStyle struct {
bold *bool

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

Is nil (lack of an attribute) and false (explicit false) a useful distinction? I think not - just make the fields bool and the code can be simplified quite a bit.

This comment has been minimized.

@fehnomenal

fehnomenal May 23, 2018

Contributor

I think you're right. Honestly I don't remember my reasoning from then. Probably for easier merging...

But I think we need transformers for styled to undo bolding, diming, ... Would you accept to call them un-...?

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

You are right. no- is probably a better prefix.

transformers := make(map[interface{}]interface{})
for k, v := range styled.SegmentTransformers {
transformers[k] = NewBuiltinFn("style-transform "+k, v)

This comment has been minimized.

@xiaq

xiaq May 23, 2018

Member

These builtins are not actually accessible from the builtin: namespace. Give them more funny names, like <style transformer xxx>.

Speaking of which, I thought a bit more about the API, and I think that exposing transformers as a writable map is likely a bad idea: this is essentially a global variable, and it is hard to coordinate access from different modules written by different people. For instance, one module may install a transformer named foo and use it later, while another may install a transformer with the same name foo. One of the two modules will use a transformer that it does not intend to use.

My amended proposal is to not expose this transformer map at all and keep it strictly internal. This part of the code can be removed.

In fact, after removing the transformer map, the builtin transformers do not even need to live in a map; the styled command can also parse the transformer programmatically.

@fehnomenal

This comment has been minimized.

Contributor

fehnomenal commented May 23, 2018

Thank you for your comments. I will have a more in depth look at them this evening.
My comment about the lack of time was targeted at a greater refactoring (w.r.t. having a framework that handles rendering differently for the shell and the web frontend) but I want to finish these changes.

Thinking about this: Do we really need to distinguish these two parts?
I understand the web frontend is/will be more flexible to display hints, errors, completions, ... But in the end it would also display the output of external programs which expect to print to a shell and thus most probably use ANSI sequences themselves.
These sequences can only be displayed properly by the web frontend if the output is parsed and enriched with css classes.
This means we need this parsing anyway and the web frontend can parse our styles (that we are outputting, not external programs) as well.

@xiaq

This comment has been minimized.

Member

xiaq commented May 23, 2018

Re. distinguishing terminal and HTML/CSS outputs: you are right that for a web interface to work we will need to convert ANSI sequences to HTML/CSS anyway, so it makes sense to just implementation styled -> ANSI conversion and let ANSI -> HTML/CSS do the rest. But I see some disadvantages to this approach.

  1. The range of possible styling supported by HTML/CSS is considerably larger than that of ANSI sequences. We will want styled to be able to represent them, losslessly. The conversion path of styled -> ANSI -> HTML/CSS will be lossy.

  2. If you consider the problem of ANSI -> HTML/CSS conversion, you will first need to parse ANSI into some internal representation - and it makes perfect sense to used styled as that representation! Now that we have a styled -> HTML/CSS converter, all you need to do now is to parse ANSI into styled.

@fehnomenal fehnomenal force-pushed the fehnomenal:styled branch from 5990fb3 to ea5beb1 May 25, 2018

@fehnomenal fehnomenal force-pushed the fehnomenal:styled branch from 8884b9c to 815181a May 25, 2018

@fehnomenal

This comment has been minimized.

Contributor

fehnomenal commented May 25, 2018

OK, I rebased to the latest master and adressed all of your comments.
Available transformers are:

  • (bg-)?<color>: Sets the foreground or background color (if color is valid according to styled.IsValidColorString)
  • (no-)?(bold|dim|italic|underlined|blink): Sets or unsets the respective attribute
  • inverse: Toggles the inverse attribute
  • force-(no-)?inverse: Sets or unsets the inverse attribute

Probably there could be even more tests.

@xiaq

Thank you! And sorry for the late response again.

I have some relatively minor comments, which hopefully is easy to address.

Another general comment: I feel that using unmarked names to mean "toggle" is a little too subtle; I would rather bold to always set text to bold, and have a toggle-bold that toggles boldness. But I am not requiring you to address that in this PR.

}
func (s *Style) ImportFromOptions(options map[string]interface{}) error {
assignColor := func(key string, assign func(string)) error {

This comment has been minimized.

@xiaq

xiaq May 27, 2018

Member

The second argument can be a *string.

This comment has been minimized.

@fehnomenal

fehnomenal May 28, 2018

Contributor

I knew there has to be a non awkward way to do this. Thx!

return nil
}
func IsValidColorString(col string) bool {

This comment has been minimized.

@xiaq

xiaq May 27, 2018

Member

This does not need to be exported. It is also probably better called isValidColorName.

I am a little torn about how you are encoding a string set using a switch statement. It is indeed the most concise way though; let's keep it as is for now.

This comment has been minimized.

@fehnomenal

fehnomenal May 28, 2018

Contributor

You're absolutely right on both points...

}
return nil
}
assignBool := func(key string, assign func(bool)) error {

This comment has been minimized.

@xiaq

xiaq May 27, 2018

Member

Idem.

"github.com/elves/elvish/eval/vals"
)
// A styled Segment is a string that has some style applied to it.

This comment has been minimized.

@xiaq

xiaq May 27, 2018

Member

Docstrings for exported symbols should start with the symbol itself (https://golang.org/doc/effective_go.html#commentary). In this case, simply remove "A styled", which is apparent from the package it is in.

This comment has been minimized.

@fehnomenal

fehnomenal May 28, 2018

Contributor

Alright I will rework and add missing docstrings.

func (Segment) Kind() string { return "styled-segment" }
// Returns the representation of this Segment. The string can be used to construct

This comment has been minimized.

@xiaq

xiaq May 27, 2018

Member

Start the docstring with Repr, so it reads "Repr returns ...". Same for docstrings below.

import (
"fmt"
)

This comment has been minimized.

@xiaq

xiaq May 27, 2018

Member

Add docstring.

Generally, exported symbols should have docstrings, the only exception being docstrings are optional for methods implementing interface.

Blink bool
Inverse bool
}

This comment has been minimized.

@xiaq

xiaq May 27, 2018

Member

This can be a variable. "DefaultStyle" is a better name; "empty" is normally understood to be StructName{} in Go.

Speaking of which, it probably makes sense to use the empty string for default colors; this makes Style{} a valid value, which is considered good practice in Go. But I am not requiring you to do this in this PR.

This comment has been minimized.

@fehnomenal

fehnomenal May 28, 2018

Contributor

I also thought about defining empty strings to be the default color but wasn't sure how to handle the style transformer names. They should still be called default and bg-default but I had the thought that it could be inconsistent...

Should the representation of segments contain the default color?

@fehnomenal

This comment has been minimized.

Contributor

fehnomenal commented May 28, 2018

Again I will work on this this evening after work. While doing this I could also add more (and better named) transformers.

I will make them uniform so that for each attribute there will be setting, unsetting and toggling.

@fehnomenal

This comment has been minimized.

Contributor

fehnomenal commented May 28, 2018

Done.

fehnomenal added some commits May 28, 2018

Simplify styled transformers
Now there are three transformers for each boolean style attribute that
allow setting, unsetting and toggling the corresponding attribute.

@xiaq xiaq merged commit e89fe48 into elves:master May 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@xiaq

This comment has been minimized.

Member

xiaq commented May 28, 2018

Marvelous. Thank you!

@zzamboni

This comment has been minimized.

Contributor

zzamboni commented May 28, 2018

@xiaq @fehnomenal this is an awesome change! I look forward to exploring all the new capabilities.

If I may suggest, removing edit:styled altogether is a bit sudden in my opinion. I normally don't mind backward-incompatible changes, but would it be possible to add edit:styled as an alias of the new styled for some time? Just to give us a chance to update all those modules which use it... :)

@fehnomenal fehnomenal deleted the fehnomenal:styled branch May 29, 2018

zzamboni added a commit to zzamboni/elvish-themes that referenced this pull request May 30, 2018

Support new styled function
Initial transformation to work with the new styled
function (elves/elvish#674)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment