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

Fix for SiCKRAGETV/sickrage-issues#2578: Add a "Size" column on shows list #2356

Merged
merged 3 commits into from Sep 6, 2015
Merged

Fix for SiCKRAGETV/sickrage-issues#2578: Add a "Size" column on shows list #2356

merged 3 commits into from Sep 6, 2015

Conversation

MGaetan89
Copy link
Contributor

It needs to be tested for potential performance issue before being included in develop/master.
I only have 2 shows with files in my dev environment, so I am not sure about the result.

@MGaetan89 MGaetan89 changed the title Fix for SiCKRAGETV/sickrage-issues#2578: Include a print "Shows list" feature Fix for SiCKRAGETV/sickrage-issues#2578: Add a "Size" column on shows list Aug 28, 2015
@MGaetan89
Copy link
Contributor Author

Just noticed that the commit message is wrong. I will fix it later.

@OmgImAlexis
Copy link
Contributor

The load times seem okay, just tried this on my install with about 100 shows totalling 4TB using a SMB mount and it loaded maybe 300ms slower.

@MGaetan89
Copy link
Contributor Author

Good to know. The overhead will be gone once the total size is saved in the DB.

@OmgImAlexis
Copy link
Contributor

Since we've removed about 500ms this week, I don't think that's too bad and you're right the db will help a LOT.

@fernandog
Copy link
Contributor

Fix size sorting and lets merge to make some tests

@OmgImAlexis
Copy link
Contributor

@MGaetan89 Did you want to work on the sorting for this or should I do it?

After looking through home.mako I'm not too sure how tablesort works considering it looks like it only lists some of the columns for sorting.

@OmgImAlexis
Copy link
Contributor

You also need to add align="center" to the td.

@MGaetan89
Copy link
Contributor Author

I can do it. I will work on it after work ;)

@MGaetan89
Copy link
Contributor Author

Commit message is fixed, and the cell is centered. I'm looking at the sort issue now.

@MGaetan89
Copy link
Contributor Author

The sort should be fix. So you can merge if you want to try it.
By doing that I think I have found a cleaner solution to sort the progress (3d2f382). I will investigate that.

@fernandog
Copy link
Contributor

Active column swapped with size

@MGaetan89
Copy link
Contributor Author

Yes, because the "Size" column is before the "Active" column

@fernandog
Copy link
Contributor

@MGaetan89

contents swapped
image

@fernandog
Copy link
Contributor

and still not working the sorting. related?

@MGaetan89
Copy link
Contributor Author

O.o I don't have that issue on my side. I tried both themes (light and dark to be sure), deleted cache, and enabled the filter row. In all cases it works fine.
I'm not sure where this might come form...

@fernandog
Copy link
Contributor

Will try delete cache

@fernandog
Copy link
Contributor

Deleted all /cache except image.
Git reset
Apply .patch
Still the same

@miigotu any idea?

@fernandog
Copy link
Contributor

Also deleted browser cache

@MGaetan89
Copy link
Contributor Author

Same for me (about browser cache).
Which browser are you using? I tried with the latest version of Chrome.
Do you have some specific option enable maybe?

@fernandog
Copy link
Contributor

Latest chrome. Will try with windows when I get home

@miigotu
Copy link
Contributor

miigotu commented Aug 28, 2015

No one has some videos on fat32 partition to test this with?
Also need to test the "free disk space" feature of the status/info page with a path on fat32 specifically.

@miigotu
Copy link
Contributor

miigotu commented Aug 28, 2015

I see two js bugs xD.

@OmgImAlexis
Copy link
Contributor

@MGaetan89 if you're still looking for a cleaner way of doing 3d2f382 I'd suggest you look at the tablesorter and change where it's getting the text from so instead of something like this 5: function(node) { return $(node).find("span").text(); }, it'd be 5: function(node) { return $(node).find("span").eq(1).text(); }, so it selects the 2nd span. This is lines 120 - 210.

@OmgImAlexis
Copy link
Contributor

I'm just taking a guess here but @fernandog's issue may be from only selecting certain columns, I haven't tried it but that's the only thing I can think of that may be causing it.

