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

fehnomenal · 2018-05-16T16:31:38Z

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 · 2018-05-16T21:32:32Z

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

xiaq · 2018-05-16T23:54:19Z

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 · 2018-05-22T00:10:56Z

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.

xiaq · 2018-05-22T23:45:07Z

edit/styled.go

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



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.

xiaq · 2018-05-22T23:50:29Z

eval/builtin_fn_styled.go

+// 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) {


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 } ...

xiaq · 2018-05-22T23:51:29Z

eval/builtin_fn_styled.go

+		}, nil
+	}
+
+	return nil, errors.New("expected string or styled segment")


Declare an error in the global scope and use it:

var errStyledSegmentArgType = errors.New("argument to styled-segment must be string or styled segment")

xiaq · 2018-05-22T23:53:45Z

eval/builtin_fn_styled.go

+			switch transformedSegment := vs[0].(type) {
+			case styled.Segment:
+				text[i] = transformedSegment
+			case *styled.Segment:


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

xiaq · 2018-05-22T23:59:20Z

eval/builtin_fn_styled_test.go

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


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(...) )

xiaq · 2018-05-23T00:39:25Z

styled/colors.go

+
+import "fmt"
+
+type Color uint


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.

xiaq · 2018-05-23T00:40:51Z

styled/styled.go

+)
+
+type TextStyle struct {
+	bold       *bool


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.

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-...?

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

xiaq · 2018-05-23T00:42:25Z

eval/builtin_fn_styled.go

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


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 · 2018-05-23T08:05:22Z

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 · 2018-05-23T22:10:37Z

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.

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.
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 · 2018-05-25T03:33:04Z

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.

xiaq · 2018-05-27T21:36:12Z

styled/styled.go

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


The second argument can be a *string.

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

xiaq · 2018-05-27T21:36:19Z

styled/styled.go

+	return nil
+}
+
+func IsValidColorString(col string) bool {


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.

You're absolutely right on both points...

xiaq · 2018-05-27T21:36:27Z

styled/styled.go

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


xiaq · 2018-05-27T21:40:13Z

styled/segment.go

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


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.

Alright I will rework and add missing docstrings.

xiaq · 2018-05-27T21:40:45Z

styled/segment.go

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


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

xiaq · 2018-05-27T21:42:26Z

styled/styled.go

+import (
+	"fmt"
+)
+


Add docstring.

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

xiaq · 2018-05-27T21:44:08Z

styled/styled.go

+	Blink      bool
+	Inverse    bool
+}
+


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.

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 · 2018-05-28T08:45:20Z

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 · 2018-05-28T16:40:37Z

Done.

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

xiaq · 2018-05-28T20:24:35Z

Marvelous. Thank you!

zzamboni · 2018-05-28T20:42:35Z

@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... :)

Initial transformation to work with the new styled function (elves/elvish#674)

xiaq requested changes May 23, 2018

View reviewed changes

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

fehnomenal added 13 commits May 25, 2018 05:25

Add compatibility test with old implementation

143722f

Add color type

41ceb84

Add basic style structs and utilities

105d8df

Add structs for styled segments and texts

8c3424a

Add default style transformers to reimplement $edit:styled~

6be5ab2

Add builtins to manipulate styled segments and texts

dbd032c

Rename style 'underline' -> 'underlined'

985a94d

Fix test case

215a686

Add conversion from styled text to ansi sequences

fa29d24

Return errors rather than throwing

31f3beb

Validate the type of boolean options

a3edcb3

Delegate old to new styled function

6394756

Rebase for new test framework api and expand test cases

35ffac4

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

fehnomenal added 8 commits May 25, 2018 05:27

Remove old builtin function $edit:styled~

cc7f673

Use strings to represent colors

c67ee05

Convert bool pointers to simple bool values

7921a1c

Validate color strings

760694d

Do no longer expose builtin style transformers

7e002ed

Fix confusion about pointers

76f155b

Make outputs more stable

e3fc4b7

Expand tests

815181a

xiaq requested changes May 27, 2018

View reviewed changes

fehnomenal added 4 commits May 28, 2018 17:47

Use pointers instead of passing setter functions

949b409

Unexport and rename color check

c6dfd4e

Use the empty string for default colors

d08f9d4

Expand tests

cd28078

fehnomenal added 2 commits May 28, 2018 18:42

Simplify styled transformers

ebb4f00

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

Rework and add doc comments

518cbb6

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

fehnomenal deleted the styled branch May 29, 2018 07:28

zzamboni mentioned this pull request May 29, 2018

Make edit:styled more high-level and allow nesting #520

Closed

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

Support new styled function

3d373bc

Initial transformation to work with the new styled function (elves/elvish#674)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fehnomenal commented May 16, 2018

fehnomenal commented May 16, 2018

xiaq commented May 16, 2018

xiaq commented May 22, 2018

xiaq left a comment

xiaq May 22, 2018

xiaq May 22, 2018

xiaq May 22, 2018

xiaq May 22, 2018

fehnomenal May 25, 2018

xiaq May 22, 2018

xiaq May 23, 2018

xiaq May 23, 2018

fehnomenal May 23, 2018

xiaq May 23, 2018

xiaq May 23, 2018

fehnomenal commented May 23, 2018

xiaq commented May 23, 2018

fehnomenal commented May 25, 2018

xiaq left a comment

xiaq May 27, 2018

fehnomenal May 28, 2018

xiaq May 27, 2018

fehnomenal May 28, 2018

xiaq May 27, 2018

xiaq May 27, 2018

fehnomenal May 28, 2018

xiaq May 27, 2018

xiaq May 27, 2018

xiaq May 27, 2018

fehnomenal May 28, 2018

fehnomenal commented May 28, 2018

fehnomenal commented May 28, 2018

xiaq commented May 28, 2018

zzamboni commented May 28, 2018


		func (Segment) Kind() string { return "styled-segment" }

		// Returns the representation of this Segment. The string can be used to construct

Add styled and styled-segment builtins #674

Add styled and styled-segment builtins #674

Conversation

fehnomenal commented May 16, 2018

fehnomenal commented May 16, 2018

xiaq commented May 16, 2018

xiaq commented May 22, 2018

xiaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fehnomenal commented May 23, 2018

xiaq commented May 23, 2018

fehnomenal commented May 25, 2018

xiaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fehnomenal commented May 28, 2018

fehnomenal commented May 28, 2018

xiaq commented May 28, 2018

zzamboni commented May 28, 2018

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

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