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.

Sign up for GitHub

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

Jump to bottom

Upgrade TTY::Cursor and add top level spinner option #15

Merged

piotrmurach merged 1 commit into piotrmurach:master from austb:mac_terminal_fix

Aug 2, 2017

Contributor

austb commented Jul 28, 2017

Terminal.app does not respect ANSI cursor save and restore sequences so
instead this commit moves the cursor explicitly with up and down
commands to ensure compatibility.

I was initially put on this track by this Github issue

I added this example to test

require 'tty-spinner'

print "hello\n"

sleep 1

print TTY::Cursor.save
print TTY::Cursor.prev_line

sleep 1

print "Hello world"
print TTY::Cursor.restore

sleep 1

The above example in tmux (on Terminal.app)

The above example in Terminal.app directly

coveralls commented Jul 28, 2017 •

edited

Coverage remained the same at 96.537% when pulling a8a0fbf on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

coveralls commented Jul 31, 2017 •

edited

Coverage remained the same at 96.537% when pulling 083935b on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

houndci-bot reviewed

View reviewed changes

lib/tty/spinner/multi.rb Outdated

@@ @@ -116,6 +141,7 @@ def success @@
                     #
                     # @api public
                     def error
+                      @master_spinner.error if @master_spinner

houndci-bot Aug 1, 2017

Use safe navigation (&.) instead of checking if an object exists before calling the method.

lib/tty/spinner/multi.rb Outdated

@@ @@ -108,6 +132,7 @@ def stop @@
                     #
                     # @api public
                     def success
+                      @master_spinner.success if @master_spinner

houndci-bot Aug 1, 2017

Use safe navigation (&.) instead of checking if an object exists before calling the method.

Contributor Author

austb Aug 1, 2017

Only added in ruby 2.3.0

lib/tty/spinner/multi.rb Outdated

@@ @@ -64,11 +76,23 @@ def count_line_offset(index) @@
                         if spinner.spinning? || spinner.success? ||
                            spinner.error? || spinner.done?
                           acc += 1
+                        else

houndci-bot Aug 1, 2017

Redundant else-clause.

examples/multi_with_inset.rb Outdated

		sp3.error

		spinners.error

houndci-bot Aug 1, 2017

1 trailing blank lines detected.

examples/multi_with_inset.rb

		@@ -0,0 +1,27 @@
		# encoding: utf-8

		require 'tty-spinner'

houndci-bot Aug 1, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

examples/multi_with_inset.rb

		@@ -0,0 +1,27 @@
		# encoding: utf-8

houndci-bot Aug 1, 2017

Missing magic comment # frozen_string_literal: true.

Contributor Author

austb commented Aug 1, 2017 •

edited

So I also added a commit that adds an option for a top level spinner and then printing the spinners with an "insets". You can run examples/multi_with_inset.rb to see an example.

We had the idea of maybe going for this in the end.

coveralls commented Aug 1, 2017 •

edited

Coverage decreased (-4.4%) to 92.149% when pulling aa15b61 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

1 similar comment

coveralls commented Aug 1, 2017

Coverage decreased (-4.4%) to 92.149% when pulling aa15b61 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

austb force-pushed the mac_terminal_fix branch from aa15b61 to 4d3faa2 Compare

August 1, 2017 22:16

coveralls commented Aug 1, 2017 •

edited

Coverage decreased (-0.7%) to 95.868% when pulling 4d3faa2 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

austb force-pushed the mac_terminal_fix branch from 4d3faa2 to 3f4c04b Compare

August 1, 2017 22:49

coveralls commented Aug 1, 2017 •

edited

Coverage decreased (-0.7%) to 95.868% when pulling 3f4c04b on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

austb force-pushed the mac_terminal_fix branch from 3f4c04b to 8446001 Compare

August 1, 2017 22:58

coveralls commented Aug 1, 2017 •

edited

Coverage decreased (-0.7%) to 95.868% when pulling 8446001 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.


          Upgrade TTY::Cursor for #save and #restore

ab8ffb8

austb force-pushed the mac_terminal_fix branch from 8446001 to ab8ffb8 Compare

August 2, 2017 16:19

