Add cursor movement to IO.ANSI #7396

mveytsman · 2018-02-26T05:17:36Z

I added the cursor movement escape codes (from https://www.tldp.org/HOWTO/Bash-Prompt-HOWTO/x361.html) to IO.ANSI

eksperimental

Shouldnt we be consistent and name the function "cursor_forth"?

eksperimental · 2018-02-26T14:00:51Z

Or use left and right instead?

eksperimental · 2018-02-26T14:01:35Z

Or backward / forward

mveytsman · 2018-02-26T15:29:02Z

Good catch @eksperimental.

According to http://ascii-table.com/ansi-escape-sequences-vt-100.php what I called cursor_forward is CUF and what I called cursor_back (oops!) is CUB.

In ECMA-48 CUF is called "CURSOR RIGHT" and CUB is "CURSOR LEFT", so calling the functions cursor_left and cursor_right would be more consistent

eksperimental · 2018-02-26T15:33:42Z

Then I'm all for up, down, right, left
Thank you

eksperimental · 2018-02-26T17:07:47Z

lib/elixir/lib/io/ansi.ex

+  @spec cursor(integer, integer) :: String.t()
+  def cursor(line, col), do: "\e[#{line};#{col}H"
+
+  @doc "Sends cursor up."


I think we need to mention the argument in the docs.
Sends cursor lines up

eksperimental · 2018-02-26T17:08:28Z

lib/elixir/lib/io/ansi.ex

+  @spec cursor_up(integer) :: String.t()
+  def cursor_up(lines), do: "\e[#{lines}A"
+
+  @doc "Sends cursor down."


Sends cursor lines down.

eksperimental · 2018-02-26T17:11:43Z

lib/elixir/lib/io/ansi.ex

+
+  @doc "Sends cursor left."
+  @spec cursor_left(integer) :: String.t()
+  def cursor_left(cols), do: "\e[#{cols}C"


Personally i dont like abbreviations in arguments. I would go with "columns"

They read better (and more natural) when you use them in the docs

eksperimental · 2018-02-26T17:12:34Z

lib/elixir/lib/io/ansi.ex

+  @doc "Sends cursor left."
+  @spec cursor_left(integer) :: String.t()
+  def cursor_left(cols), do: "\e[#{cols}C"
+


Sends cursor columns to the left.

eksperimental · 2018-02-26T17:13:32Z

lib/elixir/lib/io/ansi.ex

+  @spec cursor_left(integer) :: String.t()
+  def cursor_left(cols), do: "\e[#{cols}C"
+
+  @doc "Sends cursor right."


Sends cursor columns to the right.

eksperimental · 2018-02-26T17:17:32Z

Additionaly we could add guard checks to the function integer arguments. And what about zero and negative integers.. whats the behaviour.

eksperimental · 2018-02-26T17:19:48Z

cursor uses an absolute or a relative position? I think it should be documented

fertapric · 2018-02-26T17:33:29Z

What do you think about using 1 as default value for all the functions?

IO.ANSI.cursor_up()
IO.ANSI.cursor_down()
IO.ANSI.cursor_left()
IO.ANSI.cursor_right()

IO.ANSI.cursor_up(2)
IO.ANSI.cursor_down(3)
IO.ANSI.cursor_left(4)
IO.ANSI.cursor_right(5)

eksperimental · 2018-02-26T17:35:19Z

@fertapric it makes total sense

mveytsman · 2018-02-26T17:43:55Z

ANSI will actually default to 1 for us if we don't pass an argument, i.e. "\e[A" moves 1 line up. I think that's the correct behavior with no argument for us as well.

mveytsman · 2018-02-26T17:51:01Z

If you send the ANSI code for moving the cursor absolutely without any arguments it defaults to 0,0, but we already have this in IO.ANSI.home/0 so I think it's okay to have IO.ANSI.cursor/2 require 2 arguments.

fertapric · 2018-02-26T17:53:15Z

lib/elixir/lib/io/ansi.ex

@@ -170,6 +170,30 @@ defmodule IO.ANSI do
  @doc "Sends cursor home."
  defsequence(:home, "", "H")

+  @doc "Sends cursor to the absolute position specified by `lines`, `columns`, with 0,0 being the top left corner."


I would split this line:

@doc """ Sends cursor to the absolute position specified by `lines`, `columns`, with 0,0 being the top left corner. """

Btw, it seems the formatter leaves that @doc tag as is. Should the formatter try to convert @doc "..." into @doc """ (new line) ... """? Or at least emit a message?

fertapric · 2018-02-26T17:56:47Z

lib/elixir/test/elixir/io/ansi_test.exs

+    end
+  end
+
+  test "cursor_up/1" do


This is also testing cursor_up/0 :)

