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

move quality-chooser to vue #3818

Merged
merged 18 commits into from
Apr 20, 2018
Merged

Conversation

OmgImAlexis
Copy link
Collaborator

@OmgImAlexis OmgImAlexis commented Feb 28, 2018

@p0psicles I still need to fix the forms on the pages that use this but it should be fine to test.

MAKE SURE NOT TO SAVE SHOWS WITH THIS IT WILL 100% KILL THE SHOW QUALITY

Edit: This should be working on both the edit and new show page. Please let me know if there's any issues.

Closes #3909

@OmgImAlexis OmgImAlexis added Enhancement In progress Needs review Needs testing Requires testing to make sure it's working as intended labels Feb 28, 2018
@p0psicles
Copy link
Contributor

Looks nice. But because you replace the mako, it has to be implemented in a big bang right? Can't we create new mako vue import files. And them implement the components one page at a time?

@OmgImAlexis
Copy link
Collaborator Author

I'm good with that. I still need to fix the forms but I could rename this and copy the old file back in?

Which page would we use first? Edit or new show?

@codecov-io
Copy link

codecov-io commented Mar 1, 2018

Codecov Report

Merging #3818 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3818   +/-   ##
========================================
  Coverage    29.26%   29.26%           
========================================
  Files          276      276           
  Lines        35411    35411           
========================================
  Hits         10363    10363           
  Misses       25048    25048

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8eaaa1a...86da320. Read the comment docs.

@p0psicles
Copy link
Contributor

Else you start on the new show. Then I'll finish editShow

@OmgImAlexis
Copy link
Collaborator Author

I think the only issue I found was on the edit page it would show blank instead of the quality you have it set as so I need to fix that.

@@ -1,5 +1,54 @@
<script type="text/x-template" id="quality-chooser-template">
Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs to be in themes-default/slim/views/vue-components/qualityChooser.mako

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did you want me to replace this file with the old content and just move the new code to a new file?

@p0psicles
Copy link
Contributor

@OmgImAlexis as discussed we should remove all direct python calls / mako vars. And pass those along through props. This will result in a clean set of components, that we can move to .Vue files in the end.

<div id="archive" v-if="archive">
<h5>
<b>
Archive downloaded episodes that are not currently in <a target="_blank" href="manage/backlogOverview/"><font color="blue"><u>backlog</u>.</font></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not showing anything when changing to a lower quality. I suspect you wanted to subsittute the .

@@ -18,6 +18,20 @@
% if show.is_anime:
<script type="text/javascript" src="js/blackwhite.js?${sbPID}"></script>
% endif
<script src="js/lib/vue.js"></script>
<script src="js/lib/[email protected]"></script>
<%include file="/vue-components/inc_qualityChooser.mako"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Still including the old file.

data() {
return {
// Python convertions
allowedQualities: ${convert(allowed_qualities)},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this so it's using props/events and not directly the python vars?

Copy link
Collaborator Author

@OmgImAlexis OmgImAlexis Apr 7, 2018

Choose a reason for hiding this comment

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

For now let's leave this here since otherwise each file that uses this will need that python code which will just duplicate it.

We should wait util those vars can be returned by the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I don't fully agree. But it's up to you. And what works best for you. So i'm fine with it.
Besides this, you think it's ready to merge? Needs some more reviewing? When this is ready I can merge it into my editShow pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it'l need a rebase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think after a rebase it'll still need a little bit of testing as I believe it still has the issue of getting the wrong quality when you first open the edit show page.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I'll merge it into my branch. Let's test there.

@OmgImAlexis OmgImAlexis force-pushed the feature/quality-chooser-x-template branch from 897128f to 3a1f847 Compare April 16, 2018 02:16
@sharkykh
Copy link
Contributor

sharkykh commented Apr 16, 2018

  • Remove missed <script type="text/javascript" src="js/quality-chooser.js?${sbPID}"></script> in editShow.mako
  • Remove quality-chooser.js.map files
  • Copy vue-submit-form.js from dark to the other themes.
  • Copy recent changes from dark to the other themes.
  • Verify Backlog warning works
  • Fix Backlog warning missing word and link.
    - Use <a target="_blank" href="manage/backlogOverview/" style="color: blue;">backlog</a>.
  • Fix archive episodes button, html and function.
    - For button use <button @click="archiveEpisodes" id="archiveEpisodes" class="btn btn-inline">Archive episodes</button>
    - Instead of this.backloggedEpisodes(); (which is not a function), use this.$forceUpdate(); to re-render.
  • Fix addShows_addExistingShow.mako - remove JS import, add component import.
  • Fix massEdit quality chooser:
    • Update component to support receiving quality as a property.
    • Update component to support the <Keep> (value="keep") option, and show it for massEdit only.
    • Make sure mass-editing quality actually saves the correct quality.
  • Run gulp on slim and make sure everything is up to date.

- Fix Backlog warning missing word and link.
- Fix archive episodes button, html and function.
@OmgImAlexis OmgImAlexis force-pushed the feature/quality-chooser-x-template branch from 3a1f847 to 134097a Compare April 16, 2018 10:48
@p0psicles
Copy link
Contributor

@OmgImAlexis @sharkykh i'm a bit confused. There aren't any child -> parent event's defined nor methods to patch the quality settings for a show directly? How should we update selected quality settings?

@sharkykh
Copy link
Contributor

sharkykh commented Apr 17, 2018

@p0psicles If I understand your question,
then I think this is what is needed for massEdit, since you have to "pass" the quality to the component when/after you use <quality-chooser></quality-chooser>, so it displays the correct one.

Currently this is working off of this:

if not show is UNDEFINED:
__quality = int(show.quality)
else:
__quality = int(app.QUALITY_DEFAULT)
allowed_qualities, preferred_qualities = Quality.split_quality(__quality)
overall_quality = Quality.combine_qualities(allowed_qualities, preferred_qualities)
selected = None

(BTW, line 61 is not needed?)

@p0psicles
Copy link
Contributor

Yeah have been talking with @medariox about it. And imo we should split quality into allowed and preferred. So we can cleanly pass them as properties but also store them independently.

Else we would have so always store the combined quality. But then you keep glaring to combine them in frontend.

@OmgImAlexis
Copy link
Collaborator Author

The quality-chooser should pass a value to the main page using the @change event.

I'd prefer if the quality was saved separately and if we didn't use bitwise operators everywhere to compare quality. It'd make working with the quality settings on the frontend a lot easier.

@OmgImAlexis
Copy link
Collaborator Author

Fix addShows_addExistingShow.mako - remove JS import, add component import.

@sharkykh can you check my last commit fixed this?

@sharkykh
Copy link
Contributor

sharkykh commented Apr 17, 2018

@OmgImAlexis
Fixed the quality chooser. Save Defaults not working:

2018-04-18 02:15:27 ERROR    Thread_1 :: [6d54662] Exception generated: saveAddShowDefaults() takes at least 4 arguments (7 given)
Traceback (most recent call last):
  File "C:\Medusa\medusa\server\web\core\base.py", line 285, in async_call
    result = function(**kwargs)
TypeError: saveAddShowDefaults() takes at least 4 arguments (7 given)

The call is:
/config/general/saveAddShowDefaults?defaultStatus=7&allowed_qualities=&preferred_qualities=&subtitles=true&anime=false&scene=false&defaultStatusAfter=3

And I selected "SD" preset.

Edit: Fixed by #4027.

sharkykh and others added 4 commits April 18, 2018 15:40
**Always render** the allowed/preferred qualities select elements, so
jQuery could pull data from them to send to Python.

Signed-off-by: sharkykh <[email protected]>
it was probably re-added in the rebase by mistake

Signed-off-by: sharkykh <[email protected]>
@p0psicles
Copy link
Contributor

p0psicles commented Apr 18, 2018

Also emits an event when one of the qualities (allowed/preferred) changes.
Can be used in the parent like:
<quality-chooser @update="saveQualities"></quality-chooser>

And:

            saveQualities: function(qualities) {
                this.series.config.qualities.preferred = qualities.preferred;
                this.series.config.qualities.allowed = qualities.allowed;
            },

@p0psicles
Copy link
Contributor

What else do we need to do for this?

p0psicles
p0psicles previously approved these changes Apr 19, 2018
@sharkykh sharkykh force-pushed the feature/quality-chooser-x-template branch from 8c3e385 to c6bfe1f Compare April 19, 2018 20:40
@sharkykh
Copy link
Contributor

sharkykh commented Apr 19, 2018

With the latest commit:

  • Mass Edit quality chooser is working.
    < Keep > option will show up when editing two or more shows and at least one quality setting is different from the others.
  • Props added to the component:
    overallQuality [Number] - combined quality (allowed+preferred) to set in the quality chooser on load.
    Defaults to the converted Python var: show.quality if a show object is detected, app.QUALITY_DEFAULT otherwise.
    keep [Boolean] - This is used to mark the component for massEdit - for the added feature of the Keep option for the preset selector. Defaults to false.
  • Data:
    Now using allowedQualities, preferredQualities instead of selectedAllowed, selectedPreferred
  • Change in events:
    The qualities are now $emit()-ing individually, as:
    update:quality:allowed for allowed qualities
    update:quality:preferred for preferred qualities

Notes:

  • Recalculating the backlogged episodes after archiving is not working. This is a small visual bug.
    Marked it as @FIXME for the sake of merging faster.

when changing from a preset to custom quality

+ Fix indentation in Vue code
@p0psicles p0psicles merged commit 6b67a67 into develop Apr 20, 2018
@p0psicles p0psicles deleted the feature/quality-chooser-x-template branch April 20, 2018 05:53
@OmgImAlexis OmgImAlexis mentioned this pull request Apr 20, 2018
@p0psicles p0psicles added this to the 0.2.2 milestone Apr 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concluded Enhancement Needs testing Requires testing to make sure it's working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants