From cebdffb20517947bd6dadb040ffe80f00d5bfcb2 Mon Sep 17 00:00:00 2001
From: Kanit Wongsuphasawat <kanitw@gmail.com>
Date: Sun, 14 Jan 2018 14:55:28 -0800
Subject: [PATCH] Fix rule mark to support arbitrary (possibly oblique) line
 segments

Fix #3263
---
 src/compile/mark/init.ts       |  8 ++++++-
 src/compile/mark/rule.ts       |  7 +++++-
 test/compile/mark/init.test.ts | 26 +++++++++++++++++++++-
 test/compile/mark/rule.test.ts | 40 +++++++++++++++++++++++++++++++++-
 4 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/src/compile/mark/init.ts b/src/compile/mark/init.ts
index d9716ad5fd..e4fd9c6509 100644
--- a/src/compile/mark/init.ts
+++ b/src/compile/mark/init.ts
@@ -65,6 +65,12 @@ function orient(mark: Mark, encoding: Encoding<string>, specifiedOrient: Orient)
 
   switch (mark) {
     case RULE:
+      // return undefined for line segment rule
+      if (xIsRange && yIsRange) {
+        return undefined;
+      }
+      /* tslint:disable */
+      // intentional fall through
     case BAR:
     case AREA:
       // If there are range for both x and y, y (vertical) has higher precedence.
@@ -80,7 +86,7 @@ function orient(mark: Mark, encoding: Encoding<string>, specifiedOrient: Orient)
         }
       }
 
-      /* tslint:disable */
+
     case LINE: // intentional fall through
     case TICK: // Tick is opposite to bar, line, area and never have ranged mark.
 
diff --git a/src/compile/mark/rule.ts b/src/compile/mark/rule.ts
index d8e88d4c86..bc728d4816 100644
--- a/src/compile/mark/rule.ts
+++ b/src/compile/mark/rule.ts
@@ -19,7 +19,12 @@ export const rule: MarkCompiler = {
       ...mixins.markDefProperties(model.markDef, true),
       ...mixins.pointPosition('x', model, orient === 'horizontal' ? 'zeroOrMin' : ref.mid(width)),
       ...mixins.pointPosition('y', model, orient === 'vertical' ? 'zeroOrMin' : ref.mid(height)),
-      ...mixins.pointPosition2(model, 'zeroOrMax'),
+
+      // include x2 for horizontal or line segment rule
+      ...(orient !== 'vertical' ? mixins.pointPosition2(model, 'zeroOrMax', 'x2') : {}),
+
+      // include y2 for vertical or line segment rule
+      ...(orient !== 'horizontal' ? mixins.pointPosition2(model, 'zeroOrMax', 'y2') : {}),
 
       ...mixins.color(model),
       ...mixins.text(model, 'tooltip'),
diff --git a/test/compile/mark/init.test.ts b/test/compile/mark/init.test.ts
index c518dae213..f663729bdc 100644
--- a/test/compile/mark/init.test.ts
+++ b/test/compile/mark/init.test.ts
@@ -196,7 +196,7 @@ describe('compile/mark/init', function() {
       assert.equal(model.markDef.orient, 'vertical');
     });
 
-    it('should return correct orient for horizontal rule', function() {
+    it('should return correct orient for horizontal rule', function () {
       const model = parseUnitModelWithScaleAndLayoutSize({
         "mark": "rule",
         "encoding": {
@@ -206,6 +206,30 @@ describe('compile/mark/init', function() {
       assert.equal(model.markDef.orient, 'horizontal');
     });
 
+    it('should return undefined for line segment rule', function () {
+      const model = parseUnitModelWithScaleAndLayoutSize({
+        "mark": "rule",
+        "encoding": {
+          "y": {"value": 0},
+          "x": {"value": 0},
+          "y2": {"value": 100},
+          "x2": {"value": 100},
+        },
+      });
+      assert.equal(model.markDef.orient, undefined);
+    });
+
+    it('should return undefined for line segment rule with only x and y without x2, y2', function () {
+      const model = parseUnitModelWithScaleAndLayoutSize({
+        "mark": "rule",
+        "encoding": {
+          "y": {"value": 0},
+          "x": {"value": 0}
+        },
+      });
+      assert.equal(model.markDef.orient, undefined);
+    });
+
     it('should return correct orient for horizontal rules without x2 ', function() {
       const model = parseUnitModelWithScaleAndLayoutSize({
         "mark": "rule",
diff --git a/test/compile/mark/rule.test.ts b/test/compile/mark/rule.test.ts
index f6647ca9e5..81582b1fd7 100644
--- a/test/compile/mark/rule.test.ts
+++ b/test/compile/mark/rule.test.ts
@@ -105,13 +105,51 @@ describe('Mark: Rule', function() {
 
     const props = rule.encodeEntry(model);
 
-    it('should create horizontal rules', function() {
+    it('should create horizontal rules', function () {
       assert.deepEqual(props.x, {scale: X, field: 'a'});
       assert.deepEqual(props.x2, {scale: X, field: 'a2'});
       assert.deepEqual(props.y, {scale: Y, field: 'b'});
     });
   });
 
+  describe('with x, x2, y, and y2', () => {
+    const model = parseUnitModelWithScaleAndLayoutSize({
+      "mark": "rule",
+      "encoding": {
+        "x": {"field": "a", "type": "quantitative"},
+        "x2": {"field": "a2", "type": "quantitative"},
+        "y": {"field": "b", "type": "quantitative"},
+        "y2": {"field": "b2", "type": "quantitative"}
+      }
+    });
+
+    const props = rule.encodeEntry(model);
+
+    it('should create oblique rules', function () {
+      assert.deepEqual(props.x, {scale: X, field: 'a'});
+      assert.deepEqual(props.x2, {scale: X, field: 'a2'});
+      assert.deepEqual(props.y, {scale: Y, field: 'b'});
+      assert.deepEqual(props.y2, {scale: Y, field: 'b2'});
+    });
+  });
+
+  describe('with x and y', () => {
+    const model = parseUnitModelWithScaleAndLayoutSize({
+      "mark": "rule",
+      "encoding": {
+        "x": {"field": "a", "type": "quantitative"},
+        "y": {"field": "b", "type": "quantitative"}
+      }
+    });
+
+    const props = rule.encodeEntry(model);
+
+    it('should create oblique rules', function () {
+      assert.deepEqual(props.x, {scale: X, field: 'a'});
+      assert.deepEqual(props.y, {scale: Y, field: 'b'});
+    });
+  });
+
   describe('with y, y2, and x', () => {
     const model = parseUnitModelWithScaleAndLayoutSize({
       "mark": "rule",