😁 so it is

fertapric · 2018-02-26T18:02:41Z

lib/elixir/lib/io/ansi.ex

+
+  @doc "Sends cursor `lines` up (defaults to 1)."
+  @spec cursor_up(integer) :: String.t()
+  def cursor_up(), do: "\e[A"


I think the @spec will be assigned to cursor_up/0 and the specification won't be accurate. I see two options:

# Option 1 @spec cursor_up :: String.t() def cursor_up, do: "\e[A" @spec cursor_up(integer) :: String.t() def cursor_up(lines) when lines > 0, do: "\e[#{lines}A" # Option 2 @spec cursor_up(integer) :: String.t() def cursor_up(lines \\ 1) def cursor_up(1), do: "\e[A" def cursor_up(lines) when lines > 0, do: "\e[#{lines}A"

I would go for "Option 2" (because it's what the documentation says) :)

I feel like option 1 is more faithful to the underlying implementation which might matter in the future since terminal emulators can behave in strange ways.

fertapric · 2018-02-26T18:25:15Z

lib/elixir/lib/io/ansi.ex

+  def cursor_up(), do: "\e[A"
+
+  @spec cursor_up(integer) :: String.t()
+  def cursor_up(lines) when lines > 0, do: "\e[#{lines}A"


Last thing, promise 😅

The docs are now assigned to cursor_up/0, leaving cursor_up/1 without any documentation.

Actually thanks for all this feedback, as you can tell it's been a while and my elixir is a little rusty :)

is there a normal way to solve this problem? Or are you expected just to duplicate docs for multi-arity functions?

Multi-arity functions usually behave somehow different, so their documentation is usually not the same. There are some example in the Elixir codebase, Path.absname/1 and Path.absname/2 could be one of them.

whatyouhide · 2018-02-26T23:56:41Z

ANSI will actually default to 1 for us if we don't pass an argument, i.e. "\e[A" moves 1 line up. I think that's the correct behavior with no argument for us as well.

@mveytsman is there any practical advantage in using \e[A instead of \e1[A, or any difference in general? If there isn't and it's just a "standard" think, personally I'd rather go with cursor_up(columns \\ 1) so the code and documentation are a bit more compact.

mveytsman · 2018-02-27T00:04:22Z

@whatyouhide that's a fair point. Looking around, I don't see "\e[A" documented very much other than in the standard, so moving to \\ 1 makes sense.

My thinking was that someone may be dealing with a "weird" terminal and want to send the "\e[A" directly but in that case they can just do it themselves...

(saving a byte is also definitely not worth uglier code 😬 )

whatyouhide · 2018-02-27T00:09:01Z

someone may be dealing with a "weird" terminal and want to send the "\e[A"

Wouldn't the terminal be technically not working if \e[A wasn't the same as \e1[A? Anyways if something like this ever happens, we'll deal with it when the bug report comes in :)

whatyouhide · 2018-02-27T00:45:11Z

lib/elixir/lib/io/ansi.ex

+  @spec cursor(integer, integer) :: String.t()
+  def cursor(line, column) when line >= 0 and column >= 0, do: "\e[#{line};#{column}H"
+
+  @doc "Sends cursor `lines` up. Defaults to 1"


We don't need to mention the default, as \\ is standard in Elixir. Let's just have

@doc "Sends cursor `lines` lines up.

whatyouhide · 2018-02-27T00:47:25Z

lib/elixir/lib/io/ansi.ex

+  with 0,0 being the top left corner.
+  """
+  @spec cursor(integer, integer) :: String.t()
+  def cursor(line, column) when line >= 0 and column >= 0, do: "\e[#{line};#{column}H"


If you want to check for the arguments being also integers, you should explicitly have a is_integer/1 check. > and >= works for floats as well.

Please do this on all other functions as well :).

whatyouhide · 2018-02-27T00:48:16Z

lib/elixir/lib/io/ansi.ex

@@ -170,6 +170,29 @@ defmodule IO.ANSI do
  @doc "Sends cursor home."
  defsequence(:home, "", "H")

+  @doc """
+  Sends cursor to the absolute position specified by `lines`, `columns`, 
+  with 0,0 being the top left corner.


The first line of the documentation should be a summary of the function. Let's reshape this to be:

@doc """ Sends cursor to the absolute position specified by `line` and `column`. Line `0` and column `0` would mean the top left corner. """

mveytsman · 2018-02-27T01:07:50Z

lib/elixir/lib/io/ansi.ex

  """
  @spec cursor(integer, integer) :: String.t()
-  def cursor(line, column) when line >= 0 and column >= 0, do: "\e[#{line};#{column}H"
+  def cursor(line, column)
+      when is_integer(line) and line >= 0 and is_integer(column) and column >= 0,


@whatyouhide this is how mix format formatted it but it looks a little ugly. Think I should make a is_positive_integer guard / do we have one somewhere?

We don't but this formatting is fine. When when gets split on its own line, we just use the do/end syntax:

def cursor(line, column) when is_integer(line) and line >= 0 and is_integer(column) and column >= 0 do ... end

fertapric · 2018-02-27T08:10:02Z

lib/elixir/test/elixir/io/ansi_test.exs

+    assert IO.ANSI.cursor_up() == "\e[1A"
+    assert IO.ANSI.cursor_up(12) == "\e[12A"
+
+    assert_raise FunctionClauseError, fn ->


Should we also test the value 0? it seems a sensitive value because in an alternative implementation could be interpreted as \e[A or \e[0A. \e[0A works on my terminal and it's equivalent to \e[A or \e[1A.

Good catch! Wouldn't it be best to have a guard that the value is >= 1? :)

If we go that way, the typespec should use pos_integer()

I had the guard as > 0 but >= 1 is clearer. I changed that and the typespecs

eksperimental

Great work

fertapric · 2018-02-27T13:42:56Z

lib/elixir/test/elixir/io/ansi_test.exs

+    end
+  end
+
+  test "cursor_right/0" do


cursor_right/1

fertapric

Nice first commit! 🎉 🎊 💚

whatyouhide · 2018-02-28T00:44:41Z

Thanks @mveytsman! 💟

Max Veytsman and others added 3 commits February 26, 2018 00:16

Add cursor movement to IO.ANSI

a247c7d

Update ansi.ex

12bfa2c

Update ansi_test.exs

13ec4f0

josevalim approved these changes Feb 26, 2018

View reviewed changes

eksperimental reviewed Feb 26, 2018

View reviewed changes

Rename IO.ANSI cursor_back/forward to cursor_left/right

16332ab

eksperimental suggested changes Feb 26, 2018

View reviewed changes

Update docs and add defaults for IO.ANSI cursor movement

ee54a77

fertapric reviewed Feb 26, 2018

View reviewed changes

Max Veytsman added 2 commits February 26, 2018 12:56

Formatting

dc8c4d6

Name tests correctly

0b74e75

fertapric reviewed Feb 26, 2018

View reviewed changes

Fix specs

a3628b1

fertapric reviewed Feb 26, 2018

View reviewed changes

Fix docs

d4e0204

Switch to using a default value for cursor movement instead of 2 funs

4d23cb2

whatyouhide requested changes Feb 27, 2018

View reviewed changes

Add guards and tweak docs

7f9ad71

mveytsman commented Feb 27, 2018

View reviewed changes

Improve formatting on multiline function

d057335

fertapric reviewed Feb 27, 2018

View reviewed changes

Change guards & typespecs, add tests

87350ba

eksperimental approved these changes Feb 27, 2018

View reviewed changes

fertapric reviewed Feb 27, 2018

View reviewed changes

Fix typo in tests

0c60956

fertapric approved these changes Feb 27, 2018

View reviewed changes

whatyouhide approved these changes Feb 28, 2018

View reviewed changes

whatyouhide merged commit d94a9cc into elixir-lang:master Feb 28, 2018

Add cursor movement to IO.ANSI #7396

Add cursor movement to IO.ANSI #7396

Uh oh!

Conversation

mveytsman commented Feb 26, 2018

Uh oh!

eksperimental left a comment

Choose a reason for hiding this comment

Uh oh!

eksperimental commented Feb 26, 2018

Uh oh!

eksperimental commented Feb 26, 2018

Uh oh!

mveytsman commented Feb 26, 2018

Uh oh!

eksperimental commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eksperimental commented Feb 26, 2018

Uh oh!

eksperimental commented Feb 26, 2018

Uh oh!

fertapric commented Feb 26, 2018

Uh oh!

eksperimental commented Feb 26, 2018

Uh oh!

mveytsman commented Feb 26, 2018

Uh oh!

mveytsman commented Feb 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fertapric Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mveytsman Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whatyouhide commented Feb 26, 2018

Uh oh!

mveytsman commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whatyouhide commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fertapric Feb 26, 2018 •

edited

Loading

mveytsman Feb 26, 2018 •

edited

Loading

mveytsman commented Feb 27, 2018 •

edited

Loading