Added a terminal bell if interaction from the user is required, according to this issue: https://github.com/beetbox/beets/issues/2366 #2495

SpirosChadoulos · 2017-03-26T19:58:35Z

mention-bot · 2017-03-26T19:58:37Z

@SpirosChadoulos, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @pkess and @geigerzaehler to be potential reviewers.

sampsyo

Thanks for getting this started, and congratulations on your first beets PR!

Even though this is a small change, I have a couple of requests:

This method isn't quite the right place to emit the bell. This method just generates the list of choices to emit; it doesn't actually display the prompt. The bell should definitely be printed right next to the code that actually emits the prompt. Namely, this happens with a call to ui.input_options in the choose_candidate function in the same file.
Please put this new functionality behind a configuration option. (I, personally, don't want a bell ringing on every prompt.)

sampsyo · 2017-03-26T20:42:39Z

beets/ui/commands.py

@@ -823,6 +823,8 @@ def _get_choices(self, task):
        Returns a list of `PromptChoice`s.
        """
        # Standard, built-in choices.
+        #Bell ring to alert the user that he has to interfere with the cmd


This Travis report: https://travis-ci.org/beetbox/beets/jobs/215295136#L885

is telling you to put a space after the # character in your comments. In addition, please use complete sentences in comments that end with periods—and there's no need to use abbreviations like "cmd".

Okay I'll work on it. Thank you.

SpirosChadoulos · 2017-03-26T22:24:27Z

I put the command in the right place and I fixed my comment. Could you give me some instractions on how to put this new functionality behind a configuration option?

sampsyo · 2017-03-26T22:27:22Z

Sure! You might try looking around how other boolean operations are implemented. For example, this conditional implements the import.quiet config option. You'll want to add something like:

if config['import']['bell']:
    ...

and then add a default value to config_default.yaml. You'll also need to add an entry to the documentation, in config.rst.

…'s configuration.

SpirosChadoulos · 2017-03-27T20:36:17Z

@sampsyo I made tha changes you requested..Is my branch ready to be marged to the main beets repo?

sampsyo

Looks great! I requested a few small changes above. After that, we'll also need an entry in the changelog describing what's new. With all that in place, this will be ready to merge! ✨

sampsyo · 2017-03-27T21:37:20Z

beets/config_default.yaml

@@ -25,6 +25,7 @@ import:
    pretend: false
    search_ids: []
    duplicate_action: ask
+    bell: yes  


Let's have this default to "no" (to avoid surprising people who upgrade).

sampsyo · 2017-03-27T21:38:00Z

docs/reference/config.rst

+bell
+~~~~~~~~~~~~~~~~
+
+Whether the importer ui should ring a bell when user interaction is needed.


Suggested simpler wording:

Ring the terminal bell to get your attention when the importer needs your input.

sampsyo · 2017-03-27T21:38:11Z

docs/reference/config.rst

+
+Whether the importer ui should ring a bell when user interaction is needed.
+
+Default: ``yes``.


(If the default is changed, this needs changing too.)

sampsyo · 2017-03-27T21:39:41Z

beets/ui/commands.py

@@ -613,6 +613,9 @@ def choose_candidate(candidates, singleton, rec, cur_artist=None,
        })
        if default is None:
            require = True
+        # Bell ring when user interaction is needed.
+        if config['import']['bell']:
+            print('\a')


This seems to add an extra newline, in addition to ringing the bell. Perhaps you mean print('\a', end='')?

Also, all output from the commands needs to go through our custom ui.print_ function instead of the built-in print for dumb encoding reasons. Could you please use that function?

I didn't get the changelog describing part..What sould I do?

SpirosChadoulos · 2017-03-27T22:22:37Z

@sampsyo I did the new changes you told me. Is everything okay?

sampsyo

This looks great! Thank you again! ✨ I added a changelog entry.

SpirosChadoulos · 2017-03-28T14:20:10Z

@sampsyo Thanks a lot. Why next to the pull request there is a red x?

sampsyo · 2017-03-28T14:22:16Z

Hmm… which "x" are you referring to? If you mean the one next to the commit hash for the latest commit, it looks like that's because the AppVeyor build was canceled when I merged the PR. (You can hover over the "x" to see the details.)

SpirosChadoulos · 2017-03-28T14:56:10Z

Isn't it a problem that AppVeyor didnt' complete?

sampsyo · 2017-03-28T15:03:55Z

In this case, no. I merged the PR before the build had a chance to complete (because I already new it would succeed); AppVeyor then canceled the build. No harm, no foul. (If you like, you can see on the main branch commit history that things are still passing.)

SpirosChadoulos · 2017-03-28T15:22:52Z

Okay thanks a lot.

Introduced in #2495.

Added a terminal bell if interaction from the user is required

2fbc7f0

sampsyo requested changes Mar 26, 2017

View reviewed changes

SpirosChadoulos added 2 commits March 27, 2017 01:02

Added bell when user interaction is needed

5128b95

Added bell when user interaction is needed

28c423b

Added an if statement that allows the bell ring according to the user…

c605a91

…'s configuration.

sampsyo requested changes Mar 27, 2017

View reviewed changes

small changes for the bell issue

2189682

sampsyo added 2 commits March 27, 2017 23:23

Fix heading underline length

70ef6e7

Changelog/thanks for beetbox#2495

ffdeb1e

sampsyo approved these changes Mar 28, 2017

View reviewed changes

sampsyo merged commit ae42ea6 into beetbox:master Mar 28, 2017

sampsyo added a commit that referenced this pull request Apr 30, 2017

Always pass unicode to print_

8e78cfd

Introduced in #2495.

antlarr pushed a commit to antlarr/beets that referenced this pull request May 8, 2017

Changelog/thanks for beetbox#2495

c23b228

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a terminal bell if interaction from the user is required, according to this issue: https://github.com/beetbox/beets/issues/2366 #2495

Added a terminal bell if interaction from the user is required, according to this issue: https://github.com/beetbox/beets/issues/2366 #2495

SpirosChadoulos commented Mar 26, 2017 •

edited by sampsyo

Loading

mention-bot commented Mar 26, 2017

sampsyo left a comment

sampsyo Mar 26, 2017

SpirosChadoulos Mar 26, 2017

SpirosChadoulos commented Mar 26, 2017

sampsyo commented Mar 26, 2017

SpirosChadoulos commented Mar 27, 2017 •

edited

Loading

sampsyo left a comment

sampsyo Mar 27, 2017

sampsyo Mar 27, 2017

sampsyo Mar 27, 2017

sampsyo Mar 27, 2017

SpirosChadoulos Mar 27, 2017

SpirosChadoulos commented Mar 27, 2017

sampsyo left a comment

SpirosChadoulos commented Mar 28, 2017

sampsyo commented Mar 28, 2017

SpirosChadoulos commented Mar 28, 2017

sampsyo commented Mar 28, 2017 •

edited

Loading

SpirosChadoulos commented Mar 28, 2017


		Whether the importer ui should ring a bell when user interaction is needed.

		Default: ``yes``.

Added a terminal bell if interaction from the user is required, according to this issue: https://github.com/beetbox/beets/issues/2366 #2495

Added a terminal bell if interaction from the user is required, according to this issue: https://github.com/beetbox/beets/issues/2366 #2495

Conversation

SpirosChadoulos commented Mar 26, 2017 • edited by sampsyo Loading

mention-bot commented Mar 26, 2017

sampsyo left a comment

Choose a reason for hiding this comment

sampsyo Mar 26, 2017

Choose a reason for hiding this comment

SpirosChadoulos Mar 26, 2017

Choose a reason for hiding this comment

SpirosChadoulos commented Mar 26, 2017

sampsyo commented Mar 26, 2017

SpirosChadoulos commented Mar 27, 2017 • edited Loading

sampsyo left a comment

Choose a reason for hiding this comment

sampsyo Mar 27, 2017

Choose a reason for hiding this comment

sampsyo Mar 27, 2017

Choose a reason for hiding this comment

sampsyo Mar 27, 2017

Choose a reason for hiding this comment

sampsyo Mar 27, 2017

Choose a reason for hiding this comment

SpirosChadoulos Mar 27, 2017

Choose a reason for hiding this comment

SpirosChadoulos commented Mar 27, 2017

sampsyo left a comment

Choose a reason for hiding this comment

SpirosChadoulos commented Mar 28, 2017

sampsyo commented Mar 28, 2017

SpirosChadoulos commented Mar 28, 2017

sampsyo commented Mar 28, 2017 • edited Loading

SpirosChadoulos commented Mar 28, 2017

SpirosChadoulos commented Mar 26, 2017 •

edited by sampsyo

Loading

SpirosChadoulos commented Mar 27, 2017 •

edited

Loading

sampsyo commented Mar 28, 2017 •

edited

Loading