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

Vueify add show options #4848

Merged
merged 32 commits into from
Sep 10, 2018
Merged

Vueify add show options #4848

merged 32 commits into from
Sep 10, 2018

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Aug 5, 2018

Fixes #4838
Fixes #4525

TODO:

  • finish add-show-options.vue component
  • release black/white list groups
  • Update addShows_newShow.mako to use the new component
  • save default options. We're using vue data now that changes.
  • The save default button should only be enabled if an option has changed.
  • error handling when getting black/white lists. For example we cache the bl/wh list in dogpile. So often the api will use that. But when the bl/wh are not available from cache, they are requested through the anidb udp api, which is not that reliable. Requests can timeout. So we should handle that. Maybe give some feedback.

To properly use black/white lists from anidb, we need merge another PR, where I implemented dogpile caching for the anidb udp api.

A followup (new PR) will be to change all other pages, that use inc_addShowOptions.mako.

  • addShows_addExistingShow.mako
  • addShows_recommended.mako
  • addShows_trendingShows.mako

@@ -563,5 +570,5 @@ Vue.use(VueNativeSock, websocketUrl, {
reconnectionAttempts: 2, // (Number) number of reconnection attempts before giving up (Infinity),
reconnectionDelay: 1000 // (Number) how long to initially wait before attempting a new (1000)
});

debugger;

This comment was marked as resolved.

This comment was marked as resolved.

@@ -0,0 +1,258 @@
<template>
Copy link
Contributor

Choose a reason for hiding this comment

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

Notice you added that SFC to light and dark themes, it is no longer needed, so just remove the files and be sure to merge develop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed your pr


const { providedInfo } = this;
const { use, showId, showDir } = providedInfo;
if (use && showId !== 0 && showDir) {
Copy link
Contributor

@sharkykh sharkykh Aug 11, 2018

Choose a reason for hiding this comment

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

Don't remove this part please, it is used to automatically go to the options section when you are adding a show from a folder, with provided metadata from tvshow.nfo

Also use the normal loadsection as I did below and not the new method, because you do want it to go to the last section.

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.

I'm on mobile so not the best review, sorry in advance

section_data['default']['statusAfter'] = common.statusStrings[app.STATUS_DEFAULT_AFTER]
section_data['default']['seasonFolders'] = app.SEASON_FOLDERS_DEFAULT
section_data['default']['anime'] = app.ANIME_DEFAULT
section_data['default']['scene'] = app.SCENE_DEFAULT
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if default is the most descriptive name. Maybe showDefaults? Open to discussion of course...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think showDefaults is also fine.

@@ -37,7 +37,7 @@ module.exports = {
},
data() {
return {
localChecked: null
localChecked: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

You could just revert this change

formData.append('subtitles', subtitles ? '1' : '0');
formData.append('anime', anime ? '1' : '0');
formData.append('scene', scene ? '1' : '0');
formData.append('season_folders', seasonFolder ? '1' : '0');
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these can use Number(value) instead of the conditionals, however it should (need to check add_shows.py code) also be working with any truthy value, and I think supplying booleans is the best option in the long term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'd also rather pass booleans. And have the backend translate (as late as possible). Which in the long term should be done by the ORM, when there is no other way supported by db engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed them to Number(subtitles).. etc.

formData.append('blacklist', name);
}

const statusToDesc = { 'Wanted': 3, 'Skipped': 5, 'Ignored': 7 }
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a module in the store dedicated specifically for those, statuses, please use it instead of hardcoded values in javascript. We should only hardcode in Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea how to use that, sorry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this.$store.state.statuses I think?

@OmgImAlexis OmgImAlexis mentioned this pull request Aug 21, 2018
@p0psicles p0psicles added this to the 0.2.9 milestone Aug 21, 2018
OmgImAlexis
OmgImAlexis previously approved these changes Aug 21, 2018
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.

Look good overall. 👍

import ConfigToggleSlider from './config-toggle-slider.vue';
import AnidbReleaseGroupUi from './anidb-release-group-ui.vue';

module.exports = {
Copy link
Collaborator

@OmgImAlexis OmgImAlexis Aug 21, 2018

Choose a reason for hiding this comment

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

Should be export default {.

AnidbReleaseGroupUi,
ConfigToggleSlider
},
props: ['selectedShow'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs type and possibly default.

},
selectedSceneEnabled() {
this.update();
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be moved to mounted with something like this.

this.$watch(vm => [
    vm.selectedAnimeEnabled,
    vm.selectedStatus,
    vm.selectedStatusAfter,
    vm.selectedSeasonFolderEnabled,
    vm.selectedSceneEnabled
].join(), () => {
    this.update()
});

@@ -212,13 +226,33 @@ window.app = new Vue({
}

// Converts select boxes to command separated values [js/blackwhite.js]
generateBlackWhiteList(); // eslint-disable-line no-undef
//generateBlackWhiteList(); // eslint-disable-line no-undef
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remember to add/remove this before merge.


formData = new FormData(this.$refs.addShowForm);
}

this.otherShows.forEach(nextShow => formData.append('other_shows', nextShow));

// Because we're using te toggle-button.vue component, we don't have valid form input's for these.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing "h" in "the".

@sharkykh sharkykh added the Changelog Requires a changelog entry label Aug 23, 2018
@sharkykh sharkykh self-assigned this Aug 24, 2018
@sharkykh

This comment has been minimized.

@sharkykh sharkykh force-pushed the feature/vue-add-show-options branch 2 times, most recently from 54b5c7c to d2770ae Compare August 25, 2018 10:52
@ngosang

This comment has been minimized.

@sharkykh

This comment has been minimized.

@ngosang

This comment has been minimized.

@sharkykh sharkykh force-pushed the feature/vue-add-show-options branch from d2770ae to 2633509 Compare August 28, 2018 23:05
@sharkykh

This comment has been minimized.

@OmgImAlexis
Copy link
Collaborator

@sharkykh pretty sure there's still the issue of hitting "search" while a search is going just cancels instead of starting the new one.

@sharkykh
Copy link
Contributor

@OmgImAlexis That's the first time I'm reading about it... On develop?

@OmgImAlexis
Copy link
Collaborator

@sharkykh it's been there for a while now. If you click the "search" again it works. Pretty sure master has the same issue.

* Make addShows_newShow.mako use the component to configure the new shows options and send the options when adding a new show.
* Fix anidb-release-group-ui.vue, update the data when property get's updated after mount.
* Fix config-textbox, config-textbox-number.vue, config-toggle-slider.vue.
* Update config.py, make sure the status and statusAfter are send as Strings not the numbers.
@sharkykh sharkykh removed the Changelog Requires a changelog entry label Sep 4, 2018
@sharkykh

This comment has been minimized.

The original `addShowOptions` had that, this should too.

+ Add comment that the `QualityChooser` component should be imported
@medariox medariox modified the milestones: 0.2.9, 0.2.10 Sep 4, 2018
@sharkykh
Copy link
Contributor

sharkykh commented Sep 7, 2018

@p0psicles About that last "to-do", is it just you want to handle errors when calling home/fetch_releasegroups and show a message to the user?

@p0psicles
Copy link
Contributor Author

More like what i recently implemented. Its not relevant anymore

@p0psicles
Copy link
Contributor Author

Lets merge?

@sharkykh
Copy link
Contributor

Sure, hopefully I didn't miss anything big.

@p0psicles
Copy link
Contributor Author

Faster we merge faster we notice.

@p0psicles p0psicles merged commit a0a3d42 into develop Sep 10, 2018
@p0psicles
Copy link
Contributor Author

Tnx for your help @sharkykh

@p0psicles p0psicles deleted the feature/vue-add-show-options branch September 10, 2018 15:22
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.

5 participants