Config and VirtualKeyBoard enhancements

[IanSav]

Hi Prl,

I have attached my alpha level code for your review and comment. I welcome any suggestions or corrections you may care to offer. For others thinking about using this code, be careful! This is NOT production or even beta level code. Don't bother trying this out unless you have a good grasp of how Enigma2 works.

This code has been extracted from my active development system. I hope I have grabbed everything needed but if something is missing please ask.

These code changes require changes to the skin. The changes are documented in VirtualKeyBoard.py. Sample images to update a skin are included in the package (there are 720 and 1080 resolution versions of the images.)

Please note that this code is very much a work in progress. The VirtualKeyBoard code is feature complete but the language tables still need lots of work. The NumericalTextInput code is mostly complete but once I get a handle on all the language issues I suspect that this code will change slightly. (I am thinking that the on-screen SMS keyboard may need to change to allow for longer strings of characters required for non English languages.)

The config code is very raw and at the beginning of a very long change and rearrangement process. The ConfigBoolean and ConfigText areas are pretty complete but I am still working on some notification issues with Huevos that may result in further changes.

Please let me know what you think about the code and the direction I am taking.

Regards,
Ian.

[prl]

I have attached my alpha level code for your review and comment. I welcome any suggestions or corrections you may care to offer. For others thinking about using this code, be careful! This is NOT production or even beta level code. Don't bother trying this out unless you have a good grasp of how Enigma2 works.

I assume that for the Beyonwiz skins, 720/vkey_text.png goes into skin_dir, and the rest of 720/*.png go into skin_dir/buttons?

Is there a reason why the suggested <fonts> entries define a VirtualKeyBoard font alias, but the suggested skin doesn't use it?

Also, if anyone's puzzled why some source files appear to have almost all line different when you do a git diff, NumericalTextInput.py and VirtualKeyBoard.py use DOS-style CRLF line endings, but the other source files use Unix-style LF. If you don't want to change the line endings, you can use git diff -w instead.

[prl]

Please let me know what you think about the code and the direction I am taking.

I've installed the changes on my test U4 and with minimal testing they seem to be OK.

Could you give a rundown of the intention of the changes so that I have something more of a starting point for testing than "test every possible action in VirtualKeyboard and Config*".

There doesn't seem to be a devel branch for the changes in your easy-ui-4 repository clone, so I can't follow the progress or commit notes there.

Also, I now have a successful build of the OpenViX devel branch, so time spent on reviewing this code is time not spent in getting the genre list changes submitted to OpenViX.

[IanSav]

Hi Prl,

Sorry about the line endings. I can't seem to get SourceTree to leave the line endings alone.

All the vkey_ images can go into skin_default/buttons. When the code is submitted to the repository the default skin will be adapted to suit.

The font definition is actually used by the code itself not the skin.

Regards,
Ian.

[prl]

All the vkey_ images can go into skin_default/buttons. When the code is submitted to the repository the default skin will be adapted to suit.

Are you sure? That's not what this looks like (VirtualKeyBoard.py):

Code: Select all

# 		<ePixmap pixmap="vkey_text.png" position="10,35" zPosition="-4" size="630,50" alphatest="on"/>

i.e., not buttons/vkey_text.png.

Also, unless upstream puts the vkey_images inside skin_default, the Beyonwiz code can't put them there: they have to go into the individual skins, exactly as the recently added mark_on.png/mark_off.png pixmaps do.

[prl]

Sorry about the line endings.

Not a real problem, and already fixed. I just wanted to flag it to anyone else who tried it and was puzzled why all the lines in some files seem to have changed when they do a diff.

One thing that would make it easier for me would be if the source (.py & .xml) files in the zip had the correct relative paths e.g: lib/python/Screens/VirtualKeyBoard.py rather than VirtualKeyBoard.py. If it's hard to do that with your ZIP program, then a compressed tarball is also fine by me.

[IanSav]

Hi Prl,

All the vkey_ images can go into skin_default/buttons. When the code is submitted to the repository the default skin will be adapted to suit.

Are you sure? That's not what this looks like (VirtualKeyBoard.py):

Code: Select all

# 		<ePixmap pixmap="vkey_text.png" position="10,35" zPosition="-4" size="630,50" alphatest="on"/>

i.e., not buttons/vkey_text.png.

That is an error. Thank you for pointing it out. It has been fixed.

Also, unless upstream puts the vkey_images inside skin_default, the Beyonwiz code can't put them there: they have to go into the individual skins, exactly as the recently added mark_on.png/mark_off.png pixmaps do.

This change is being made in discussion with OpenPLi and OpenViX developers. If accepted the change will become global and the default skins will be updated. I hope to be able to make the submission very soon. I am trying to fix a few more languages before I make up the pull requests.

What do you think about the code changes in VirtualBeyBoard.py? (Compare my changes with the latest source from OpenPLi! ) Is there anything that could or should be improved or optimised?

Regards,
Ian.

[prl]

What do you think about the code changes in VirtualBeyBoard.py? (Compare my changes with the latest source from OpenPLi! ) Is there anything that could or should be improved or optimised?

Is there a strong reason why I should compare the changes with OpenPli rather than with the Beyonwiz code?

As for the other questions:

Could you give a rundown of the intention of the changes so that I have something more of a starting point for testing than "test every possible action in VirtualKeyboard and Config*".

There doesn't seem to be a devel branch for the changes in your easy-ui-4 repository clone, so I can't follow the progress or commit notes there.

And

I now have a successful build of the OpenViX devel branch, so time spent on reviewing this code is time not spent in getting the genre list changes submitted to OpenViX.

[IanSav]

Hi Prl,

Is there a strong reason why I should compare the changes with OpenPli rather than with the Beyonwiz code?

The suggestion for the comparison was just to indicate why all the effort for this rewrite. The recent changes made to the OpenPLi version are what triggered this rewrite. Their aim to improve the number of kemaps per language was good but the implementation was horrid.

As for the other questions:

Could you give a rundown of the intention of the changes so that I have something more of a starting point for testing than "test every possible action in VirtualKeyboard and Config*".
There doesn't seem to be a devel branch for the changes in your easy-ui-4 repository clone, so I can't follow the progress or commit notes there.

This code is very early alpha and subject to frequent changes. Until it stabilises a bit it won't go into the repository. VirtualKeyBoard was changed for the reason above and also to improve the UI. It will follow the UI designs for the new Setup that is also being developed. The changes to config are to fix the rampant over triggering of Notification events. There is also a bit of code bloat and code rot that will go as part of the clean up.

At this point the only item worth checking out is the ConfigText object. I don't want to distract you from other tasks but if you want to test out the changes I can also give you the test harness I wrote for the development and testing process.

And

I now have a successful build of the OpenViX devel branch, so time spent on reviewing this code is time not spent in getting the genre list changes submitted to OpenViX.

I offered the code for you to look at what I was changed so you could see how it may relate to your work on Input. You did ask for a copy for review. I only asked for your review as I value your input. If you don't have the time then that is okay. You can always comment when you have time. I would suggest you could comment when I file the pull request but PeterU wants me to submit these changes upstream so I haven't been filing Beyonwiz pull requests. It is a shame that those upstream acceptances are still not making it into the Beyonwiz code. (Maybe I will start submitting copies of the pull requests here so that Beyonwiz users don't get left in the past.)

Regards,
Ian.

[prl]

In the old code for ConfigElement::isChanged() self.saved_value is explicitly tested against self.saved_value is None, but in the new code the test is (effectively) bool(self.saved_value), which will be true in many more cases, in particular, it will be true if self.saved_value is "", or other values that test logically as False.

Also in ConfigElement::isChanged(), if self.saved_value is None, the test strv != sv will be False if sv is "None". This is a bug in both the old and new code.

A comment is added to the definition of self.callNotifiersOnSaveAndCancel to clarify its function, but is function is commented out wherever it is tested.

ConfigBoolean and friends have been moved from after ConfigSelection to before ConfigSelection, which makes diff useless to compare the two versions of ConfigBoolean. I'll cut/paste them back to their original positions to do the comparison. IMO the move should be done in a separate commit from any other changes to the file to make changes easier to track. But that doesn't help with comparing snapshots; it's only useful in a branch.

In general, it would have been good to be able to review the code as a series of commits rather than a single batch of changes.

Are asserts enabled in OpenPli? They aren't in Beyonwiz code.

What is the rationale for changing the key names? Also, where the legacy key names have the same value as the corresponding new names, perhaps it would be clearer to use:

Code: Select all

KEYA_LEFT = 0
...
KEY_LEFT = KEYA_LEFT

rather than

Code: Select all

KEYA_LEFT = 0
...
KEY_LEFT = 0

I'll continue looking at the code when I have time and post as I go.

[prl]

I've moved ConfigBoolean to a place in the code where it can be compared more easily. It all seems fine, though I'm not convinced that having both "Enable/Disable" and "Enabled/Disabled" as representations of ConfigBoolean is really necessary.

One combination that I really don't like for boolean options is "Enable some option: Enable/Disable" rather than "Yes/No" or a switch.

Now that ConfigYesNo uses "Yes/No" as iots options will the "yes/no" in MessageBox also be capitalised for consistency?

Is the intention to permanently remove the getHTML() method from ConfigBoolean? It is used in OpenWebif.

More generally, why are you catching BaseException rather than Exception? One, I think undesirable, side-effect of that is that choice is that it will catch the SystemExit exception, which is raised by sys.exit() and prevent exiting on a fatal error.

Having changed() called from setValue() only when the value has changed is a very good idea.

Is self.edited necessary? Can't wasEdited() be implemented as self.value != self.last_value?

It appears that methods have been moved around inside ConfigText, too, so I'll pause here and resume when I've made ConfigText more comparable with the Beyonwiz code.

Looking at NumericalTextInput...

I'd have thought that the MAP_HEXFAST* would be more logical as "0A", "1B", "2C"... rather than "0", "1A", 2B"... since Ahex = 10dec and not 11dec, and so on.

Other than that, it looks clean and a lot more flexible than before.

[IanSav]

Hi Prl,

In the old code for ConfigElement::isChanged() self.saved_value is explicitly tested against self.saved_value is None, but in the new code the test is (effectively) bool(self.saved_value), which will be true in many more cases, in particular, it will be true if self.saved_value is "", or other values that test logically as False.

Also in ConfigElement::isChanged(), if self.saved_value is None, the test strv != sv will be False if sv is "None". This is a bug in both the old and new code.

I will check this out in due course. Initially I have been working on ConfigText and ConfigBoolean. I am aware that there is much to check and fix in this project. I welcome your ongoing input to ensure that nothing is left overlooked.

A comment is added to the definition of self.callNotifiersOnSaveAndCancel to clarify its function, but is function is commented out wherever it is tested.

This is part of the failed OpenViX attempt to fix notifiers. I will be sorting this out shortly.

ConfigBoolean and friends have been moved from after ConfigSelection to before ConfigSelection, which makes diff useless to compare the two versions of ConfigBoolean. I'll cut/paste them back to their original positions to do the comparison. IMO the move should be done in a separate commit from any other changes to the file to make changes easier to track. But that doesn't help with comparing snapshots; it's only useful in a branch.

In general, it would have been good to be able to review the code as a series of commits rather than a single batch of changes.

This code and all the changes are taken from my development environment. Once the code is written and tested I will go back to the repository code and structure the changes into logical steps that will make sense as a series of repository updates. The code and changes are very experimental at the moment and subject to frequent changes and reversions.

Are asserts enabled in OpenPli? They aren't in Beyonwiz code.

I don't know. OpenPLi make it extremely difficult to maintain an up to date development environment. In general I look at OpenPLi issues at the end of the development cycle.

What is the rationale for changing the key names? Also, where the legacy key names have the same value as the corresponding new names, perhaps it would be clearer to use:

Code: Select all

KEYA_LEFT = 0
...
KEY_LEFT = KEYA_LEFT

rather than

Code: Select all

KEYA_LEFT = 0
...
KEY_LEFT = 0

This was done because of the confusion relating to exactly what the KEY_ values really represent. Most people I asked thought that these related to the KEY_ key code values. That is NOT correct. These values represent actions to be performed on the current item being edited. I believe that a name change will remove this confusion and eliminate any conflict if both values are required in any part of the code.

I need to keep both sets of values until such time that all code that gropes into ConfigList.py and config.py are either corrected or updated.

Your comment about the initialisation are valid and appropriate. I lazily used a copy and paste to get started. I will address this immediately.

I'll continue looking at the code when I have time and post as I go.

I would appreciate your interest, corrections and suggestions as I progress this significant refactor project.

Regards,
Ian.

[IanSav]

Hi Prl,

I've moved ConfigBoolean to a place in the code where it can be compared more easily. It all seems fine, though I'm not convinced that having both "Enable/Disable" and "Enabled/Disabled" as representations of ConfigBoolean is really necessary.

The moves will eventually make the list of classes in config.py more logical and grouped with their parent classes.

One combination that I really don't like for boolean options is "Enable some option: Enable/Disable" rather than "Yes/No" or a switch.

This should be tackled as a separate project to improve the language and grammar throughout Enigma2. There was a user who expressed interest in that task. I hope they will come forward and offer to resume their efforts.

Now that ConfigYesNo uses "Yes/No" as iots options will the "yes/no" in MessageBox also be capitalised for consistency?

It should be but may take me a while to get there. If I want a change of scenery I may break out and make that change sooner.

Is the intention to permanently remove the getHTML() method from ConfigBoolean? It is used in OpenWebif.

No. I was trying to identify if this was part of the deprecated HTML code within Enigma2. Now that you have pointed out the issue I will restore the code and add a note regarding its purpose.

More generally, why are you catching BaseException rather than Exception? One, I think undesirable, side-effect of that is that choice is that it will catch the SystemExit exception, which is raised by sys.exit() and prevent exiting on a fatal error.

This was a knee jerk change to address PEP8 errors for unqualified "Except" statements. Without investigating the matter too closely I simply selected the highest level exception by default. I am happy to use "Exception" if you believe it to be correct and more appropriate. My aim to to have as much of the code function in a stable manner without relying on exceptions.

Having changed() called from setValue() only when the value has changed is a very good idea.

Is self.edited necessary? Can't wasEdited() be implemented as self.value != self.last_value?

Probably. As I mentioned previously this code is very alpha. I am trying to get the functionality and ideas correct. Optimisation and clean up will come later down the track. When this code goes to beta status PLEASE comment and correct any inefficiencies that I have failed to address.

It appears that methods have been moved around inside ConfigText, too, so I'll pause here and resume when I've made ConfigText more comparable with the Beyonwiz code.

I am hoping to complete effort on ConfigText soon. It should be a model on the style and approach I hope to take for all the other classes in this module.

Looking at NumericalTextInput...

I'd have thought that the MAP_HEXFAST* would be more logical as "0A", "1B", "2C"... rather than "0", "1A", 2B"... since Ahex = 10dec and not 11dec, and so on.

I am open to change but I was going for making it easier on the fingers to quickly find and press the buttons required to enter HEX. I felt that the distance to move a finger to 0 was unwarranted. Also the layout I used looked better on the on-screen keypad.

If you still feel the arrangement I used should be changed then I will be happy to comply.

Other than that, it looks clean and a lot more flexible than before.

Thank you.

Regards,
Ian.

[IanSav]

Hi,

One of the issues that has been causing a delay in my work on this project is coming to an understanding of exactly how the notifiers *should* be working. To that end I would like to offer the following rationale to see if this would be the correct and appropriate way forward. This text, if acceptable, will become a standalone document in the /doc area of the repository.

Code: Select all

A Guide to Config Class Notifiers
---------------------------------
Immediate ("immediate_feedback=True") and Final ("immediate_feedback=False") 
notifiers should be considered as unrelated.  The "initial_call=True" or 
"initial_call=False" parameters simply control if the notifiers are triggered 
during the initialisation of the objects.  This control may be required to 
inhibit processing before the UI is initialised and/or displayed.

"Immediate" notifiers are only used during the editing process of a single 
item.  While an item is currently selected any change to the value should 
trigger an "Immediate" type notification.

"Final" notifiers are only used when the user leaves the currently selected 
item.  This notifier only triggers if the value is different from when the 
user entered the selected item.  It doesn't matter what happens to the item 
while the user is on that item.  It can be edited, deleted, manipulated in 
ANY way.  The test is simply if the item's value is different, at exit, from 
its value on entry then the "Final" notifier will trigger.  If the value is 
the same then the "Final" notifier will NOT trigger.

The onChange callback in ConfigList is similar to the "Final" notifier.  It 
is a generic event that triggers when the value of the currently selected 
item has changed from when the item was first selected.  It differs from the 
"Final" notifier in that onChange event applies to each and every item in the 
ConfigList.  It does not require specific creation / initialisation for each 
individual item in the ConfigList as would be required for the "Final" 
notifier.

What doe people think? If I have the concept correct I will start appling the principles across the changes in this refactor.

[EDIT: Correct spelling of rationale.]

Regards,
Ian.

[prl]

More generally, why are you catching BaseException rather than Exception? One, I think undesirable, side-effect of that is that choice is that it will catch the SystemExit exception, which is raised by sys.exit() and prevent exiting on a fatal error.

This was a knee jerk change to address PEP8 errors for unqualified "Except" statements. Without investigating the matter too closely I simply selected the highest level exception by default. I am happy to use "Exception" if you believe it to be correct and more appropriate. My aim to to have as much of the code function in a stable manner without relying on exceptions.

I only raised it because I hadn't seen a BaseException before and had to go and check what it was. I initially thought it might have been thrown from int() for some kinds of inappropriate input. BaseException isn't used anywhere else in the base Beyonwiz code, except Exception is used for cases where it's "let's try this and do some default if it blows up".

I'd have thought that the MAP_HEXFAST* would be more logical as "0A", "1B", "2C"... rather than "0", "1A", 2B"... since Ahex = 10dec and not 11dec, and so on.

I am open to change but I was going for making it easier on the fingers to quickly find and press the buttons required to enter HEX. I felt that the distance to move a finger to 0 was unwarranted. Also the layout I used looked better on the on-screen keypad.

If you still feel the arrangement I used should be changed then I will be happy to comply.

I appreciate the issue about the placement of '0', but it still feels odd to me to use '2' to enter '11' (etc).

Thank you.

No problem.

[prl]

One of the issues that has been causing a delay in my work on this project is coming to an understanding of exactly how the notifiers *should* be working. To that end I would like to offer the following rational to see if this would be the correct and appropriate way forward. This text, if acceptable, will become a standalone document in the /doc area of the repository.

Code: Select all

A Guide to Config Class Notifiers
---------------------------------
** handy description***

What doe people think? If I have the concept correct I will start appling the principles across the changes in this refactor.

That's how I've always assumed that they work. That assumption may not be entirely accurate about how they in fact do work

The only minor clarification I think of that should be in there is that exiting the settings screen is also considered "leaving" the setting as a trigger for a settings "Final" notifier.

Btw:
rational: Based on or in accordance with reason or logic. (and similar)
rationale: A set of reasons or a logical basis for a course of action or belief.

[IanSav]

Hi Prl,

The KEY_ definitions are now initialised to their KEYA_ values.

All BaseExceptions have been replaced by Exceptions.

Regards,
Ian.

[prl]

I've added the reply I intended to write about the key choice for HEX entry to my earlier reply. I don't know what happened there.

[IanSav]

Hi Prl,

That's how I've always assumed that they work. That assumption may not be entirely accurate about how they in fact do work

You know what they say about assumptions!

The fact is, notifications are/were a mess. OpenViX had a go at fixing them and failed. Huevos asked me to help sort things out. I decided that the whole file deserved some TLC to address the issues and improve the content.

The only minor clarification I think of that should be in there is that exiting the settings screen is also considered "leaving" the setting as a trigger for a settings "Final" notifier.

Document updated:

Code: Select all

A Guide to Config Class Notifiers
---------------------------------

Written by IanSav - 16-Jun-2018

This document explains the basis and use of Notifiers in the "config.py" 
based "ConfigXYZ" based classes.

Immediate ("immediate_feedback=True") and Final ("immediate_feedback=False") 
notifiers should be considered as unrelated.  The "initial_call=True" or 
"initial_call=False" parameters simply control if the notifiers are triggered 
during the initialisation of the objects.  This control may be required to 
inhibit processing before the UI is initialised and/or displayed.

"Immediate" notifiers are only used during the editing process of a single 
item.  While an item is currently selected any change to the value should 
trigger an "Immediate" type notification.

Actions that do NOT change the value of an item, like moving the cursor, 
selecting text or changing an operating mode, should NOT trigger a notifier!

"Final" notifiers are only used when the user leaves the currently selected 
item.  This notifier only triggers if the value is different from when the 
user entered the selected item.  It doesn't matter what happens to the item 
while the user is on that item.  It can be edited, deleted, manipulated in 
ANY way.  The test is simply if the item's value is different, at exit, from 
its value on entry then the "Final" notifier will trigger.  If the value is 
the same then the "Final" notifier will NOT trigger.

The onChange callback in ConfigList is similar to the "Final" notifier.  It 
is a generic event that triggers when the value of the currently selected 
item has changed from when the item was first selected.  It differs from the 
"Final" notifier in that onChange event applies to each and every item in the 
ConfigList.  It does not require specific creation / initialisation for each 
individual item in the ConfigList as would be required for the "Final" 
notifier.

Using "Save" or "Cancel" to exit the screen should also trigger the "Final" 
and "onChange" events for the item currently selected at the time the "Save" 
or "Cancel" button was pressed.

---END---

Btw:
rational: Based on or in accordance with reason or logic. (and similar)
rationale: A set of reasons or a logical basis for a course of action or belief.

I rely on spell checkers too much. It is a shame they can't work out what I meant to say.

[EDIT: Config notifier document updated to include information about non change actions.]

Regards,
Ian.

[prl]

"Loath" and "loathe" gets people all the time, too

[IanSav]

Hi Prl,

I've added the reply I intended to write about the key choice for HEX entry to my earlier reply. I don't know what happened there.

The solution is easy. I will offer both alternatives.

Regards,
Ian.

[IanSav]

Hi,

The notifier document above has been updated to add a comment relating to actions that don't change the text being changed to NOT trigger any notifications.

This is a significant problem in the original code.

Regards,
Ian.

[IanSav]

Hi Prl,

Is the intention to permanently remove the getHTML() method from ConfigBoolean? It is used in OpenWebif.

After you asked this question I went back to reaffirm why I commented out this code. Are you sure that this code is used in OpenWebif? I just search all of Enigma2 for the Beyonwiz, OpenViX and OpenPLi images and the OpenWebif source yet the ONLY place I can see this code defined / used is in "lib/python/Components/config.py" and "lib/python/Components/Sources/Setup.py". I could find no active use of this method!

Can you please clarify if you are aware of any use of getHTML or unsafeAssign anywhere in any code. If they are actually being used then shouldn't the definition also be in the base ConfigElement class? It is odd that the methods are selectively included in only a few derivative classes.

I still suspect that this is old legacy code that is now just bloat.

Regards,
Ian.

[prl]

Curious. It appears that OpenWebif uses JSON for data like current settings values.

However, getHTML() is used in enigma2-plugins vlcplayer.

But speaking of OpenWebif, in plugin/controllers/models/config.py, getJsonFromConfig() only works for specific named Config* classes. If you want ConfigEnabledDisabled to work with OpenWebif, you'll need to add it to getJsonFromConfig(). ConfigOnOff should probably also be added.

[prl]

OK, I've re-arranged ConfigText now so that I can compare it more easily to the Beyonwiz code.

In ConfigText.insertChar(), the parameters ch and owr have been renamed to char and sameChar. Why? I can't see anything wrong with ch, and if owr is to be changed, perhaps it should be to overwrite.

The corresponding argument names in ConfigMacText.insertChar() haven't been changed. They should probably be consistent, whichever names are chosen.

A number of config classes don't seem to call ConfigElement.onSelect() and ConfigElement.onDeselect() in their own onSelect() and onDeselect() methods. I'd have thought they should, or as least do something like what's in the base class.

In answer to the question in ConfigSlider.getMulti():

Code: Select all

		self.checkValues()  # #### Is this really required?

It doesn't look like it to me. The only thing I can think of is if ConfigSlider.getMulti() is called from an asynchronous thread, but I don't think it is, and if it can be, then there might need to be other sanity checks elsewhere in the config.py code, too.

[IanSav]

Hi Prl,

OK, I've re-arranged ConfigText now so that I can compare it more easily to the Beyonwiz code.

May I suggest that you also keep a copy of config.py in the shuffled order so that you can more easily track future changes without having to do the full reshuffle for code comparisons to see what is changing.

In ConfigText.insertChar(), the parameters ch and owr have been renamed to char and sameChar. Why? I can't see anything wrong with ch, and if owr is to be changed, perhaps it should be to overwrite.

I do this to clarify my understanding of the code. It is possible that the names may change again when the code is finalised. Remember this is early alpha code. Under normal circumstances this would never be seen. The code is usually cleaned up and finalised before it gets submitted to the repository.

The corresponding argument names in ConfigMacText.insertChar() haven't been changed. They should probably be consistent, whichever names are chosen.

So far I have only focused on ConfigText and the Boolean family. Identical changes will be made throught to code for consistency and uniformity once I find the code changes with which I am happy. All the changes will then be reviewed during the external code review.

A number of config classes don't seem to call ConfigElement.onSelect() and ConfigElement.onDeselect() in their own onSelect() and onDeselect() methods. I'd have thought they should, or as least do something like what's in the base class.

This is a significant problem with the existing code. I will be getting to ALL the classes in due time. I am focusing on only ConfigText and ConfigBoolean to work on getting the notifiers and general code style correct. Once I have these two sub-classes working and looking as desired I will start going through all the other classes in this file.

You will also note that the NumericalTextInput helper is also not consistently initialised in all the classes. Partial or inconsistent implementation is a common problem throughout the classes.

In answer to the question in ConfigSlider.getMulti():

Code: Select all

		self.checkValues()  # #### Is this really required?

It doesn't look like it to me. The only thing I can think of is if ConfigSlider.getMulti() is called from an asynchronous thread, but I don't think it is, and if it can be, then there might need to be other sanity checks elsewhere in the config.py code, too.

When I get back to this class that code may well go.

Thanks for your on-going reviews.

Regards,
Ian.

[IanSav]

Hi Prl,

Curious. It appears that OpenWebif uses JSON for data like current settings values.

However, getHTML() is used in enigma2-plugins vlcplayer.

I wonder if the getHTML would be better moved to enigma2-plugins vlcplayer where it is used rather than bloating up the Config classes?

But speaking of OpenWebif, in plugin/controllers/models/config.py, getJsonFromConfig() only works for specific named Config* classes. If you want ConfigEnabledDisabled to work with OpenWebif, you'll need to add it to getJsonFromConfig(). ConfigOnOff should probably also be added.

This is a good idea. I will try to remember to get this done. The scope of this whole project has spun out of control. It started as a refactor of Setup.py spread to ConfigList.py then to config.py and then to VirtualKeyBoard.py and NumericalTestInput.py. I will try to keep a handle on all the changes that need to be made. If you feel I am missing any changes when the pull requests are raised please let me know.

Regards,
Ian.

[prl]

However, getHTML() is used in enigma2-plugins vlcplayer.

I wonder if the getHTML would be better moved to enigma2-plugins vlcplayer where it is used rather than bloating up the Config classes?

On a bit of a closer look at vlcplayer, it appears that getHTML() should be deleted there. It doesn't seem to be called anywhere in vlcplayer.

Adding it to the config classes the plugin could be done, but it would either mean subclassing all the config classes used there, or (ugly) having vlcplayer add getHTML() methods to the classes as needed.

I'd go for throwing it out.

[prl]

What possesses people to write things like this!

Code: Select all

					if os_path.islink(testname):
						my_isLink = True
					else:
						my_isLink = False

[from Plugins/Extensions/FileCommander/FileListmod.py]

[IanSav]

Hi Prl,

Perhaps onlookers will begin to understand why we are spending so much time and effort try to improve the code within Enigma2. There is lots of poor coding lurking under the covers. Unfortunately not all our efforts are readily visible to users.

I am waiting for some language confirmations and then I will submit the new VirtualKeyBoard for everyone to experience. I am wondering if I should bring forward the pull requests for Setup and ConfigList. They could be stand alone updates but ConfigList may need to have some adjustments to work with config changes. For the moment I will hold back until more of the config code is finished.

Regards,
Ian.

[prl]

Some posts have been moved from another topic. This post is in response to:

Hi,

Work is progressing quite well though I don't know if or when Beyonwiz will get the code.

I have now changed emphasis from the Beyonwiz image to the OpenViX and OpenPLi images. It has become unworkable for me continue wasting large amounts of time and effort trying to get code accepted into the Beyonwiz image (see here for an example). I am no longer maintaining Beyonwiz compatibility / functionality as a priority. This is likely to cause some level of problem in the future as the Beyonwiz code differs from both OpenViX and OpenPLi.

I am unable to get any public code testing done for the Beyonwiz image. Without access to Beyonwiz testers I won't proceed to formal production code submission.

I regret being forced into this position but the circumstances are completely out of my control.

Regards,
Ian.

I am unable to get any public code testing done for the Beyonwiz image. Without access to Beyonwiz testers I won't proceed to formal production code submission.

I tested and reported back on the setup refactor patch you made available in March 2017.

I've tried out all the user-visible actions in IanSav's early post in the topic, and it all seems to work as advertised. It will be good to get the same uniform functionality across more setup screens than is currently the case.

For more the recent package of code files you asked more for code review, and in fact were rather discouraging to potential testers:

I have attached my alpha level code for your review and comment. I welcome any suggestions or corrections you may care to offer. For others thinking about using this code, be careful! This is NOT production or even beta level code. Don't bother trying this out unless you have a good grasp of how Enigma2 works.

But, in fact, I did some testing anyway:

I've installed the changes on my test U4 and with minimal testing they seem to be OK.

I also made some effort re-arranging the code so that I could more easily compare it to the existing code, and posted a reasonable amount of commentary on it.

What did you want beyond that?

[IanSav]

Hi Prl,

I very much appreciate your efforts and support. Unfortunately not everyone associated with Beyonwiz development is so supportive.

The config.py code has continued to mutate significantly. The rate of change is starting to subside as the code starts to get cleaner and more logically organised. I wanted to get these changes into a beta where they can be heavily exercised by a number of users. I want to see how the changes are perceived. If they meet with user expectations then that would be okay. If users are unhappy about anything I would like to know now rather than later when things may prove more difficult to change. I also want to see how code holds up when used by different people with different expectations. This feedback would help to see if any changes are required before the code is completed.

The nature and breadth of the changes is not conducive to a patch kit for non technical users. It is those users who need to test and review the changes as they have been made with them in mind. I feel the best way to get this testing and feedback is to add the code to a beta. I have been wanting to do this for over a year but, as you should be aware, this has proven impossible.

I have been getting significantly more interest and support from OpenViX and OpenPLi which is why I am biasing my efforts there. If you would like to test and review my more recent efforts I will try and get a test kit available for you though I would still prefer to have the code more widely available so that it can be properly tested and reviewed by the people for whom it was written.

Regards,
Ian.

[prl]

If I had access to the new code I could see whether I could build a patch kit for it. I don't think there would have been any issues making one from the files you last posted. But perhaps the discussion should continue in a more suitable topic.

In the original discussion, IanSav replied:

Hi Prl,

Please move the appropriate posts to a thread in the developers area so I can discuss my reservations with you.

Regards,
Ian.

That post has been left in place.

[prl]

I can easily generate patch files that replace files like setup.xml and keyboard.xml, and restore them on uninstall.

[IanSav]

Hi Prl,

I am reluctant to build a patch kit because it is not particularly user friendly and invites lots of compatibility and upgrade issues. There are lots of files to keep in sync and these files will be affected by updates.

On OpenViX I believe there is going to be a beta build branch created to test the code though that has not yet been finally decided. The code may go into the standard Dev branch. I was hoping for Beta access here but since I started asking well over a year ago that seems very unlikely.

What do you think?

Regards,
Ian.

[prl]

I am reluctant to build a patch kit because it is not particularly user friendly and invites lots of compatibility and upgrade issues. There are lots of files to keep in sync and these files will be affected by updates.

Unless the scope of the number of files affected has changed greatly since you posted the ZIP file last month, which had 5 files in it, it's well in the range of size of change that I'd be willing to undertake in a patch kit. I always make sure when I make kits available which firmware versions they should be on, for users to remove patches before applying online updates, and more recently, to indicate whether the patches are likely to be incompatible with adoxa's patches. I always stress that the updates are intended for testing only and not for normal use.

I'm happy to manage the patch kit process myself, while making clear that the actual work was yours.

If your intention is to do more than just offer the changes for testing and to try to maintain them as long term patches, then I might not be so willing to try to keep up with that.

[IanSav]

Hi Prl,

I think I could work it that there are smaller changes that can be *mostly* self contained but I was hoping that they go into a beta so that they could be widely experienced without all the "patch kit" pain. For example, we could separate Setup.py, ConfigList.py, keymap and the skins into a single change to start the testing experience.

I think could also add the new VirtualKeyBoard and its skin changes as a separate testing component. There is also a new NumericalTextInput.py that is getting close to finished, though it also needs a language review for the foreign languages. These two modules will also need some skin changes including equivalent changes in the skin_default but this is very much more difficult to co-ordinate separately from OpenViX and OpenPLi.

Later I could introduce the new config.py and change ConfigList.py, Setup.py etc to match.

I am concerned that having Adoxa's patches mixed with patches coming from me/us is only going to make user's lives more haphazard and stressful. I am also concerned that any patch type changes I propose will cause me to be blamed for *ANY* issues that get experienced by any users of anything other than the official firmware.

I will take you advice on what to do but I will not accept unfair criticism and personal attack for trying to improve Enigma2.

Regards,
Ian.

[prl]

I'd prefer to see the testing done in steps rather than an all-at-once approach that makes it harder to concentrate testing on particular areas (much the same as my preference to see the steps in the development of the code I reviewed earlier).

Most of the people who would be doing the testing are aware of the potential for conflicts between different patches, and I'd give advice about that if I was managing the creation of the patches.

I simply want to make it easier to do the testing you want to do and I do not wish to be drawn into arguments with others doing development and testing.

[adoxa]

I'd also be willing to incorporate changes directly into Patches by Adoxa, (have to rename it, then ).

[IanSav]

Hi,

Okay, so we have interest to make stuff happen. Adoxa's offer certainly limits the chances of conflicting patches. What should the first item to consider for testing.

We could take VirtualKeyBoard.py and then ensure that the three Beyonwiz skins are adapted to support the changes so that we don't need skin_default to be changed at this point in time. (When OpenViX and OpenPLi accept the code then skin_default will be updated and we can remove the extra images from the Beyonwiz skins - if desired).

Alternatively or additionally I could prepare Setup.py, ConfigList.py keymap.xml and the "Setup" screen to improve the whole "Setup" experience. There may need to be future changes to account for changes still in development with config.py.

Thoughts?

Regards,
Ian.

[Paul_oz53]

I'd also be willing to incorporate changes directly into Patches by Adoxa, (have to rename it, then ).

Perhaps: Patches by Adoxa, IanSav and prl. Copyright. All rights reserved, Paul_oz53, 2018.

By virtue of this notice, I hereby grant Adoxa, IanSav and prl irrevocably and with immediate effect unrestricted rights in perpetuity to use this phrase without attribution. Paul_oz53.

I'm still not using the U4 as our main machine so I would test on that machine. In practice, the firmware at 20180704 has reached the point that the only patch that I continue to routinely use is Adoxa's Ice-desc patch, which I do not consider essential on the U4 and would drop if a potential for conflict arose. I assume that S2F would not be an issue but I can sacrifice it too if necessary.

I am currently attempting to modify keymap.xml to work with my keyboards using IanSav's version as a template but it would be reverted to standard if a patch is being tested, noting Ian mentions it as a part of the potential patch. No doubt others will have their own idiosyncracies at play.

[IanSav]

Hi Paul,

You, and others interested in keymap changes, should be pleased to note that my preferred changes to keymap.xml are to add new mappings to suit my new code and not to change existing definitions. That is, I don't expect any conflicts with any existing changes.

Regards,
Ian.

[prl]

You, and others interested in keymap changes, should be pleased to note that my preferred changes to keymap.xml are to add new mappings to suit my new code and not to change existing definitions. That is, I don't expect any conflicts with any existing changes.

Adoxa's patch kits could work like that, provided there weren't any conflicts when patch runs on keymap.xml. My patch kits would overwrite keymap.xml (and restore it on uninstall). Of course, if/when the patches become a beta, any changes to keymap.xml that a user had made would need to be re-applied.

[IanSav]

Hi All,

Just a quick note to say that I am working on an update to OverlayHD that will support all these Setup related changes. Once this is done I will reverse engineer skin changes for the Beyonwiz included skins. Once I have some workable skins for everyone to use then I will submit the latest version of my test code.

Regards,
Ian.

[Paul_oz53]

Hi All,

Just a quick note to say that I am working on an update to OverlayHD that will support all these Setup related changes. Once this is done I will reverse engineer skin changes for the Beyonwiz included skins. Once I have some workable skins for everyone to use then I will submit the latest version of my test code.

Regards,
Ian.

Looking forward to it Ian.

You, and others interested in keymap changes, should be pleased to note that my preferred changes to keymap.xml are to add new mappings to suit my new code and not to change existing definitions. That is, I don't expect any conflicts with any existing changes.

Adoxa's patch kits could work like that, provided there weren't any conflicts when patch runs on keymap.xml. My patch kits would overwrite keymap.xml (and restore it on uninstall). Of course, if/when the patches become a beta, any changes to keymap.xml that a user had made would need to be re-applied.

In the past, I found the easiest way to cause a crash was to be careless with keymap.xml. I now have the function key colour mappings working with the keyboard - thanks prl, et al. I'm mindful to restore defaults before applying patches but I look forward to the day when patches are unnecessary.

For me, this is all about improving the user options for keyboard entry when renaming files, etc. using an actual keyboard. My personal wish list is getting shorter by the day!

[IanSav]

Hi Prl /Adoxa,

I am almost ready to generate a test kit for the new Setup.py, ConfigList.py, config.py etc. What is required to get this new code and skins into a patch kit?

Regards,
Ian.

[prl]

I have my own scripts for generating my patch installer/uninstaller scripts. I'm happy to share, but they have built-in assumptions about the relative locations of the skins and the code. Those assumptions can be changed relatively easily by changing path variables, but it's all a bit fragile and has a definite air of "for author's use only" about it. The scripts have only ever been used on OS X, but, possibly with some minor modifications, should work on Linux. I'm not sure about other Unixoid environments.

I assume that adoxa has similar build scripts for his patch kits, and they may be more robust.

As an alternative, I'd be happy to take the changes, do some initial testing and then create a patch kit that I could make available.

It would make things slightly easier on my side if you were able to put the relative paths of the files into the zip, e.g. lib/python/Screen/Setup.py, rather than just Setup.py).

[adoxa]

Depends how you want to handle it. If you're happy to have it as part of Experimental just send me the files or branch and I'll take care of it. If you want to DIY edit the attached setup.sh to change the backup, newlist and title variables, copy the files as they would be under usr and then zip and attach. The zip should contain something like:

Code: Select all

enhanced-config/setup.sh
enhanced-config/usr/lib/enigma2/python/Components/config.py
enhanced-config/usr/lib/enigma2/python/Components/ConfigList.py
enhanced-config/usr/lib/enigma2/python/Screens/Setup.py
...

Alternatively, you could grab the Experimental source and adjust that to build an enhanced-config package.

[IanSav]

Hi,

I am concerned about various patch kits conflicting. I would like to put all the files here so that everyone can see what I have changed and can integrate these changes into existing patch kits.

Thoughts?

Regards,
Ian.