code cleanup #69

fuzzard · 2017-11-12T10:22:13Z

Further cleanup of existing code. Removing unused variables/functions.
Stub OpenLiveStream and CloseLiveStream as they are not required for implementation as we now rely on GetChannelStreamProperties
Removed some globals that are not used throughout the codebase anymore.

Let me know if ive overstepped anything at this stage. I think this is the final pass for cleanups that i can see at this stage.

ksooo · 2017-11-12T17:26:26Z

src/HDHomeRunTuners.cpp

-      }
-    }
-  }        
+        return jsonChannel["URL"].asString();


wrong indentation?

ksooo · 2017-11-12T17:27:08Z

src/HDHomeRunTuners.h

@@ -65,7 +65,7 @@ class HDHomeRunTuners
  };

 public:
-  HDHomeRunTuners();
+  HDHomeRunTuners(){};


Looks ugly without a blank between ) and {.

ksooo · 2017-11-12T17:28:09Z

src/Utils.h

@@ -47,7 +47,6 @@ int DbgPrintf(const char* szFormat, ...);
  } while (0)

 #define PVR_STRCPY(dest, source) do { strncpy(dest, source, sizeof(dest)-1); dest[sizeof(dest)-1] = '\0'; } while(0)


Maybe you can also get rid of this ugly (and unneeded) macro?

ksooo · 2017-11-12T17:28:42Z

src/client.h

@@ -27,6 +27,8 @@
 #include <libXBMC_pvr.h>
 #include <p8-platform/util/StringUtils.h>

+using namespace ADDON;
+
 typedef std::string String;


Another ugly piece of code... Something like this is just for the lazy people (or people not knowing "search and replace"... ;-)

fuzzard · 2017-11-12T20:47:44Z

Removed the typedef and the macro as suggested. Also saw a comment elsewhere about not using the namespace. So i pulled that change as well.
The indentation is correct, i can put the {} back in if you think that makes it easier to read. The github diff makes it look a lot more complicated than the change is.

Happy to squash, unless you can do that on merge, just let me know

ksooo

Code changes look sane. Thanks. Could you please update changelog and addon version number? Then we're good to go, imo.

fuzzard · 2017-11-13T19:53:45Z

rebased and added changelog/version bump.
Cheers

fuzzard · 2017-11-21T10:02:43Z

squashed and rebased after jsoncpp merge.
Also did some changes to headers. Explicitly putting the header includes in files that use them, rather than having them inherit from other headers. Reordered to suit kodi guidelines as well.
also had another unused variable thats been removed.

fuzzard · 2017-12-03T11:28:38Z

Couple more minors changed.
Removed addon namespace usage as it was only used for logging for the most part, so have just directly used the ADDON::XXXX types
Further header alterations. Make sure each file imports any and all headers required. In the past a lot of headers were just being pulled in due to the inclusion of the p8-platform headers.
Shuffled some variable declarations to better suit when they are required.
Removed another typdef that was only used a single time. Clearer to just have the declaration as it is i think.

fuzzard · 2017-12-05T11:46:28Z

@ksooo any chance of a final review. Ive repushed all of the changes and split it into individual commits to make it easier to review. The single commit was getting a bit out of hand to keep track of everything i think.

I also had a relook and think about the PVR_STRCPY define, and i didnt see it before, but it null terminates whatever is passed to it. In my initial pass of removing that, i wasnt dealing with null termination at all, which obviously is an issue. Should i stick with the define, or is the strcpy + null terminate at each use ok from a readability viewpoint?

sort headers according to kodi guidelines and include all appropriate headers in each source file

whitespace, aesthetics/layout cleanup Change final for auto range loops, move variable declarations closer to actual usage

fuzzard · 2017-12-09T22:57:22Z

Another commit added. Currently a discover is never done past initial startup of addon. This means no additional tuners will ever be added unless the addon is restarted. The change will now add a tuner without a full discover if it is not found in the m_Tuners vector. To handle a tuner disappearing, it will do a clear on the vector if the size is larger than the discovered nTunerCount returned by the discovery request.
This doesnt really handle the situation of the tuner count staying the same, but with different tuners. This is such a corner case, that i couldnt work out a clean way to handle it without a lot of extra code for no real use. On the subsequent update call (60 minutes), this should all wash out as the m_tuners size will be larger than the next discovery, so it will reset the vector and recreate it all anyway.

Figured id throw this and 2 other commits into this PR, as its proven difficult to continue to get PR's merged (this is in no way aimed at you ksooo, because i know you have far more stuff to do than keep looking at one measly addon).
If required i can pull out the feature change commits and put into another PR if desired.

