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

Added Programme Name to the available columns for the Downloads list. #212

Merged
merged 6 commits into from May 6, 2017

Conversation

Projects
None yet
2 participants
@nbl1268
Contributor

nbl1268 commented Apr 29, 2017

Added new column to show the Programme Name for each of the items contained within the Download list.

nbl1268 added some commits Apr 29, 2017

@ribbons

Hey, thanks for your PR 😄

Looks good generally - one thing that jumps out though is that the changes to Providers/PodcastProvider/PodcastProvider.csproj and installer/Installer.wixproj as well as the addition of packages.conf under installer and Providers/PodcastProvider don't seem to be related to this change, could you drop them out of the PR? Let me know if you aren't sure how to do that.

I'll sprinkle a few other comments through the changes of things that occur to me.

@ribbons

ribbons requested changes Apr 30, 2017 edited

Oh, also - could you add yourself to AUTHORS.md and update the copyright date in the headers of the files you've changed.

default:
throw new InvalidDataException("Invalid column: " + sortBy.ToString());
}
using (SQLiteCommand command = new SQLiteCommand("select downloads.epid from downloads, episodes where downloads.epid=episodes.epid order by " + orderBy, FetchDbConn()))
using (SQLiteCommand command = new SQLiteCommand(sqlCmd + orderBy, FetchDbConn()))

This comment has been minimized.

@ribbons

ribbons Apr 30, 2017

Owner

Although it would be slightly less efficient, I'd say just add the programmes table to the original SQL command which then saves the need for the sqlCmd variable and duplicating parts of the statement. I'm pretty sure we won't be able to notice the improvement in performance when the Programme Name column isn't shown, even on huge lists of downloads.

This comment has been minimized.

@nbl1268

nbl1268 May 6, 2017

Contributor

ok, that makes. made this change but also needed to update the orderBy statement for EpisodeName to remove teh ambiguity of which 'name' column to order by.

@@ -825,7 +827,7 @@ private void ShowDownloadInfo(int epid)
}
this.SetToolbarButtons(buttons.ToArray());
this.SetSideBar(TextUtils.StripDateFromName(info.Name, info.Date), infoText, Model.Episode.GetImage(epid));
this.SetSideBar(TextUtils.StripDateFromName(progInfo.Name + " - " + info.Name, info.Date), infoText, Model.Episode.GetImage(epid));

This comment has been minimized.

@ribbons

ribbons Apr 30, 2017

Owner

I'm not sure if this addition belongs in this PR as it looks like it adds the programme name before the episode name on the sidebar. If you'd like to add this as well, I'd suggest splitting it out as a separate PR.

This comment has been minimized.

@nbl1268

nbl1268 May 6, 2017

Contributor

yes that is what it does, have removed and will add as a separate PR

@@ -1401,6 +1403,11 @@ private ListViewItem DownloadListItem(Model.Download info, ListViewItem item)
item.SubItems[column].Text = durationText;
break;
case Model.Download.DownloadCols.ProgrammeName:
// display Programme Name in the download list

This comment has been minimized.

@ribbons

ribbons Apr 30, 2017

Owner

I don't think that this comment is necessary.

This comment has been minimized.

@nbl1268

nbl1268 May 6, 2017

Contributor

removed comment

@@ -700,11 +702,16 @@ public static int Compare(int epid1, int epid2)
case DownloadCols.Duration:
orderBy = "duration" + (sortAsc ? string.Empty : " desc");
break;
case DownloadCols.ProgrammeName:
// OrderBy Programme.Name

This comment has been minimized.

@ribbons

ribbons Apr 30, 2017

Owner

I don't think that this comment is necessary.

This comment has been minimized.

@nbl1268

nbl1268 May 6, 2017

Contributor

removed comment

@@ -2422,7 +2429,7 @@ private void InitDownloadList()
this.ListDownloads.Clear();
this.ListDownloads.HideAllProgress();
const string DefaultColSizes = "0,2.49|1,0.81|2,1.28|3,1.04|4,0.6";
const string DefaultColSizes = "0,2.49|1,0.81|2,1.28|3,1.04|4,0.6|5,1.0";

This comment has been minimized.

@ribbons

ribbons Apr 30, 2017

Owner

I think that this could probably do with being a bit bigger - on my machine it only shows up as Programme N...

1.2 gets the whole title to display, but perhaps it should be 1.3 or 1.4 to display more of the programme names.

This comment has been minimized.

@nbl1268

nbl1268 May 6, 2017

Contributor

updated column width value to 1.4

@nbl1268

This comment has been minimized.

Contributor

nbl1268 commented May 2, 2017

@ribbons

This comment has been minimized.

Owner

ribbons commented May 2, 2017

Haven’t much experience using GitHub, thus wasn’t too sure how to make contact with you re all of this.
Seems though that this PR did the trick.

Yes, absolutely the right way 😄.

Re the ‘Providers’ and ‘Installer’ etc., I’m assuming its probably as simple as clicking the respective files (or folders) and asking GitHub to ignore them but any pointers you can give me there would be appreciated.

I'm not aware of an option like that, but I may have missed it. I'd suggest something along these lines from your local repository:

git reset HEAD~1 Providers/PodcastProvider/PodcastProvider.csproj installer/Installer.wixproj
git add Providers/PodcastProvider/PodcastProvider.csproj installer/Installer.wixproj
git rm installer/packages.conf Providers/PodcastProvider/packages.conf

Then make the other changes you want to make, and round off with:

git add <the files you've changed>
git commit --amend
git push --force-with-lease

Not sure if you are new to Git as well as GitHub, but if so - although it seems totally baffling at first it is an incredibly powerful and useful tool.

@nbl1268

This comment has been minimized.

Contributor

nbl1268 commented May 6, 2017

Actually only used github, but will do some reading to under git as well.
Also can see a .gitignore file that contains what look like filters for selected files and folders.
Could that be an equivalent option for those provide and installer files?

nbl1268 added some commits May 6, 2017

Updates for existing Pull Request
Remove unneeded files from the commit
Update Installer.wixproj
remove unnecessary changes
Update PodcastProvider.csproj
remove unnecessary change to this file
@nbl1268

This comment has been minimized.

Contributor

nbl1268 commented May 6, 2017

Hi Matt, Believe i've got all of your requests completed and also removed those unnecessary files. But not sure what is the next step i need to do. Neil

Clean up PR
Fix accidental lf-at-eol changes for the authors and project files and
remove an unused variable.  Also tweak the wording added to the authors
file and revert an accidental? .gitignore addition.
@ribbons

ribbons approved these changes May 6, 2017

To save you the trouble of having to make more changes, I took the liberty of pushing an additional commit which removed an unused variable and the addition to .gitignore (feel free to add that via another PR if you intended to add it), fixed some newline at end of file issues and tweaked the AUTHORS.md file wording.

@ribbons ribbons merged commit 695d32b into ribbons:master May 6, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ribbons

This comment has been minimized.

Owner

ribbons commented May 6, 2017

Many thanks for your PR 👍

@nbl1268

This comment has been minimized.

Contributor

nbl1268 commented May 7, 2017

Thanks really appreciate your help getting this first PR through :)
Yeah i do need that change to the .gitignore to filter out the local file syncing.

@ribbons ribbons added the enhancement label May 12, 2018

@ribbons ribbons added this to the 0.32 milestone May 12, 2018

@ribbons ribbons changed the title from Added new column to show the Programme Name for each of the items contained within the Download list. to Added Programme Name to the available columns for the Downloads list. May 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment