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

Add skip by rating or tag #2201

Closed
wants to merge 6 commits into from
Closed

Add skip by rating or tag #2201

wants to merge 6 commits into from

Conversation

101100
Copy link

@101100 101100 commented Jan 15, 2017

This adds a plugin that skips songs automatically if the rating is 0 or they have a non-empty skip tag.

I haven't done GTK in Python before, so there are two sections that I wonder if they could be done in a neater way; I'll add line comments for those.

Fixes #2161.

(self._CFG_SKIP_BY_TAG,
_("Skip Songs with a Non-Empty 'skip' _Tag")),
]
vb2 = Gtk.VBox(spacing=6)
Copy link
Author

Choose a reason for hiding this comment

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

Do I need vb2 along with vb here, or is that redundant? I copied this code from the duplicate songs plugin, but don't have the textbox above the checkboxes.

Copy link
Member

Choose a reason for hiding this comment

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

I think you could just return the frame and get rid of vb, but how it is now makes it easier to extend in the future.


def shouldSkip(self, playlist, song_iter):
song_index = playlist.get_path(song_iter).get_indices()[0]
song = playlist.get()[song_index]
Copy link
Author

Choose a reason for hiding this comment

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

Is there a nicer way to get the song object using the iterator? I looked here but I didn't see a better way.

Copy link
Member

Choose a reason for hiding this comment

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

playlist.get_value(song_iter) should work (from qltk.models.ObjectStore)

@lazka
Copy link
Member

lazka commented Jan 22, 2017

Sorry for the delay, I'll have a look tomorrow.

Copy link
Member

@lazka lazka left a comment

Choose a reason for hiding this comment

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

thanks

(self._CFG_SKIP_BY_TAG,
_("Skip Songs with a Non-Empty 'skip' _Tag")),
]
vb2 = Gtk.VBox(spacing=6)
Copy link
Member

Choose a reason for hiding this comment

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

I think you could just return the frame and get rid of vb, but how it is now makes it easier to extend in the future.


return previous

def shouldSkip(self, playlist, song_iter):
Copy link
Member

Choose a reason for hiding this comment

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

shouldSkip() -> should_skip()

Copy link
Author

Choose a reason for hiding this comment

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

Done.


def shouldSkip(self, playlist, song_iter):
song_index = playlist.get_path(song_iter).get_indices()[0]
song = playlist.get()[song_index]
Copy link
Member

Choose a reason for hiding this comment

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

playlist.get_value(song_iter) should work (from qltk.models.ObjectStore)

def shouldSkip(self, playlist, song_iter):
song_index = playlist.get_path(song_iter).get_indices()[0]
song = playlist.get()[song_index]
rating = song("~#rating")
Copy link
Member

Choose a reason for hiding this comment

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

~#rating defaults to the default rating if it isn't set, you might also want to check song.has_rating in case the default is set to 0 by the user.

Copy link
Author

@101100 101100 Jan 25, 2017

Choose a reason for hiding this comment

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

Good idea; added a check for has_rating.

song = playlist.get()[song_index]
rating = song("~#rating")

shouldSkip = False
Copy link
Member

Choose a reason for hiding this comment

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

should_skip

Copy link
Author

Choose a reason for hiding this comment

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

Done.

if self.config_get_bool(self._CFG_SKIP_BY_RATING) and rating <= 0:
shouldSkip = True
print_d("Rating is %f; skipping..." % (rating))
elif self.config_get_bool(self._CFG_SKIP_BY_TAG) and \
Copy link
Member

Choose a reason for hiding this comment

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

FYI generally we prefer to use parentheses over multi-line expressions rather than \ at the end.

Copy link
Author

Choose a reason for hiding this comment

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

Switched to parentheses.

shouldSkip = True
print_d("Rating is %f; skipping..." % (rating))
elif self.config_get_bool(self._CFG_SKIP_BY_TAG) and \
song("skip") != '':
Copy link
Member

Choose a reason for hiding this comment

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

and song("skip") is more Pythonic I'd say

Copy link
Author

Choose a reason for hiding this comment

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

Removed comparison with '' and moved and to the next line.

@declension
Copy link
Member

Seems conceptually ok - but I'm still confused why users would want to add a skip tag.

@lazka
Copy link
Member

lazka commented Jan 25, 2017

Seems conceptually ok - but I'm still confused why users would want to add a skip tag.

Agreed, zero rating should be enough (or below threshold)

@101100
Copy link
Author

101100 commented Jan 25, 2017

but I'm still confused why users would want to add a skip tag.

For me, a rating of zero means "I don't even want this synchronized." A skip tag is usually added to any non-song in an album. I would like to have the option to use both. Since others might not agree, that is why I went to the effort to add the preferences.

The other issue is that ratings do not save consistently to the actual files, while tags do.

@101100
Copy link
Author

101100 commented Jan 25, 2017

I've made the default to skip ratings of zero and ignore the skip tag.

@lazka lazka closed this in 2c446eb Mar 6, 2017
@lazka
Copy link
Member

lazka commented Mar 6, 2017

I've made a few adjustments and pushed to master.

@101100
Copy link
Author

101100 commented Mar 14, 2017

@lazka Thanks for adding this.

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.

3 participants