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

Selective apply of functions to items #1843

Merged
merged 12 commits into from Feb 4, 2016

Conversation

Projects
None yet
3 participants
@pkess
Collaborator

pkess commented Jan 28, 2016

as suggested in #1838 i implemented a more generic way to let the user select a list of items to apply an action for

@sampsyo implementation for modify and move command will follow if this approach meets your requirements

pkess added some commits Jan 28, 2016

Introduced input_select_items
alternative and more flexibile implementation to fulfil #1723
Added test case for new input method
introduced print_modify_item as seperate function
preparation to implement interactive selection in modify command
@pkess

This comment has been minimized.

Collaborator

pkess commented Jan 28, 2016

I added the interactive mode for both modify and move command now and added a changelog entry. Please review
TODO:

  • Add test case for modify and timid mode
  • Maybe add a configuration option to put move in timid mode always
out_items = []
choice = input_options(
('y', 'n', 's'), False,
'%s (Yes/No/Selective)?' % prompt)

This comment has been minimized.

@sampsyo

sampsyo Jan 29, 2016

Member

A tiny thing, but I might find the verb "Select" more intuitive than the noun "Selective".

@@ -1364,6 +1359,21 @@ def modify_items(lib, mods, dels, query, write, move, album, confirm):
obj.try_sync(write, move)
def print_modify_item(obj, mods, dels):

This comment has been minimized.

@sampsyo

sampsyo Jan 29, 2016

Member

More naming stuff: I suggest print_and_modify, without item, because the argument can either be an Item or an Album.

This comment has been minimized.

@pkess

pkess Jan 29, 2016

Collaborator

I think print_and_modify implies that the function would really apply those changes.
What about print_modify_changes?

This comment has been minimized.

@sampsyo

sampsyo Jan 29, 2016

Member

Well, it does call obj.update(), which actually modifies the object—so it's not just printing. Maybe update_and_print?

This comment has been minimized.

@pkess

pkess Jan 30, 2016

Collaborator

You are right. I changed it and will push the change later

