Skip to content
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

Fix interactive prompt for options > terminal rows #35

Merged
merged 1 commit into from
Jun 5, 2018

Conversation

lugray
Copy link
Contributor

@lugray lugray commented May 31, 2018

Implement a scrolling list. wait_for_actionable_user_input stops re-rendering options when not necessary (midway through processing a ⬆️/⬇️ sequence) since it was causing some noticeable flicker, but also means that tests had to change more, since fewer lines are written. fmt_full_line is needed now since the lengths of lines can change, which was not true before since the only thing that changed was the marker vs space and colour. This also caused tests to change, prompting the option_text helper.

screen shot 2018-05-31 at 3 18 04 pm

screen shot 2018-05-31 at 3 18 13 pm

screen shot 2018-05-31 at 3 18 21 pm

screen shot 2018-05-31 at 3 18 31 pm

cc @Shopify/dev-infra
Fixes #34

@lugray lugray requested review from burke and lavoiesl May 31, 2018 19:24
lib/cli/ui.rb Outdated
#
# * +enable_color+ - should color be used? default to true
#
def self.fmt_full_line(input, enable_color: true)
Copy link
Member

@burke burke May 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to accomplish the same thing by writing something like:

\e[nA or \e[nB # to move to the right row
\r # to reset to col 0
<text>
\e[K # to clear to the end of the line

This would be flicker-free too.

EDIT: e.g.: puts "asdf asdf asdf asdf \n\e[A\rasdf\e[K\n"

Copy link
Member

@burke burke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM but let's use \e[K rather than padding

Copy link
Contributor

@lavoiesl lavoiesl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as expected 🎩
image

@lugray
Copy link
Contributor Author

lugray commented May 31, 2018

@burke: I did this without \n\e[A\r. Any reason you think it's wanted?

@lugray lugray force-pushed the handle_many_options branch 3 times, most recently from 1360fa1 to 98e4133 Compare May 31, 2018 23:10
@@ -157,12 +165,43 @@ def raw_tty!
end
end

def presented_options(recalculate: false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really hard to follow.
I feel like it would do well to have some comments throughout explaining the code as it's quite logic dense and sort of magic with the [-2].last etc

@lugray
Copy link
Contributor Author

lugray commented Jun 1, 2018

Refactored to hopefully improve clarity.

def presented_options(recalculate: false)
return @presented_options unless recalculate

@presented_options = @options.zip(1..Float::INFINITY)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@options.each.with_index(1) is another way if you think it's more clear. I'm undecided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change would need to_a as well (as you suggest is an emumerator which prevents the manipulations below). The zip seems clearer to me.


@presented_options = @options.zip(1..Float::INFINITY)
while num_lines > max_options
if last_visible_option_number - @active > @active - first_visible_option_number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is kind of hard to read. Maybe change to:

distance_from_selection_to_end = last_visible_option_number - @active
distance_from_start_to_selection = @active - first_visible_option_number

# try to keep the selection centered in the window:
if distance_from_selection_to_end > distance_from_start_to_selection
  # selection is closer to top than bottom, so trim a row from the bottom
  ...
else
  # closer to bottom, so trim from top

@@ -5,6 +5,7 @@ module CLI
module UI
module Terminal
DEFAULT_WIDTH = 80
DEFAULT_HEIGHT = 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typical default height is 24 fwiw

Copy link
Member

@burke burke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I still think I like my naming for the dense bit a little better but no major complaint.

@lugray lugray force-pushed the handle_many_options branch from 038d9bd to 32ef33b Compare June 5, 2018 13:04
@lugray
Copy link
Contributor Author

lugray commented Jun 5, 2018

Updated.

Copy link
Member

@burke burke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@lugray lugray merged commit 778a1d6 into master Jun 5, 2018
@lugray lugray deleted the handle_many_options branch June 5, 2018 17:42
@lugray lugray temporarily deployed to production November 9, 2018 18:39 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants