search

google / sagetv

SageTV is a cross-platform networked DVR and media management system
http://forums.sagetv.com/
Apache License 2.0
267 stars 173 forks source link

SageConstants -- more cleanup #328

Open stuckless opened 7 years ago

stuckless commented 7 years ago

public static final boolean LITE = false; public static final boolean ENFORCE_EMBEDDED_RESTRICTIONS = false; public static final boolean PVR = true; public static final boolean SERVER_FUNCTION = true; public static final boolean LIBRARY_FUNCTION = true;

ENFORCE_EMBEDDED_RESTRICTIONS is never read, so it can be removed.

All other constants here are read in MANY places but the contant is never updated (and it's final, so, it can't be changed), so it begs the question... Should we remove these?

This is again, in the spirit of clean up. There is lots of if/else blocks using these constants. But if they never change, can/should they be removed? It may help to understand what the function of each of these was supposed to be.

enternoescape commented 7 years ago

I think some of these would no longer be needed once we make things more modular.

stuckless commented 7 years ago

I agree. I actually started out in the "modular" approach. I spent about week splitting things up, but I kept getting caught on direct access to many constants, like Sage.DBG and all the static access methods in Sage that it quickly became a mess for me. So then I thought, that maybe the first step should be to clean up some stuff, then refactor some things, and the modularize the code.

My next thing will be to break Sage.java into a bunch of utility classes where SOME of them can be in a "shared". It amazing when you go to move code, say in the MiniClient to a new project, how many access points it has into Sage.java.

Narflex commented 7 years ago

You can remove all these other constants you mention.

Jeff Kardatzke Sent from my Android

On Jul 1, 2017 9:04 AM, "Sean Stuckless" notifications@github.com wrote:

I agree. I actually started out in the "modular" approach. I spent about week splitting things up, but I kept getting caught on direct access to many constants, like Sage.DBG and all the static access methods in Sage that it quickly became a mess for me. So then I thought, that maybe the first step should be to clean up some stuff, then refactor some things, and the modularize the code.

My next thing will be to break Sage.java into a bunch of utility classes where SOME of them can be in a "shared". It amazing when you go to move code, say in the MiniClient to a new project, how many access points it has into Sage.java.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/google/sagetv/issues/328#issuecomment-312440303, or mute the thread https://github.com/notifications/unsubscribe-auth/ANEIDPYtnPzCXCopE_Zi2zsihKJkySwjks5sJm4GgaJpZM4OLVxM .

stuckless commented 7 years ago

@Narflex - I'm guessing there was an intention somewhere to create a "lite" version (whatever that means) or an NON PVR version? Just curious what the intention was going to be here with these constants.

Narflex commented 7 years ago

Ahhh...memories. :) Here's why they each exist:

public static final boolean LITE = false;

We did have a product called SageTV Lite at one point. It was for OEMs to bundle with hardware. I know we at least bundled it with the Plextor TV 100U for Windows, and that may have been it. It had more limited functionality to entice people to pay for an upgrade to a full version. That device also did MPEG4 encoding which is why you'll see various references in the code to DivX or MPEG4 stream types for recording. The Lite version also only had 3 days of guide data (so the EPG server recognized the difference based on the version string sent to it) and it also did not have Imported Media functionality.

public static final boolean ENFORCE_EMBEDDED_RESTRICTIONS = false;

This was for testing EMBEDDED JAR builds on non-embedded platforms. I noticed you removed a reference to this I had left in which was one of our little 'crack' checks where we'd make it misbehave if was running in a way it shouldn't have (i.e. pirated/cracked software). I meant to remove all of these before it went open source. There were various things in the code that would cause odd behavior if we detected cracked software like dropping random recordings, locking up randomly....and my personal favorite was randomly deleted items from the running STV which you probably saw various forums posts about over the years where people complained that items would just randomly disappear from the UI. :D

public static final boolean PVR = true;

This was for the HD200/300 since those ran a Sage.jar file but didn't have PVR functionality. Although we did actually build test ones that included PVR, so you could actually hook up a USB drive and USB capture device to an HD300 and load up an alternate STV and actually use it as a standalone DVR. I don't recall if we actually turned this flag on for the production firmware or not...but we never publicized that capability since it was somewhat hacky.

public static final boolean SERVER_FUNCTION = true;

I think this was something else we limited with the Lite version where you couldn't use it as a server...another reason to entice people to upgrade to the full version.

public static final boolean LIBRARY_FUNCTION = true;

I know this was for the Lite version where we didn't allow imported media, again to entice upgrades.

stuckless commented 7 years ago

@Narflex thanks for that bit history. Always good to understand history and intention of these things.

stuckless commented 7 years ago

Added Pull Request #329 to remove LITE (still need separate pull requests for the others)

Narflex commented 7 years ago

Wizard.COMPACT_DB could also be removed.

jusjoken commented 7 years ago

Any risk these are used in the STV in some direct or indirect manner?

Narflex commented 7 years ago

The APIs which return the values these constants represent shouldn't be removed....but the constants have 'constant' values now and all we are doing is forcing them all to that value and then removing the dead code around them.

No one should have been trying to use Java code in the STV to directly access these since nobody outside of internal SageTV devs would have cared about their values. So we should be all good. ☺️

Jeff Kardatzke Sent from my Android

On Jul 3, 2017 7:06 PM, "Ken Birch" notifications@github.com wrote:

Any risk these are used in the STV in some direct or indirect manner?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/sagetv/issues/328#issuecomment-312763142, or mute the thread https://github.com/notifications/unsubscribe-auth/ANEIDH15CEw1weQUjgd8vVgtsIlbYobJks5sKZ4PgaJpZM4OLVxM .

stuckless commented 7 years ago

How do you suggest I handle this code

outStream.write(("WIZARD_SYNC2 " + Integer.toString(Wizard.VERSION & 0xFF) + " " + Boolean.toString(Wizard.COMPACT_DB) + "\r\n").getBytes(Sage.BYTE_CHARSET));

Should COMPATCT_DB be removed as a parameter, or, should it just send false (and then do nothing with it on the other side)

Narflex commented 7 years ago

I'd remove the parameter altogether from that call and then increment the client compatible version in Sage.java to match what the new version # is associated with this change....make sure you do both in the same pull request.

On Tue, Jul 4, 2017 at 3:47 AM, Sean Stuckless notifications@github.com wrote:

How do you suggest I handle this code

outStream.write(("WIZARD_SYNC2 " + Integer.toString(Wizard.VERSION & 0xFF) + " " + Boolean.toString(Wizard.COMPACT_DB) + "\r\n").getBytes(Sage.BYTE_CHARSET));

Should COMPATCT_DB be removed as a parameter, or, should it just send false (and then do nothing with it on the other side)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/sagetv/issues/328#issuecomment-312847259, or mute the thread https://github.com/notifications/unsubscribe-auth/ANEIDPeQiYwY_QUr0-bPvFbzOYdXqyiRks5sKhhRgaJpZM4OLVxM .

-- Jeffrey Kardatzke jkardatzke@google.com Google, Inc.

Narflex commented 7 years ago

If you want you can also remove those couple places where Sage.q gets updated with the comment 'piracy protection'. That's updating the array that already gets set properly in Sage.java (and is modifying it in case someone pirated the software in a way that just used a new DB key so this would then end up likely corrupting their DB....unless it was all zeros, lol).

stuckless commented 7 years ago

Can Sage.q be removed? I was looking at Sage.q the other day... I was unsure if we could remove it or not. What does the "q" stand for?

Narflex commented 7 years ago

It needs to stay for people that upgrade from V7. q is just an arbitrary name so it looked like it was just some other obfuscated code...although I had that one disabled in the obfuscator because I was calling it from the EXE to set the DB encryption key (and the EXE was protected with anti-crack software and the normal licensing we had).

On Wed, Jul 5, 2017 at 10:31 AM, Sean Stuckless notifications@github.com wrote:

Can Sage.q be removed? I was looking at Sage.q the other day... I was unsure if we could remove it or not. What does the "q" stand for?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/sagetv/issues/328#issuecomment-313172389, or mute the thread https://github.com/notifications/unsubscribe-auth/ANEIDK44wuoqwU23Pg9bRTzMK_I-2giYks5sK8iLgaJpZM4OLVxM .

-- Jeffrey Kardatzke jkardatzke@google.com Google, Inc.