ksooo · 2017-12-09T23:02:42Z

Another commit added.

Hold on! Give me some time to review the PR. I have the feeling if I find no time soon I will be confronted with a 50 commits PR containing all kind of different stuff. ;-)

ksooo

lgtm, just some nitpicking from my end. cannot say anything about the correctness of the hdhomerun functioality specific changes, though.

ksooo · 2017-12-11T12:36:48Z

pvr.hdhomerun/addon.xml.in

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <addon
  id="pvr.hdhomerun"
-  version="3.1.9"
+  version="3.1.10"


If you add new features, please increase minor version number (3.1.9 => 3.2.0 in this case).

ksooo · 2017-12-11T12:37:12Z

pvr.hdhomerun/changelog.txt

+v3.1.10
+- code cleanup
+- guide filters updated
+- additional epg_tag fields added


If you add new features, please increase minor version number (3.1.9 => 3.2.0 in this case).

ksooo · 2017-12-11T12:41:55Z

src/client.cpp

-
+  strncpy(signalStatus.strAdapterName, "PVR HDHomeRun Adapter 1",
+          sizeof(signalStatus.strAdapterName) - 1);
+  signalStatus.strAdapterName[sizeof(signalStatus.strAdapterName)-1] = '\0';


Could you please add blanks around the -?

ksooo · 2017-12-11T12:42:08Z

src/client.cpp

+  signalStatus.strAdapterName[sizeof(signalStatus.strAdapterName)-1] = '\0';
+  strncpy(signalStatus.strAdapterStatus, "OK",
+          sizeof(signalStatus.strAdapterStatus) - 1);
+  signalStatus.strAdapterStatus[sizeof(signalStatus.strAdapterStatus)-1] = '\0';


Could you please add blanks around the -?

make sure we null terminate our strncpy

change location of variable declarations to avoid construction before a failure case

remove and indent as needed whitespace remove temp variable and return directly in _GetChannelStreamURL()

change to c++ style casts for time_t and last for auto range loop in PvrGetEPGForChannel()

This handles an episode that is numbered via SD's guide to match an episode number such as E2017-199. This sets the series to first number (the year), and episodenumber to the last number.

move guidenumberset outside of tuner loop so it retains the set of channels for all found tuners.

Should test thread creation of updatethread actually succeeds, if not addon should fail as no guide/lineup will be updated.

If a new tuner is found, add it to the list and process as normal If total tuners found is less than the current list, clear and recreate tuner list for all This doesnt not handle the situation of the same tuner count, but different tuners. It will have a greater size on next update call, so should even out in the end, but it will go an hour between the next update call before it is fixed.

fuzzard · 2017-12-11T19:16:00Z

Cheers, done.

win10: fix build on UWP after #69

ksooo reviewed Nov 12, 2017

View reviewed changes

src/HDHomeRunTuners.cpp

}

}

}

return jsonChannel["URL"].asString();

Copy link

Member

ksooo Nov 12, 2017

Choose a reason for hiding this comment

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

wrong indentation?

ksooo reviewed Nov 12, 2017

View reviewed changes

fuzzard force-pushed the minors branch 2 times, most recently from 555ee0d to 3f1a124 Compare November 12, 2017 23:21

ksooo reviewed Nov 13, 2017

View reviewed changes

fuzzard force-pushed the minors branch from bd6fbab to b933a4f Compare November 13, 2017 19:53

fuzzard force-pushed the minors branch from b933a4f to 993b478 Compare November 21, 2017 09:57

fuzzard force-pushed the minors branch 4 times, most recently from 5d4ac31 to cb100a7 Compare December 3, 2017 11:27

fuzzard force-pushed the minors branch 5 times, most recently from 151b63a to ec1a4ef Compare December 5, 2017 11:38

fuzzard added 3 commits December 5, 2017 21:48

fix and arrange headers

398ca43

sort headers according to kodi guidelines and include all appropriate headers in each source file

remove typedefs

ecd88a7

remove usage of ADDON namespace

3b950e4

fuzzard force-pushed the minors branch 4 times, most recently from 4cac101 to ffc0b27 Compare December 8, 2017 04:13

fuzzard force-pushed the minors branch from ffc0b27 to 5264dd2 Compare December 8, 2017 06:42

cleanups to Update method

f95b234

whitespace, aesthetics/layout cleanup Change final for auto range loops, move variable declarations closer to actual usage

fuzzard force-pushed the minors branch from 5264dd2 to 6fdef45 Compare December 8, 2017 06:43

ksooo requested changes Dec 11, 2017

View reviewed changes

fuzzard added 13 commits December 12, 2017 05:15

remove unneeded defines

81b136e

make sure we null terminate our strncpy

remove unused variables/methods

fb941ac

cleanup header for HDHomeRunTuners

0bc8dee

change to c++11 for auto range loop

4931092

change location of variable declarations to avoid construction before a failure case

whitespace fixups

31f94ae

remove and indent as needed whitespace remove temp variable and return directly in _GetChannelStreamURL()

c++ style casts and for auto range loop

67bbb7d

change to c++ style casts for time_t and last for auto range loop in PvrGetEPGForChannel()

changelog and version bump

75c97c8

add additional EPG_TAG fields and content masks

e7cbab6

handle additional episode numbering

6ea2d88

This handles an episode that is numbered via SD's guide to match an episode number such as E2017-199. This sets the series to first number (the year), and episodenumber to the last number.

move guidenumberset outside of tuner loop

dc42fa8

move guidenumberset outside of tuner loop so it retains the set of channels for all found tuners.

test thread creation succeeds for g_UpdateThread

3f9ed01

Should test thread creation of updatethread actually succeeds, if not addon should fail as no guide/lineup will be updated.

use nullptr instead of NULL

06fa8fd

fuzzard force-pushed the minors branch from 19036e3 to e93c9f2 Compare December 11, 2017 19:15

ksooo approved these changes Dec 11, 2017

View reviewed changes

ksooo merged commit 63dfccb into kodi-pvr:master Dec 11, 2017

afedchin added a commit to afedchin/pvr.hdhomerun that referenced this pull request Dec 13, 2017

win10: fix build on UWP after kodi-pvr#69

ec7ed95

afedchin mentioned this pull request Dec 13, 2017

win10: fix build on UWP after #69 #72

Merged

afedchin added a commit that referenced this pull request Dec 13, 2017

Merge pull request #72 from afedchin/win10

b00a8b5

win10: fix build on UWP after #69

fuzzard pushed a commit to fuzzard/pvr.hdhomerun that referenced this pull request Dec 27, 2017

win10: fix build on UWP after kodi-pvr#69

0c0d8e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

code cleanup #69

code cleanup #69

fuzzard commented Nov 12, 2017

ksooo Nov 12, 2017

ksooo Nov 12, 2017 •

edited

Loading

ksooo Nov 12, 2017

ksooo Nov 12, 2017 •

edited

Loading

fuzzard commented Nov 12, 2017 •

edited

Loading

ksooo left a comment

fuzzard commented Nov 13, 2017

fuzzard commented Nov 21, 2017

fuzzard commented Dec 3, 2017

fuzzard commented Dec 5, 2017

fuzzard commented Dec 9, 2017

ksooo commented Dec 9, 2017

ksooo left a comment

ksooo Dec 11, 2017

ksooo Dec 11, 2017

ksooo Dec 11, 2017

ksooo Dec 11, 2017

fuzzard commented Dec 11, 2017

		@@ -47,7 +47,6 @@ int DbgPrintf(const char* szFormat, ...);
		} while (0)

		#define PVR_STRCPY(dest, source) do { strncpy(dest, source, sizeof(dest)-1); dest[sizeof(dest)-1] = '\0'; } while(0)

code cleanup #69

code cleanup #69

Conversation

fuzzard commented Nov 12, 2017

ksooo Nov 12, 2017

Choose a reason for hiding this comment

ksooo Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

ksooo Nov 12, 2017

Choose a reason for hiding this comment

ksooo Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

fuzzard commented Nov 12, 2017 • edited Loading

ksooo left a comment

Choose a reason for hiding this comment

fuzzard commented Nov 13, 2017

fuzzard commented Nov 21, 2017

fuzzard commented Dec 3, 2017

fuzzard commented Dec 5, 2017

fuzzard commented Dec 9, 2017

ksooo commented Dec 9, 2017

ksooo left a comment

Choose a reason for hiding this comment

ksooo Dec 11, 2017

Choose a reason for hiding this comment

ksooo Dec 11, 2017

Choose a reason for hiding this comment

ksooo Dec 11, 2017

Choose a reason for hiding this comment

ksooo Dec 11, 2017

Choose a reason for hiding this comment

fuzzard commented Dec 11, 2017

ksooo Nov 12, 2017 •

edited

Loading

ksooo Nov 12, 2017 •

edited

Loading

fuzzard commented Nov 12, 2017 •

edited

Loading