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 ability to manually change episode quality. Fixes #4474 #4658

Merged
merged 17 commits into from
Jul 15, 2018

Conversation

medariox
Copy link
Contributor

  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

img1
img2

Copy link
Contributor

@sharkykh sharkykh left a comment

Choose a reason for hiding this comment

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

Not a full review yet...

@@ -297,7 +297,7 @@ def downloadSubtitleMissed(self, *args, **kwargs):
to_download[(cur_indexer_id, cur_series_id)] = [str(x[b'season']) + 'x' + str(x[b'episode']) for x in all_eps_results]

for epResult in to_download[(cur_indexer_id, cur_series_id)]:
season, episode = epResult.split('x')
season, episode = epResult.lstrip('s').split('e')
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a problem here, look at line 297, it's still using the SNxEP format

<option selected value="">Change quality to:</option>
<% qualities = sorted(Quality.qualityStrings.items(), key=operator.itemgetter(0)) %>
% for quality, name in qualities:
% if quality > 1: # Skip N/A (0) and Unknown (1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer:

% if quality not in (Quality.NA, Quality.UNKNOWN):

CHANGELOG.md Outdated
@@ -3,6 +3,7 @@
**New Features**

- Hot-swap themes: No need to restart the Medusa after changing the theme ([#4271](https://github.com/pymedusa/Medusa/pull/4271))
- Added ability to manually change episode quality ([#4658](https://github.com/pymedusa/Medusa/pull/4658))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't call it a new feature...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was never possible to only change the quality of an episode... so it definitely is IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we can also move it to improvements, it's the same for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like improvements would be a better description.

@@ -414,4 +424,19 @@ MEDUSA.home.displayShow = function() { // eslint-disable-line max-lines
log.error(error.data);
});
});

function setQuality(quality, seriesSlug, episodes) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use const setQuality = (quality, seriesSlug, episodes) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? That's a really ugly syntax IMO 😕

Copy link
Collaborator

Choose a reason for hiding this comment

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

How so? That's an arrow function.

The reason I wanted this change was the fact that functions get hoisted and you should avoid that if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like that at first. But it gets better when your getting used to it

});

api.patch('series/' + seriesSlug + '/episodes', patchData)
.then(response => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

.then should be on the same line as the api.patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I change it the XO lint won't pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

Try

api.patch('series/' + seriesSlug + '/episodes', patchData).then(response => {
    log.info(response.data);
    window.location.reload();
}).catch(error => {
    log.error(error.data);
});

@OmgImAlexis
Copy link
Collaborator

Strings can be checked if (var_name) no need for the check against an empty string.

@sharkykh
Copy link
Contributor

sharkykh commented Jul 13, 2018

I hope this won't cause more confusion.

For example I changed from Snatched <SD> to Skipped (no quality selected),
and got Skipped <SD>. Doesn't really make sense.

@sharkykh sharkykh self-requested a review July 14, 2018 10:45
@p0psicles
Copy link
Contributor

p0psicles commented Jul 15, 2018

I think where moving to a situation where status and quality should be independent of one another. It gives more flexibility. I think that almost in all places we use the quality or the status of an episode. But not both at the same time. That we display them together shouldn't matter much.

As an example the Skipped (SD) could occur. As you can change the status of an already downloaded ep to Skipped. Then i dont mind knowing what's already on disk.

Do we really want to make all these decisions for our users? Like removing the quality when its changed to the status skipped? Users already voiced they want to have that flexibility.

Copy link
Contributor

@p0psicles p0psicles left a comment

Choose a reason for hiding this comment

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

Im not the best reviewer. But this looks good te me.

Copy link
Collaborator

@OmgImAlexis OmgImAlexis left a comment

Choose a reason for hiding this comment

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

I would have preferred the new code in showheader.mako to use Vue's store but other than that it looks good. 👍

@sharkykh sharkykh removed their request for review July 15, 2018 13:13
@medariox medariox merged commit 516ec37 into develop Jul 15, 2018
@medariox medariox deleted the feature/add-quality-changer branch July 15, 2018 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants