72 replies to this topic

[littlesat]

When you dona merge request via git you already spend of time,!therefore it is better to discussnit here first... I gave all comments very polite on GitHub and As I merged it first I also showed good intentions...

Edited by littlesat, 8 April 2018 - 14:01.

[IanSav]

Hi Littlesat,

 

I have still not been told what is wrong with the recent submissions.  There is NO useful information in GitHub.

 

The Setup rewrite was started more than a year ago.  It was always going to be written.  It solves a number of issues for skin developers and makes writing and maintaining skins *much* easier.  I also took the opportunity to make some user interface and performance improvements along the way.  I also wrote some documentation to help others learn about Enigma2 and help improve and standardise future submissions.  All the changes were ultimately to improve performance and benefit end users.

 

It was suggested to me that I submit it to OpenPli as it would then feed down to all the images that take code from OpenPLi etc.  Image maintainers are very troubled by all the merge issues when taking code from higher level repositories.  This submission was an example of how it could be done without breaking anything and making all downstream merges significantly easier.  This won't always be the case but it is a good goal for all Enigma2 development.

 

Some of the comments I have read seem to indicate that the biggest issue is that the code looks like code in other images.  A sentiment expressed in comments like "why must Pli to match OpenViX and Beyonwiz images" is typical.  There appears to be no thought about how the code may actually improve OpenPLi.  It still seems to me like OpenPLi like to complain about other images not feeding back to OpenPLi but when such code changes are suggested comments like "why must Pli to match OpenViX and Beyonwiz images" appear.  Damned if you do and damned if you don't.  In all cases you are always damned!

 

Does anyone have any positive suggestions on how to move forward?

 

Regards,

Ian.

Edited by IanSav, 8 April 2018 - 14:29.

[littlesat]

there is a lot of burden in the code to match the compatibility. First vix need to change the menu path stuff and also e.g. the branding stuff before such commit meet pli standard. In addition I do not see or are able to verify the perfomance inprovement. In addition as far I can see we need two other changes as far I can see due to the setup.xml object thetnwas created for setup.xml for plug-ins
But the biggest trigger for the revert was the branding stuff

Edited by littlesat, 8 April 2018 - 15:17.

[IanSav]

Hi Littlesat,

there is a lot of burden in the code to match the compatibility. First vix need to change the menu path stuff and also e.g. the branding stuff before such commit meet pli standard. In addition I do not see or are able to verify the perfomance inprovement. In addition as far I can see we need two other changes due to the setup.xml object thetnwas created for setup.xml for plug-ins.

If you didn't understand something you could have asked me.

 

I don't convert or manipulate either menu path mechanism.  I simply use whichever one is available on the image upon which the code runs.  This is done with a single getattr() call.

 

For performance improvement on OpenPLi instead of processing the XML setup menu structure for every menu entry lookup I create a dictionary of all menu titles when setup.xml is first loaded.

 

Regards,

Ian.

[littlesat]

Yes abd that getattr call must be out there for openpli... or vix and the other image need to adapt to make it compatible... we want to keep our code clean and then these kind of checks should be avoided
Processing xml directly is less performance then cashing them as they are in fact already cashed...

Edited by littlesat, 8 April 2018 - 15:27.

[IanSav]

Hi Littlesat,

 

You seem value code cleanliness.  So do I.  I believe my code is clean and optimal.  If there are further optimisation to be made then please point them out.  If your only concern was the little bit compatibility code then why didn't you simply ask for another commit update to remove the compatibility code?

 

It is the middle of the night here so I don't want to start a range of new tests now but let me share some of my previous timing tests.  These tests were for OpenViX but there were also savings on OpenPLi.  These tests were all performed on the same Beyonwiz T3 hardware.  This test involves starting at Live TV then navigating the menu system to get to the Graphica EPG settings. The steps were done twice to average results.  The getSetupTitle timing is the time to get the title strings for the menu.  The Setup run times are the time to generate the on screen Setup menu.

This is the timing tests on the original code:

< 34242.503> [Setup] PROFILE: getSetupTitle run time = 43.570995 msec.
< 34243.400> [Setup] PROFILE: getSetupTitle run time = 44.252872 msec.
< 34249.252> [Setup] PROFILE: getSetupTitle run time = 44.991016 msec.
< 34250.265> [Setup] PROFILE: getSetupTitle run time = 40.044069 msec.
< 34260.195> [Setup] PROFILE: Setup run time = 415.024042 msec.
< 34268.511> [Setup] PROFILE: getSetupTitle run time = 42.843103 msec.
< 34269.355> [Setup] PROFILE: getSetupTitle run time = 48.081160 msec.
< 34274.979> [Setup] PROFILE: getSetupTitle run time = 49.048185 msec.
< 34276.088> [Setup] PROFILE: getSetupTitle run time = 39.834023 msec.
< 34288.812> [Setup] PROFILE: Setup run time = 382.602930 msec.

This is the same test sequence using the new Setup.py code:

< 33259.360> [Setup] XML source file: '/usr/share/enigma2/setup.xml'
< 33259.413> [Setup] PROFILE: getSetupTitle run time = 53.852081 msec.
< 33260.565> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33260.566> [Setup] PROFILE: getSetupTitle run time = 1.101017 msec.
< 33269.628> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33269.629> [Setup] PROFILE: getSetupTitle run time = 1.004934 msec.
< 33270.935> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33270.936> [Setup] PROFILE: getSetupTitle run time = 1.139879 msec.
< 33284.148> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33284.149> [Setup] XML Setup menu 'epggraphical'
< 33284.243> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33284.243> [Setup] XML Setup menu 'epggraphical'
< 33284.334> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33284.335> [Setup] XML Setup menu 'epggraphical'
< 33284.420> [Setup] PROFILE: Setup run time = 272.813797 msec.
< 33293.185> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33293.186> [Setup] PROFILE: getSetupTitle run time = 1.102924 msec.
< 33294.314> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33294.315> [Setup] PROFILE: getSetupTitle run time = 1.204014 msec.
< 33299.970> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33299.971> [Setup] PROFILE: getSetupTitle run time = 0.984907 msec.
< 33301.130> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33301.131> [Setup] PROFILE: getSetupTitle run time = 1.039028 msec.
< 33310.784> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33310.785> [Setup] XML Setup menu 'epggraphical'
< 33310.874> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33310.875> [Setup] XML Setup menu 'epggraphical'
< 33310.971> [Setup] XML cached source file: '/usr/share/enigma2/setup.xml'
< 33310.972> [Setup] XML Setup menu 'epggraphical'
< 33311.068> [Setup] PROFILE: Setup run time = 285.242081 msec.

Please note that the new code resulted in significant execution time savings. In OpenPLi the setup.xml file is only read once at startup so this savings are not as dramatic but they exist.  The new caching code will allow developers to edit and test setup.xml without needing to restart Enigma2 for every test / change.  This is a feature enhancement for OpenPLi developers without any significant cost.

 

The first menu lookup takes a little longer as the dictionary is created but EVERY single lookup until the next restart is a simple dictionary lookup that is significantly faster than searching the XML.  The saving is that I only reprocess the XML code once on every setup.xml load and then simply return the cached titles on all future look ups.

 
Regards,
Ian.

 

[Huevos]

Hi Betacentauri,

Reverting your code doesn’t mean that we don’t want any contribution from other developers!
Why all? I don’t know but maybe there was no time to fix the code or there are issues that have to be discussed how to do it. Maybe read the comments in the commits like here (https://github.com/O...c4877fb49b631e7) and start a discussion with littlesat, ims, taapat, ... why it’s a good idea to do it in this way.

I think I have made 4 or 5 pull requests to OpenPLi.  Do you know how many have stayed in OpenPLi?  NONE.  As far as I am aware none of the code was faulty or caused any issues.  They were just changes that OpenPLi don't understand or want.  There appears to be no regard for what improvements they may have offered to others.

 

There is no place to interact with OpenPLi developers as that area of the forum is closed to outsiders.

 

Regards,

Ian.

Ian, you need to calm down. I explained to you when I saw the PR that it should only contain PLi specific code as it is for their enigma2. You should have closed the PR then, but you chose not to listen.

 

Also just in the last few days PLi has accepted 2 commits of your work, i.e. the graphical switches and the config.attribute commit, even though initially this caused a crash in OpenWebIF. Once that was sorted the commit was reinstated.

 

And as for PLi not accepting code from other images that too is not true. I have pushed a lot of code to PLi from OpenViX over the last 7 or 8 years and most of it has been accepted. Yes, some of my suggestions have been rejected, but in the end we must accept it is their image and their decision what they will accept and what they reject.

 

And lastly some of my code has been rejected on the first submission and accepted later.

 

@OpenPLi team, we could really do with this developer area being sorted out, and that external developers also have access to developer images rather than being forced to build there own.

[littlesat]

So you make the code more complicated only for when you make edits to setup.xml.... I do not see the real need. Developers can simply restart e2... no need for reload complicafions....

Edited by littlesat, 8 April 2018 - 17:40.

[IanSav]

Hi,

 

I can assure you all that if I were not calm you would certainly know it!  These sort of comments don't help move things forward.

 

So once again we have another we always do it that way excuse.  As a developers I waste far too much time rebooting systems when a reboot is totally unnecessary.  But I am sure that I can make my life easier and save lots of time by constantly repeating to myself "don't worry we always do it that way".  Hang on, why are we using PVR/DVR devices anyway, we all used to have VCRs  why change, they were so much easier and NEVER crashed.

 

Do you see my point?  It is often helpful to consider things from other perspectives.  Sometimes other opinions could improve things you never considered.

 

By the way, *still* no details or explanation for the code reversion.

 

Regards,

Ian.

[littlesat]

I think you receiver sufficiënt feedback ...

Edited by littlesat, 9 April 2018 - 06:50.

[IanSav]

Hi Littlesat,

I think you receiver sufficiënt feedback ...

Where?  I am still unaware of any reason or justification for all these reversions and hostile comments.

 

Regards,

Ian.

Edited by IanSav, 9 April 2018 - 07:42.

[littlesat]

Read this thread alone....

[IanSav]

Hi,

I have been in communication with WanWizard who has kindly extracted the relevant items from the OpenPLi developer member only area of the forum for me to review as I do not have access to that area of the forums.

From what I have read, it appears to me that my code did NOT crash or cause failures as I had been led to believe via a comment in a private message to me.
 
That being said, from what I can tell, it appears that there were four areas with which people had issues, these being:

• Some code was rejected because it was not optimised.
   
• Some code was rejected because it contained code that, although it would make merging easier with other images, added code not specific to OpenPLi to which OpenPLi developers took objection.
   
• Some code was rejected because developers didn't fully understand the value of their inclusion.
   
• The proposal was not discussed with OpenPLi developers before the submission was made.

 

I will now endeavour to address each of the above points from my perspective:

 

1)

The code I replaced was:

def init(self, contexts = [ ], actions = { }, prio=0):

The code that was rejected for not being optimised is actually correct and fixes the original code.  The original and optimised version of the code, as required by the OpenPLi developers, is syntactically correct but does NOT do what is expected or required!

The code I wrote was:

    def init(self, contexts=None, actions=None, prio=0):
        if not contexts:
            contexts = []
        if not actions:
            actions = {}

I was told that the extra code was not necessary and unwanted.  The fact is that while both are syntactically correct, the original code does not work correctly and does not do the same thing as the correct replacement code. 

Please refer to http://pylint-messages.wikidot.com/messages:w0102 and https://nedbatchelder.com/blog/200806/pylint.html for details.

In essence the problem with the original code is it appears to be an efficient way to default an empty list and dictionary.  What actually happens is that these items get created as a persistent objects.  Every invocation of the “init” method that doesn't specify the parameters will be use that same (persistent) objects.  Any changes to either object will persist and be carried to every other invocation!

 

2)

It is a shame that OpenPLi do not wish to consider making more Enigma2 code interchangeable between images.  I am sure that the OpenPLi developers know that the OpenPLi image is used as an upstream feed for many Enigma2 images.  Making code more mergeable can only be a good thing for the Enigma2 community.  That said, I acknowledge that this image is OpenPLi's to do with as they please.

 

In my opinion a more productive and conducive way to handle this  issue (or any issue) would have been to not merge the code but add comments to the pull request to ask for amendment commits to remove the offending code.  The act of merging and then reverting the code sent a signal to me, and possibly others looking at the commit logs, that I had done something wrong and that there was something wrong with the code.  Without detailed supporting comments in the reversion commit the exact problems was anyone's guess.  The fact that the only details were hidden in an OpenPLi developers only area of the forum only added more frustration for me because I was not seeing the full picture. 

 

3)

There was one comment from an OpenPLi developer that didn't think much of the option to sort Setup.py based menu items alphabetically.  I am concerned that one developer not understanding the value of an option is sufficient cause to eliminate a large selection of other changes and improvements.  If the particular option was so objectionable that it needed to be removed then that should also have been a comment added to the original pull request so that the feature could be explained and discussed.  If the feature still has to go then, again, a modifying commit could be made so that the rest of the pull request could be accepted.

 
4)

One of the other criticisms I received was that I had not talked to OpenPLi developers about my changes.  If the only place to do this is in the closed off developers area, then it makes it nigh impossible to do so. It also appears, from my own experience, that using the public forums for such things is frowned upon.  Therefore to remove this catch 22 scenario there really needs to be a way for non OpenPLi core developers to contribute to discussions and/or code for discussion, review and hopefully inclusion.  Resources like:

• Submission guidelines
• Code rejection guidelines
• Area to discuss proposed changes/fixes

 would go a long way to address this issue.

 

I believe that the main issue behind this whole incident is a lack of appropriate communication.

 

It would be beneficial to all involved if code submission guidelines and/or requirements can made available to anyone wanting to become involved in submitting code to OpenPLi.  Additionally it would also be constructive for contributors to receive a formal code rejection message detailing exactly why the code was rejected and what will be required to make the code acceptable.  There needs to be meaningful communications between all parties.
 
If there are issues with commits or pull requests the issues should be raised and managed in GitHub.  Having pointers within GitHub pointing to a forum that requires separate authentication is not conducive to GitHub users following the narrative relating to the commit or pull request.  GitHub should have the whole story in the one place.  Further to this, having links in GitHub that point to private / developer member only area of the forum is inappropriate and causes problems exactly like what happened here.

I feel OpenPLi members need to be aware that when the same questions are repeated in a short period of time in the same thread then perhaps the person asking the questions doesn't realise that the question has been answered or perhaps didn't understand the answer.  They are not doing it to annoy people.  Rather than getting frustrated with each other, efforts should be made by all parties involved to read beyond the words and look at the underlying misunderstanding/point of confusion then rephrase the question or reply, whichever as appropriate.
 
So now, what is the way forward?
 
As for the work I have already done, I believe that the code I want to submit is generally understood.  I am prepared to modify the code to remove all support for other images.  Would this make my contribution acceptable to OpenPLi?

Regards,
Ian.

 

Edited by IanSav, 14 April 2018 - 12:58.