@@ -1364,6 +1359,21 @@ def modify_items(lib, mods, dels, query, write, move, album, confirm):
obj.try_sync(write, move)
def print_modify_item(obj, mods, dels):
"""Print the modifications to an item
and return False if no changes were made

This comment has been minimized.

@sampsyo

sampsyo Jan 29, 2016

Member

Or, more specifically, "return a bool indicating whether any changes were made"?

elif choice == 's':
for item in items:
rep(item)
if input_yn('%s (y/n)?' % prompt, True):

This comment has been minimized.

@sampsyo

sampsyo Jan 29, 2016

Member

Perhaps it would be worthwhile to have some way—even if it's not an explicit option letter—to cancel the process entirely (return []) from this prompt in case you enter it accidentally.

This comment has been minimized.

@pkess

pkess Jan 29, 2016

Collaborator

Sorry, i don't understand, what you mean

This comment has been minimized.

@sampsyo

sampsyo Jan 29, 2016

Member

Sorry, that was confusingly put. Here's what I mean. Say you get a prompt like this:

Really do something to 1000 albums? (Yes/No/Select) s
Do something to album 1? n
Do something to album 2? n
...

If I accidentally hit "s" when I meant "n", it will be a long time before I can counteract my mistake. I'll have to hit "n" all day or find some other way to kill the process.

The same thing happens if I accidentally hit "y" for one of the thousand prompts and want to start over.

In both cases, it would be useful to have some action I could take to cancel the whole process at one of the individual prompts. That would cause the input_select_items function to return an empty list, indicating that no action should be taken.

This comment has been minimized.

@pkess

pkess Jan 30, 2016

Collaborator

yeah, thats right. I would suggest to press CTRL-C in this case and try it again.

The best will be to implement the interface in a way git checkout -p does.

Apply this hunk to index and worktree [y,n,q,a,d,/,e,?]? ?
y - apply this hunk to index and worktree
n - do not apply this hunk to index and worktree
q - quit; do not apply this hunk nor any of the remaining ones
a - apply this hunk and all later hunks in the file
d - do not apply this hunk nor any of the later hunks in the file
g - select a hunk to go to
/ - search for a hunk matching the given regex
j - leave this hunk undecided, see next undecided hunk
J - leave this hunk undecided, see next hunk
k - leave this hunk undecided, see previous undecided hunk
K - leave this hunk undecided, see previous hunk
s - split the current hunk into smaller hunks
e - manually edit the current hunk
? - print help

I would suggest these commands:

  • y - yes
  • n - no
  • a - print all following and ask again
  • q - quit and revert selected ones
  • ? - print help

please leave comment whether this attracts to you

This comment has been minimized.

@sampsyo

sampsyo Jan 31, 2016

Member

Yeah, ^C is fine if it works. We've had trouble with getting keyboard interrupts to work while inside prompts, but maybe that's only in multi-threaded environments.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 29, 2016

Cool! This is feeling like the right direction. It might make sense to try to unify all the different places where we do confirmation (if there are any others—remove, perhaps?) and funnel them through a similar codepath.

@pkess

This comment has been minimized.

Collaborator

pkess commented Jan 29, 2016

I changed "Selective" to "Select items" now, as it more intuitive.
I also changed the docstring. Ok now?

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 29, 2016

Cool, that helps! Thanks!

If we're going to have a noun in the prompt, though, it should be "Select {}s".format('album' if album else 'item'), though, since this function could be called on both Album and Item objects. In fact, maybe the function name should avoid using item also.

Thanks for being with all the terminology bike-shedding! We're definitely getting closer.

out_items = []
choice = input_options(
('y', 'n', 's'), False,
'%s (Yes/No/Select items)?' % prompt)

This comment has been minimized.

@sampsyo

sampsyo Jan 29, 2016

Member

Again on the detail-obsession topic: most of the other confirmation prompts in beets seem to have the form "Question? (A/b/c)" rather than "Question (A/b/c)?", so we should probably try for consistency with the placement of the question mark.

Also, we usually format this kind of prompt using just the initial letters, one of which is capitalized to indicate that it's the default. Of course, that isn't as easy to read when the options aren't just Y and N, which are self-evident. It's possible that we need some creativity here—how can we make it clear that there's a default, that you can just type one letter, and yet also that there's a third option?

This comment has been minimized.

@pkess

pkess Jan 30, 2016

Collaborator

That's right. I would like a way which will let you print a help like suggested above

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 31, 2016

On the broader question of what the prompt should look like: let's stick with something simple for now. The plain (colorized) output from input_options will do just fine:

>>> beets.ui.input_options(('yes', 'no', 'select'))
[Y]es, No, Select?

The capitals, brackets, and colors help suggest that Y and N are options, that Y is the default, and that you can also choose to "select" without being too verbose.


Short, off-topic rant: Overall, I'm worried about adding too much complexity to what should be a very simple prompt for most uses. The majority of people will just want to say "yes" or "no" to a listing, so giving a long list of options to read seems like a shame. Git is certainly a counterexample, but IMO option overload is one of the reasons git is so unusable.

changed prompt for input_select_items
moved question mark and removed word "item"
@pkess

This comment has been minimized.

Collaborator

pkess commented Feb 3, 2016

Hi,

i changed the prompt to a more common one.
If i understand you, you don't want to extend the interface to support those additional options mentioned above. So i would not implement this at the moment.

I also added some code to catch a KeyboardInterrupt in pkess/catch_keyboardinterrupt. Are there any contradictions to implement it like this? It would suppress the Backtrace on output and exit in a clean way. I could also add a line to tell the user s.th. like "Abort due to CTRL-C"

@pkess

This comment has been minimized.

Collaborator

pkess commented Feb 3, 2016

If you are ok with this implementation, i would like to merge it to master.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Feb 4, 2016

Great!

Regarding KeyboardInterrupt, I believe we already handle this at the top level (in main), so I don't think we need another handler here.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Feb 4, 2016

Looks good to me after a review, so please do feel free to merge.

If you can, at some point, please add a brief note to the docs for modify describing the feature. It will be good to have at least one description written down of the feature's intent and exactly how to use it.

@pkess pkess merged commit 89d38ca into beetbox:master Feb 4, 2016

pkess added a commit that referenced this pull request Feb 4, 2016

Merge branch 'selective_modify2'
Conflicts:
	docs/changelog.rst

closes #1843

@pkess pkess deleted the pkess:selective_modify2 branch Feb 4, 2016

pkess added a commit to pkess/beets that referenced this pull request Feb 4, 2016

sampsyo added a commit that referenced this pull request Feb 4, 2016

sampsyo added a commit that referenced this pull request Feb 7, 2016

Refine naming and docs for #1843
Most urgently, this function doesn't just work on *items*; it works on
arbitrary objects (and, in particular, albums).

sampsyo added a commit that referenced this pull request Feb 7, 2016

A little more documentation massaging for #1843
To make the docstirng fit in with the standard style.
@Vrihub

This comment has been minimized.

Contributor

Vrihub commented Nov 24, 2016

If you can, at some point, please add a brief note to the docs for modify describing the feature. It will be good to have at least one description written down of the feature's intent and exactly how to use it.

Yes, please document modify select feature :)

@pkess

This comment has been minimized.

Collaborator

pkess commented Nov 24, 2016

Hi, i just opend PR #2280 to meet these requirements

sampsyo added a commit that referenced this pull request Nov 25, 2016

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