59 replies to this topic

[littlesat]

I did a look at it... and I totally do not understand the need for that callback.... all keys WHERE something is changed we do actually the callback... so where is the need to do it as callback and do those extra checks on previous value after it is changed by something...? It might be that on a key change indeed nothing is change, e.g. in a list with no options or so... But I do not see why such a big change ever should be made.... The commit looks logical... but also with OpenPLi when you make no change nothing is done....

 

My 2cts... Actually I know the complete config stuff could be rewritten. This only adds extra spaghetti to the code... And as you see the only result is it will break compatibility... Plus I know and see it is a lot of effort with at the end almost no real gain in performance and users even do not notice it.

 

Also when you want to get this crashed the plugin needs to somehow 'hack' into the config stuff.... which is also not really needed....

 

P.S. I saw OpenATV has something, as far I can see closed source, that is capable of translating subtitles with AI.

Edited by littlesat, 6 May 2024 - 12:56.

[Huevos]

So the callback deals with false notifiers that pli demonstrates. If you download the notifier tester from my repo you will see. The fact is that from configlistscreen you can't tell what is happening in config.py which is why there is a callback.

[littlesat]

In the original code it is looked at the key press. Only key presses that change the value do what you now do with the callback with an extra precious check. I do not see why this is better….. beside the fallback makes it more complicated. And as far I can see it is always correctly done as is…. It makes no difference what config.py ‘says’. As far I can see in practice zero added value so far. I’m afraid you are mislead by the description of the merge request. As it ‘was’ it looks to the key presses. Left right for example change something ‘always’… up down for example no values are changed. When you really target to improve it you do only a change with up/down etc… .what also does not happen with that change. So when the real target here is avoid/reduce notifiers I suggest there exist a better approach and also one that avoids breaking stuff…. Just by ‘remember’ a previous value when you focus a config and only send a notifier when you leave the focus and the value is changed. So I do not understand how all this this target that goal…. As far I can see the fallback stuff will send maybe a few less, bacause with a short change it still send them, but still too many notifiers.

Edited by littlesat, 7 May 2024 - 22:12.

[Huevos]

You are looking at 2 different problems. One is the notifier. That is handled in config.py with many problems in PLi. The other is the button press is handled in configlistscreen without any knowledge of if the config item changed or not. That is what the callback fixes.

[Huevos]

By the way, the real problem is why is that plugin poking around in configlistscreen.

[littlesat]

Yep indeed…the real issue is the plugin is poking. The only need for that is making it not compatible with openpli (lol).. And Instill totally do not see not experience any issues with the openpli config code….. as what we do we know when something is changed… when you do it really good then you do it when you leave the focus (up/down/next page/prev page/start/end/green button, leave config)on a specific config and that is not done with the fallback. So as far I can see it is exactly doing the same.

Edited by littlesat, 8 May 2024 - 19:07.

[Huevos]

Yep indeed…the real issue is the plugin is poking. The only need for that is making it not compatible with openpli (lol).. And Instill totally do not see not experience any issues with the openpli config code….. as what we do we know when something is changed… when you do it really good then you do it when you leave the focus (up/down/next page/prev page/start/end/green button, leave config)on a specific config and that is not done with the fallback. So as far I can see it is exactly doing the same.

https://github.com/O...st.py#L235-L237

	def keyLeft(self):
		self["config"].handleKey(KEY_LEFT)
		self.__changed()

	def keyRight(self):
		self["config"].handleKey(KEY_RIGHT)
		self.__changed()

	def keyHome(self):
		self["config"].handleKey(KEY_HOME)
		self.__changed()

	def keyEnd(self):
		self["config"].handleKey(KEY_END)
		self.__changed()

	def keyDelete(self):
		self["config"].handleKey(KEY_DELETE)
		self.__changed()

	def keyBackspace(self):
		self["config"].handleKey(KEY_BACKSPACE)
		self.__changed()

	def keyToggleOW(self):
		self["config"].handleKey(KEY_TOGGLEOW)
		self.__changed()

	def keyGotAscii(self):
		self["config"].handleKey(KEY_ASCII)
		self.__changed()

	def keyNumberGlobal(self, number):
		self["config"].handleKey(KEY_0 + number)
		self.__changed()

As you can see in this code PLi calls "self.__changed()" on every key press irrespective of whether the config item actually change. That means in PLi there are false positives. The images that use the callback only call "self.__changed()" when there really is a change.

[Huevos]

Then in config.py for example ConfigSelection...

 

https://github.com/O...ig.py#L345-L351

	def setValue(self, value):
		if str(value) in map(str, self.choices):
			self._value = self.choices[self.choices.index(value)]
		else:
			self._value = self.default
		self._descr = None
		self.changed()

In that function PLi calls "self.changed()" which triggers all the notifiers, irrespective of whether or not there was actually a change to the value of the config item. So here you have another load of false positives.

 

In images that want to avoid this problem you have something like this:

	def setValue(self, value):
		prev = str(self._value) if hasattr(self, "_value") else None
		if str(value) in map(str, self.choices):
			self._value = self.choices[self.choices.index(value)]
		else:
			self._value = self.default
		self._descr = None
		if prev != str(self._value):
			self.changed()

 

Edited by Huevos, 9 May 2024 - 13:09.

[Huevos]

Plugin for testing notifiers:

https://github.com/H...ester/plugin.py

[littlesat]

As you can see in this code PLi calls "self.__changed()" on every key press irrespective of whether the config item actually change. That means in PLi there are false positive
->
It is not that dramatic as you say here. As it is only on key presses that change something. On key presses that do not change something it does not happen.

I think this could be resolved much easier. No callback stuff needed. This can be done with a solution with a much less impact on the code when you want to try to do it. You need to ‘target’ that you store somehow the previous value in a structural way. And the. Do a check in that __changed()/changed() function. Less impact in the code and then much more clearly capture the target. And most likely you even do not have add a value for the prev value as you can also do checks when it is changed.

And maybe even a better target is only do it with changes when you ‘lost’ focus with up/down key and check there for a change

Also instead if prev value I prefer to go for new value and then do something when new != current

And note I know for 15+ years the code is not optimal. But blindly hack for the notifiers with callbacks is not optimal.

Edited by littlesat, 10 May 2024 - 07:21.

[littlesat]

Btw what instead of the callback let the keypress routine return True when it is changed and then call the changed stuff when the result is True. No need of fallback…. And I think better readable code. Also avoids plugins ‘hack-in’

Edited by littlesat, 10 May 2024 - 07:46.

[littlesat]

I just did try to discover false notifiers... But I could not find or trigger them.... how do you trigger them? Here only Notifiers on OpenPLi so far I can see when it is mandatory....

Edited by littlesat, 10 May 2024 - 11:56.

[littlesat]

One I found... e.g. go to al list and press e.g. number key... then a notification is send to where the list is created....  'finetuning not really dramatic' But I think we could make a more easier solution for that....

Edited by littlesat, 10 May 2024 - 12:16.

[littlesat]

I tried to resolve it a more easier and better 'backwards compatible' way...

https://github.com/O...b0cd58a1db490f8

[Huevos]

All you have done is worked around the callback. This only affects feedback to ConfigListScreen (and only affect plugins that are poking around where they shouldn't be).

 

But you have missed the main problem which is the false notifiers.

Edited by Huevos, 10 May 2024 - 22:14.

[littlesat]

Tell me what I missed….. what do I now need te do to trigger notifiers. I just demonstrate another solution with for not less impact to the code and a risk stuff does not get compatible. The fallback gives me the feeling from work-a-round stuff. And the way prev value is managed.

Edited by littlesat, 10 May 2024 - 22:16.

[Huevos]

There is no impact from the callback to the distros that are using it. The problem is PLi is not using it so that plugin crashes in PLi.

 

Really pointless making these tiny changes when the Setup.py, ConfigList.py, config.py, setup.xml need updating to the modern code. In current PLi it is impossible to use plugins that have a setup.xml file. And you have hundreds of lines of unnecessary code throughout enigma repo.

[littlesat]

I at least demonstrated that there are different solutions and the fallback wirh how the prev value is done draws a red flag.
I think that is not done.
The plug-in’s setup.xml and also lines that can be reduced. For plugins that can use setup.xml a lot of python lines can indeed be reduced. But sjit happens with forking from here and not always merge things back without accepting comments.
And by the way maybe at thia moment you can spend tons of times I do not have anymore in such a way I had some years ago when we’re both coding at that frontend support. What I miss are currently the good discussions from that time we had… now I only feel you tell how good they are or that ‘other side’ and we are far far behind the modern wonderful and fancy code…. That feels like a smash in my face and not really friendly.

The target here were the notifiers. So far for ‘key press’ part I resolved them and I also ‘prepared’ a prev_value what I can use to extend without the fallback. And when a new target is a setup.xml for plugins…. I thing that can also be done easy.

The new creation of the setup was orriginaly my own code long time ago. It will now remove/add options depending on ‘changes’.
Before I started it all configs were fully statically. I do not remember I ever see merge request in improvements about this work….

And I know for 2-3 years ago you did a lot of work here. At that moment I could not do any code at all due to health issues…. Not give comments…

Edited by littlesat, 10 May 2024 - 23:24.

[Huevos]

Like I said the callback is only a problem on PLi. Our code is backwards compatible irrespective of whether the 3rd party code uses the callback arg or not.

[Huevos]

 

But sjit happens with forking from here and not always merge things back without accepting comments.
 

 

What is the problem with forking? PLi is just a fork of DMM. The only reason Andy started the oe-alliance was because PLi refused his commits.

 

 

And by the way maybe at this moment you can spend tons of times I do not have anymore in such a way I had some years ago when we’re both coding at that frontend support. What I miss are currently the good discussions from that time we had… now I only feel you tell how good they are or that ‘other side’ and we are far far behind the modern wonderful and fancy code…. That feels like a smash in my face and not really friendly.

That is not what is happening here. I said "do you want me to port the setup code to PLi". The setup code was developed 5 years ago and has been tested at length and improved. Now, 5 years later you want it redesigned for use in PLi. I don't have time for that and the current code works well, and the whole point of the upgrade is so PLi is compatible with other images. If we start changing things that will not be the case.

 

Anyway, I am going to step back from this because you obviously don't want what I was offering.

Edited by Huevos, 11 May 2024 - 09:33.