% except sickbeard.exceptions.ShowDirNotFoundException:
<% show_size = sickbeard.helpers.get_size(curShow._location) %>
% endtry

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@MGaetan89
Copy link
Contributor Author

@OmgImAlexis I did it in #2368 ;)
@miigotu What are the 2 JS bugs?

@@ -222,7 +224,7 @@ $(document).ready(function(){
4: { sorter: 'quality' },
5: { sorter: 'eps' },
% if sickbeard.FILTER_ROW:
6: { filter : 'parsed' }
7: { filter : 'parsed' }
% endif
},

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@MGaetan89
Copy link
Contributor Author

I updated the default column sort index. @fernandog can you try again to see if it works for you now?

@miigotu can you detail the two JS bugs you talked about in #2356 (comment)?

@OmgImAlexis
Copy link
Contributor

@MGaetan89 since I said I was going to redo a lot of the js, what are your thoughts on converting this over to using the data element for sorting? Obviously I won't change anything now but I think it'd be a better option than using this method of trying to select the correct piece of text, etc.

@miigotu
Copy link
Contributor

miigotu commented Aug 30, 2015

@MGaetan89 I was referring to the item index and the sortlist item.

@miigotu miigotu closed this Aug 30, 2015
@miigotu miigotu reopened this Aug 30, 2015
@MGaetan89
Copy link
Contributor Author

It's a good idea. And the sort options are duplicated (one for the show table, one for the anime table). So they could be extracted in a common variable to prevent issues when a change is made.

@miigotu Ok. I thought there were something else.

@fernandog
Copy link
Contributor

working here! thanks!

@MGaetan89
Copy link
Contributor Author

@fernandog Thanks for the feedback. I will address @miigotu comment and then it should good to merge.
I don't know if someone was able to test on fat32?

@fernandog
Copy link
Contributor

is it possible to check if tv-show folder is fat32, then don't calculate it ?

@WebSpider
Copy link
Contributor

Yes, it is. https://github.com/giampaolo/psutil to detect the file system type.

-----Original Message-----
From: "Fernando" notifications@github.com
Sent: ‎8/‎31/‎2015 11:17 PM
To: "SiCKRAGETV/SickRage" SickRage@noreply.github.com
Subject: Re: [SickRage] Fix for SiCKRAGETV/sickrage-issues#2578: Add a "Size"column on shows list (#2356)

is it possible to check if tv-show folder is fat32, then don't calculate it ?

Reply to this email directly or view it on GitHub.

@miigotu
Copy link
Contributor

miigotu commented Aug 31, 2015

@WebSpider psutil is not an option, as it requires c extensions.

@MGaetan89
Copy link
Contributor Author

Maybe the easiest solution would be to add an option to enable/disable this column? Disabled by default, user would have to enable it if they want to display it.
The downside is that the feature will be hidden and not working the same way as the other columns...

@fernandog
Copy link
Contributor

what about this:
http://stackoverflow.com/a/7120061

@miigotu
Copy link
Contributor

miigotu commented Sep 1, 2015

@fernandog that SO answer is for our hard link check, and it is not a good solution anyways for even that as it does more processing than necessary to determine if two paths are on that same partition.

@MGaetan89 it just needs tested on fat32, because I'm not sure if it was getting file size, or determining free space on fat32 that takes a hella amount of time. If it is determining free space, then the slow one would be on the status page.

@MGaetan89
Copy link
Contributor Author

I guess I can setup a disk with fat32 and some shows, but I can do that until next week-end (11/12th September).

@miigotu
Copy link
Contributor

miigotu commented Sep 6, 2015

Conflicts now ^

@MGaetan89
Copy link
Contributor Author

Updated :)

@miigotu
Copy link
Contributor

miigotu commented Sep 6, 2015

Im just gonna merge this, and if someone's install barfs Im sure we will know pretty quick.

miigotu added a commit that referenced this pull request Sep 6, 2015
Fix for SiCKRAGETV/sickrage-issues#2578: Add a "Size" column on shows list
@miigotu miigotu merged commit 3797973 into SiCKRAGE:develop Sep 6, 2015
@MGaetan89 MGaetan89 deleted the fr/show_size branch September 6, 2015 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants