Jump to content


Photo

merge requests for PLi's git


  • Please log in to reply
1748 replies to this topic

Re: merge requests for PLi's git #301 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 12:06

That might be problematic, on a different git where I did the work originally I still have big chunks in commits, what I did was m_ prefixed all variables, as I could not distinguish easily in the code a class variable from a local, then added chunked transfer and filtration and some wrappers rework. So what you're asking here it is doable but very time consuming. I have the code running on my dream and amico alien, I'm an IPTV heavy user, so the code is very well tested in the real day to day use. 

 

So, to reiterate, I will not do small commits in the near future as the time for me is at the premium, as well I do understand your concerns, so I do not mind if you will not merge the change.

One last argument I'd have is that  1st service as is now in the openpli is not usable, so you wont loose anything if you merge my changes, as well I'd be happy to look at any issues that might arise in the future with the code.


Edited by zakalibit, 6 November 2013 - 12:07.


Re: merge requests for PLi's git #302 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 6 November 2013 - 12:07

Then I think we must consider importing the patch locally (not on sourceforge) and then we split it up somewhat and then commit it.


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #303 littlesat

  • PLi® Core member
  • 57,062 posts

+698
Excellent

Posted 6 November 2013 - 12:13

But can you at least look at the white spaces...

 

WIth those we mean extra spaces that are not required at the end of lines of codes.... empty lines with no code and spaces.... and mixed up taps/spaces...

 

Examples below, with some speudo code where I replace the spaces by underscores...

 

if (x == y)_________

{

____________________

   x = y;

}__________________________

 

This is a simple method to keep code clean.... ;)

 

And indeed it is better to have patches "splintered" into functional changes.


Edited by littlesat, 6 November 2013 - 12:14.

WaveFrontier 28.2E | 23.5E | 19.2E | 16E | 13E | 10/9E | 7E | 5E | 1W | 4/5W | 15W


Re: merge requests for PLi's git #304 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 6 November 2013 - 12:14

Also a little bit of coding style would be appreciated. I saw a line like this: "if (a) do_b;". This is not considered good practise. Please split it into two lines.


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #305 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 12:25

Also a little bit of coding style would be appreciated. I saw a line like this: "if (a) do_b;". This is not considered good practise. Please split it into two lines.

this is matter of taste, but in my opinion good practice is for one liner if statement:

 

if(a) do_b;

 

or

 

if(a) {

    do_b;

}

 

i'd like to hear your argument why oneliner if statement is not a good practice :) I want to hear practical arguments :)

 

 

sorry about the white spaces, it is probably a copy paste artifact, will fix them



Re: merge requests for PLi's git #306 littlesat

  • PLi® Core member
  • 57,062 posts

+698
Excellent

Posted 6 November 2013 - 12:31

The white spaces are most time something the editor does induce... e.g. a good editor makes the correct intends for your... but in between it also creates sometimes unintended indents...


WaveFrontier 28.2E | 23.5E | 19.2E | 16E | 13E | 10/9E | 7E | 5E | 1W | 4/5W | 15W


Re: merge requests for PLi's git #307 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 12:32

And indeed it is better to have patches "splintered" into functional changes.

I hear you :) I'd have the same reaction for sure, but when was writing the code I was not thinking about this :) just was pissed of the way the streams were played, I spent a lot of time to come-up with this code, I had few other worse incarnations of this code with circular buffers and reconnect feature, but after long time staring at the enigma logs I realized that they give more trouble the benefit :) so the current version is the bare minimum ( well, if you don take the cosmetics in consideration :) )


Edited by zakalibit, 6 November 2013 - 12:35.


Re: merge requests for PLi's git #308 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 12:58

fixed whitespaces and indentation

https://sourceforge....ge-requests/26/

https://sourceforge....22b2252501e339/



Re: merge requests for PLi's git #309 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 13:00

Actually the best to review the code is not to look at the diff but at the code it self, I reckon peterg is very familiar with the httpstream code, so he can look at my version of file, should not be hard to understand my code, there is nothing too complex



Re: merge requests for PLi's git #310 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 6 November 2013 - 13:03

Also a little bit of coding style would be appreciated. I saw a line like this: "if (a) do_b;". This is not considered good practise. Please split it into two lines.

this is matter of taste, but in my opinion good practice is for one liner if statement:

...

i'd like to hear your argument why oneliner if statement is not a good practice :) I want to hear practical arguments :)

 

There is nothing practical about sharing code in a project. It's not about that I like it this way (or not), it's about a common coding style. Have you ever tried to read/change a project that has many different styles through de code? That's not something you want.

 

BTW idents we do with tabs of four "spaces", not spaces. I don't know if you already adhere to that.


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #311 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 13:14

There is nothing practical about sharing code in a project. It's not about that I like it this way (or not), it's about a common coding style. Have you ever tried to read/change a project that has many different styles through de code? That's not something you want.

 

BTW idents we do with tabs of four "spaces", not spaces. I don't know if you already adhere to that.

 

Ok, I see no point having an argument over this,  just one thing to keep in might is coding style has 2 aspect practical and cosmetic, which is a matter of taste, the practical one is about code readability and how much code you can see on one page on the screen, this is how I see it.

 

I'll retab the code :) this is another one that I'd disagree, it is a horizontal space waster.


Edited by zakalibit, 6 November 2013 - 13:15.


Re: merge requests for PLi's git #312 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 6 November 2013 - 13:19

I'll retab the code :) this is another one that I'd disagree, it is a horizontal space waster.

I don't understand?

 

You mean you're using some editor that cannot configure the tab width?


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #313 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 13:20

I'll retab the code :) this is another one that I'd disagree, it is a horizontal space waster.

I don't understand?

 

You mean you're using some editor that cannot configure the tab width?

 

you're right I've over-exaggerated on this one :)



Re: merge requests for PLi's git #314 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 13:21

ok converted to tabs https://sourceforge....e9031c1a89495d/



Re: merge requests for PLi's git #315 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 6 November 2013 - 13:22

I'm a great fan of tabs for indenting:

- every coder can configure their favourite tab width locally

- every indent is one character in the code, easier for editing, finding etc. imho


Edited by Erik Slagter, 6 November 2013 - 13:23.

* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #316 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 13:27

I'm a great fan of tabs for indenting:

- every coder can configure their favourite tab width locally

- every indent is one character in the code, easier for editing, finding etc. imho

ok you sold it to me :)



Re: merge requests for PLi's git #317 pieterg

  • PLi® Core member
  • 32,766 posts

+245
Excellent

Posted 6 November 2013 - 15:12

what I did was m_ prefixed all variables, as I could not distinguish easily in the code a class variable from a local

The convention used in that particular piece of code is such that local variables don't use capitals, and class members use camelcase (with a low-case first character).
I admit e2 is a bit of a mess, with different naming conventions.
I usually stick to the naming conventions of the particular class I'm working on, with a slight deprecation of the Hungarian style, because that reminds me of Windows programmers (no offense).
So if you have to change the naming convention, change it for the whole class at once, in a seperate commit.
Please don't mix cosmetics with functionality, in a single commit.

Also, those int-->size_t changes are more or less cosmetic, because they don't fix actual bugs (there are no signed/unsigned mixups afaics).

With that out of the way, I'm very interested in your fixes.
Please add a detailed description of what you are changing, and why you are changing it, in each of your commits.
Then we'll be more than happy to merge or cherrypick your changes.

Re: merge requests for PLi's git #318 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 15:59

I'm not sure I'll do that soon, all you say makes sense :) but you know you do not think about all this while having a beer in the evening and trying to make streaming working properly :) and I wasn't planing to push it in to pli in the first place, I just fixed for my self at the time, so I can enjoy watching iptv. 

Let me digest all this and see if I'll get in to the mood of redoing all this.



Re: merge requests for PLi's git #319 theparasol

  • Senior Member
  • 4,157 posts

+198
Excellent

Posted 6 November 2013 - 16:28

Proof of concept is done.
Perhaps someone else can do the wanted recoding?

@Camping: ZGemma H.2S, Technisat Multytenne 4-in-1 @Home: Edision Mini 4K, Wave Frontier T55, EMP Centauri EMP DiSEqC 8/1 switch, 4x Inverto Ultra Black single LNB


Re: merge requests for PLi's git #320 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 6 November 2013 - 17:20

Proof of concept is done.
Perhaps someone else can do the wanted recoding?

I could assist with code review and general help as time permits.




16 user(s) are reading this topic

0 members, 16 guests, 0 anonymous users