houndci-bot reviewed

View reviewed changes

spec/unit/multi/top_level_spinner_spec.rb


		expect { spinners.auto_spin }.not_to raise_exception
		end

houndci-bot Aug 2, 2017

Extra empty line detected at block body end.

spec/unit/multi/top_level_spinner_spec.rb

+                end
+                it "doesn't raise exception" do
+                  spinners = TTY::Spinner::Multi.new(output: output, message: "Top level spinner")

houndci-bot Aug 2, 2017

Line is too long. [84/80]

spec/unit/multi/top_level_spinner_spec.rb

+              RSpec.describe TTY::Spinner::Multi, '#auto_spin' do
+                let(:output) { StringIO.new('', 'w+') }
+                it "raises and exception when #auto_spin is called without a top level spinner" do

houndci-bot Aug 2, 2017

Line is too long. [84/80]

spec/unit/multi/top_level_spinner_spec.rb

+              end
+              RSpec.describe TTY::Spinner::Multi, '#auto_spin' do
+                let(:output) { StringIO.new('', 'w+') }

houndci-bot Aug 2, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

spec/unit/multi/top_level_spinner_spec.rb

+                end
+              end
+              RSpec.describe TTY::Spinner::Multi, '#auto_spin' do

houndci-bot Aug 2, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

spec/unit/multi/top_level_spinner_spec.rb


		spinner = spinners.register ""

		expect(spinners.line_inset(spinner)).to eq('')

houndci-bot Aug 2, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

spec/unit/multi/top_level_spinner_spec.rb

+              #
+              RSpec.describe TTY::Spinner::Multi, '#line_inset' do
+                let(:output) { StringIO.new('', 'w+') }

houndci-bot Aug 2, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

spec/unit/multi/top_level_spinner_spec.rb

+              # coding: utf-8
+              #
+              RSpec.describe TTY::Spinner::Multi, '#line_inset' do

houndci-bot Aug 2, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

spec/unit/multi/top_level_spinner_spec.rb

		@@ -0,0 +1,57 @@
		# coding: utf-8
		#

houndci-bot Aug 2, 2017

Add an empty line after magic comments.

spec/unit/multi/top_level_spinner_spec.rb

		@@ -0,0 +1,57 @@
		# coding: utf-8

houndci-bot Aug 2, 2017

Missing magic comment # frozen_string_literal: true.

coveralls commented Aug 2, 2017 •

edited

Coverage increased (+0.2%) to 96.748% when pulling ab8ffb8 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

4 similar comments

coveralls commented Aug 2, 2017

Coverage increased (+0.2%) to 96.748% when pulling ab8ffb8 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

coveralls commented Aug 2, 2017

Coverage increased (+0.2%) to 96.748% when pulling ab8ffb8 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

coveralls commented Aug 2, 2017

Coverage increased (+0.2%) to 96.748% when pulling ab8ffb8 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

coveralls commented Aug 2, 2017

Coverage increased (+0.2%) to 96.748% when pulling ab8ffb8 on austb:mac_terminal_fix into dcf138d on piotrmurach:master.

austb changed the title ~~Move TTY::Cursor up and down explicitly~~ Upgrade TTY::Cursor and add top level spinner option

Contributor Author

austb commented Aug 2, 2017

@piotrmurach this is ready to go post tty-cursor release yesterday.

Owner

piotrmurach commented Aug 2, 2017

@austb this is great addition! Thanks for working on it.

Going forward, before next release of the library, I think it would be good to first raise some GitHub issues so that we can agree on what we're working on etc... I try to maintain consistency between apis of tty packages and document everything as I'm going along developing library and GitHub issues would definitely help. Therefore it would be good to:

agree on multi spinner apis and use cases, as much as I appreciate your use case and want this library to be super useful to you and your project, I think we can also account for some generic scenarios on how people may want to call let's say few spinners that download files etc...
add docs to readme explaining how people may want to use multi spinners
discuss any future features that you may wish to add before release, basically i want to be clear what we are aiming for.

Again, thank you for giving tty-spinner such great ideas!

piotrmurach merged commit 6078684 into piotrmurach:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment