Using ConfigText over ConfigSelection...

[IanSav]

Hi,

Is there any reason why "config.usage.default_path", "config.usage.timer_path", "config.usage.instantrec_path" and possibly other path settings are defined in "UsageConfig.py" and elsewhere as ConfigText objects while the UI settings to set these objects fudge around to enter the data in a ConfigSelection object format?

The only use that these configuration settings get, in the Python code at least, is to use the .value property. In this case a ConfigText and ConfigSelection would return the same value.

I intend to simplify and improve the UI for setting the value of these items and would like to confirm that these definitions can be safely changed from ConfigText to ConfigSelection. Lots of code can be removed and the logic is simplified. So far in my early and limited testing there has been no issue with the change.

This code is change will be part of my Setup clean up.

As part of my clean up effort I also found a bug. If there is a list of directories in the selection list but some valid directories are between a pair of invalid directories (not writeable, or with my new code, located on the Flash) then you can not step past an invalid directory to access the other valid directories.

E.g. Say the directory list contains: "/mnt/hdd/media/", "/mnt/DVDTITLE/", "/mnt/hdd/media/movies/", "/mnt/hdd/media/tvshows/", "/mnt/", "/mnt/autofs/NAS/" where"/mnt/DVDTITLE" is a DVD or read-only mounted file system.

• Assume the currently selected directory is "/mnt/hdd/media/".
• If you try and use RIGHT to scroll right around the list of directories you get an error that "/mnt/DVDTITLE" is not writeable and can not scroll further right.
• If you try and use LEFT to scroll left around the list of directories you get to "/mnt/autofs/NAS/" but then get an error that "/mnt/" is on the Flash chip and can't scroll further left.

Thus you can NEVER access or select the "/mnt/hdd/media/movies/" or "/mnt/hdd/media/tvshows/" entries.

This bug will also be corrected in my refactored code.

Regards,
Ian.

[prl]

You might need to give more context to the directory selection bug. I was able recently to select a non-existent directory from the selection list in the Timer Entry screen's Location setting, which is a ConfigSelection.

Is it just in the settings for the recording defaults (and perhaps for the Timeshift directory)?

[IanSav]

Hi Prl,

What do you think about the definition of the paths as "ConfigText" yet all the management is done with "ConfigSelection" variables that are then copied to the original "ConfigText" variables? As I said, so far in my development and testing using a "ConfigSelection" for definition and editing seems to be working fine.

I can probably set up some evaluation files if you would like to look at the code and give it a test.

As to the directory selection bug, this can not be be easily demonstrated at the moment. All I did was arrange to have two unacceptable directories (no write access and one on the Flash partition) in the "config.movielist.videodirs" list. I had the entries separated by some valid entries. Then use the current Recordings.py UI and try to use LEFT and RIGHT to navigate along the list of directories in "Default movie location" entry. When an unacceptable directory is encountered a MessageBox pop up is generated to highlight the issue and the list pointer is set to the last (previous) entry. This means that once an invalid entry in encountered the entries beyond that entry can not be accessed via that direction. Similarly an error going in the reverse direction stops navigation to earlier entries in the list. This means that if there are two unacceptable directories are in the list both separated by acceptable directories then parts of the "config.movielist.videodirs" list can become inaccessible.

As another example, here is a potential list of directory entries "A, B, C, D, E, F, G, H". "C" is the current and selected entry. "B" and "G" are read only or Flash based directories. Using LEFT and RIGHT the entries "C", "D", "E" and "F" can be selected. Trying to navigate to "B" will generate an error and the current pointer will be reset to "C". Trying to navigate to "G" will generate an error and the current pointer will be reset to "F". That means that with the current UI entries "B" and "G" can not and must not be selected and entries "A" and "H" can never be accessed or selected.

Does this better explain the bug?

Regards,
Ian.

[prl]

I don't know why there is a temporary ConfigSelection used for configuration, but the values are stored in a ConfigText. There may be problems in knowing what the set of choices for the config variable are at startup time, and so there may be problems with a ConfigSelection reverting to its default if its value isn't in the list of choices available at startup.

The problem with being able to navigate through invalid directory entries in the three recording location configs (and in "Timeshift location") is that the config variables have "final" notifiers that validate the config value when the item is deselected (which is what I'd expect), but the containing screens (Screens.RecordingSettings resp Screens.TimeshiftSettings) also use the "on_change" mechanism in their ConfigListScreen parents to test (and reset if invalid) the selected value on each selection change.

I'm not sure why that second test is done that way. I think that if there is a test on each selection change, it should do no more than warn, and any test that prohibits selection should only be done when the config item is deselected (by UP/DOWN or by the screen closing).

[IanSav]

Hi,

I have proceeded to code up my refactor and enhancement of the "Setup.py" screen and some of the other screens that are cut and pastes of the "Setup.py" code.

I have coded up the following changes:

• "Setup.py" has been refactored and enhanced.
• "Recording.py" now uses "Setup.py" as its base class.
• All the copy / pasted code in "Recordings.py" is now gone.
• The "UserConfig.py" path definitions are now "ConfigSelection" rather that "ConfigText" objects.
• The UI components of the path settings now join the rest of the settings in "setup.,xml".
• The scrolling through the path options now shows all Bookmarks but won't allow the user to save or navigate away from any setting that is invalid.
• Invalid path settings no longer cause a "MessageBox" pop up but rather show an error message in the "footnote" area.
• The OK button is disabled if any path settings are invalid.
• Pressing GREEN while the OK label is hidden and the current path setting is invalid will pop up the original error "MessageBox".

I have attached the proposed new code for both "Setup.py" and "Recordings.py". Some things to note:

• This code is not final and subject to further change.
• The "Recordings.py" code uses some new features in "Setup.py".
• "self.footnote" is defined in "Setup.py" but is now an always displayed Label with the contents set or cleared in response to the "*" reboot flag or any other status message request.
• The ["config"] tupple is now ("Setup label text", config.definition, "Setup description text", "Setup XML data attribute") so the "item[3]" element used in the code refers to the "setup.xml" data attribute of the setup item.

The following is an extract from the proposed new "UserConfig.py" file:

Code: Select all

	tmpvalue = resolveFilename(SCOPE_HDD)
	if not tmpvalue.endswith('/'):
		tmpvalue = tmpvalue + "/"
	config.usage.default_path = ConfigSelection(default=tmpvalue, choices=[(tmpvalue, tmpvalue)])
	config.usage.timer_path = ConfigSelection(default="<default>", choices=[("<default>", "<default>")])
	config.usage.instantrec_path = ConfigSelection(default="<default>", choices=[("<default>", "<default>")])

This extract shows that the paths are now "ConfigSelection" objects rather than the previous "ConfigText" objects.

The following is an extract from the proposed new "setup.xml" file:

Code: Select all

<!--suppress XmlUnboundNsPrefix -->
<setupxml>
	<default menuimage="menus/mainmenu_tasks_setup_system.png" />
	<setup key="recording" title="Recording and playback settings" menuimage="menus/mainmenu_tasks_setup_default.png">
		<item level="0" text="Movie location" description="Set the location for your recordings. Press 'OK' to add new locations, select left/right to select an existing location." data="DefaultPath" requires="SetupFlagGUISimple">config.usage.default_path</item>
		<item level="2" text="Default movie location" description="Set the default location for your recordings. Press 'OK' to add new locations, select left/right to select an existing location." data="DefaultPath">config.usage.default_path</item>
		<item level="2" text="Timer recording location" description="Set the default location for your timers. Press 'OK' to add new locations, select left/right to select an existing location." data="TimerPath">config.usage.timer_path</item>
		<item level="2" text="Instant recording location" description="Set the default location for your instant recordings. Press 'OK' to add new locations, select left/right to select an existing location." data="InstantRecPath">config.usage.instantrec_path</item>
<!-- 		<item level="2" text="Preferred tuner for recordings" description="Configure which tuner will be preferred for recordings, when more than one tuner is available. 'Disabled' would select a tuner based on preferred tuner in customise screen. 'Auto' would choose based on E2's default rules, ignoring preferred tuner in customise screen.">config.usage.recording_frontend_priority</item> -->
		<item level="1" text="Recordings always have priority" description="Enable to allow a recording to interrupt live TV when there are no free tuners.">config.recording.asktozap</item>
		<item level="0" text="Margin before recording (minutes)" description="Set to a non-zero value to begin a recording earlier than the start time shown in the EPG.">config.recording.margin_before</item>
		<item level="0" text="Margin after recording (minutes)" description="Set to a non-zero value to stop a recording later than the end time shown in the EPG.">config.recording.margin_after</item>
		<item level="0" text="Default length of instant recording" description="Set the default length of an instant recording. 'Padded event' makes the default length the remainder of the event currently being broadcast plus the 'after' margin set above. 'Padded event' defaults to 120 minutes if no EPG information is available for the event.">config.recording.instant_recording_length</item>
		<item level="2" text="Show message when recording starts" description="Enable to display a pop-up message when a recording starts.">config.usage.show_message_when_recording_starts</item>
		<item level="2" text="Show movie lengths in movie list" description="Enable to display the length of each recording in the movie list (this may result in additional loading time).">config.usage.load_length_of_movies_in_moviellist</item>
		<item level="2" text="Show status icons in movie list" description="Choose the type of status indicator icons shown in the movie list.">config.usage.show_icons_in_movielist</item>
		<item level="2" text="Quit movie player with EXIT button" description="Enable to leave the movie player by pressing EXIT. If the PiP window is shown, EXIT button behaviour is determined by the 'Close PiP on EXIT' setting. When the infobar is shown, press EXIT to close it.">config.usage.leave_movieplayer_onExit</item>
		<item level="2" text="Behaviour when a movie is started" description="Choose the behaviour when movie playback is started.">config.usage.on_movie_start</item>
		<item level="2" text="Behaviour when a movie is stopped" description="Choose the behaviour when movie playback is manually stopped.">config.usage.on_movie_stop</item>
		<item level="2" text="Behaviour when a movie reaches the end" description="Choose the behaviour when the end of a movie is reached during playback.">config.usage.on_movie_eof</item>
		<item level="2" text="Display message before playing next movie" description="Enable to display a pop-up message when a movie has finished playback, prior to playback of the next movie in the list.">config.usage.next_movie_msg</item>
		<item level="1" text="Enable auto-skip" description="Enable to automatically skip playback over sections marked with edit points.">config.seek.autoskip</item>
		<item level="2" text="Seekbar/time entry skip activation" description="Choose seekbar activation with either long LEFT/RIGHT or long << / >>. Time entry skip is set to the controls not used for seekbar activation.">config.seek.baractivation</item>
		<item level="2" text="Seekbar skip" description="Set the skip size for the seekbar (as a percentage of the overall recording length).">config.seek.sensibility</item>
		<item level="2" text="Fast forward speeds" description="Configure all possible fast forward speeds. Press the >> button multiple times to step through speeds defined by this setting.">config.seek.speeds_forward</item>
		<item level="2" text="Rewind speeds" description="Configure all possible rewind speeds. Press the << button multiple times to step through speeds defined by this setting.">config.seek.speeds_backward</item>
		<item level="2" text="Slow motion speeds" description="Configure all possible slow motion speeds.">config.seek.speeds_slowmotion</item>
		<item level="2" text="Initial fast forward speed" description="Set the initial fast forward speed. When you press the >> button, forwarding starts at this speed.">config.seek.enter_forward</item>
		<item level="2" text="Initial rewind speed" description="Set the initial rewind speed. When you press the << button, rewinding starts at this speed.">config.seek.enter_backward</item>
		<item level="2" text="Limit character set for recording filenames" description="Limit the characters that can be used in recording filenames to (7 bit) ASCII. This ensures compatibility with operating systems or file systems with limited character sets.">config.recording.ascii_filenames</item>
		<item level="2" text="Composition of recording filenames" description="Specify how recording filenames are constructed.">config.recording.filename_composition</item>
		<item level="2" text="Remove completed timers after (days)" description="Set the number of days old timers are retained before they are automatically removed from the timer list.">config.recording.keep_timers</item>
		<item level="2" text="Offline decode delay (ms)" description="Set the offline decoding delay (in milliseconds). The specified delay is observed at each control word parity change.">config.recording.offline_decode_delay</item>
		<item level="2" text="Default recording type" description="Descramble & record ECM' provides the option to descramble after recording if descrambling on recording fails. 'Don't descramble, record ECM' saves a scrambled recording that can be descrambled on playback. 'Normal' descrambles the recording and does not record ECM.">config.recording.ecm_data</item>
	</setup>
</setupxml>

This extact shows some of the new tags and attributes available for setup based screens.

I currently have one remaining issue that is giving me grief. When I use the ".setChoices" method to update the current directory options and the currently selected item of the "Setup.py" based "["config"]" list object in "Recordings.py" the screen does *not* update the newly programmed current selection. Can anyone assist me in resolving this problem.

I would also welcome feedback and suggestions on the new code.

Regards,
Ian.

[prl]

I think I suggested that the reason for using the ConfigText/ConfigSelection pairs was to avoid using .setChoices(). There are a few instances of the use of .setChoices() in the code. Looking at them may help.

[prl]

The Recordings.py has had all ts line endings changed from the Unix form <LF> to the DOS form <CR><LF>. I'm not sure whether the transformation back is handled automatically in Windows git, but if it isn't, git will see every line in the file as having been changed.

[prl]

...
I currently have one remaining issue that is giving me grief. When I use the ".setChoices" method to update the current directory options and the currently selected item of the "Setup.py" based "["config"]" list object in "Recordings.py" the screen does *not* update the newly programmed current selection. Can anyone assist me in resolving this problem.
....

What change are you expecting?

Using ConfigSelection.setChoices() shouldn't change the current value of the selection unless the current value isn't in the new list. If the current selection isn't in the new list, then the current selection will be set to the default, which will be the default passed to setChoices() if the default passed is not None. If no default is passed, or it is None, the default will be the first element in the choices list if it's a list, or an essentially random element in the list if the choices are a dict (which isn't the case here).

If you want the code to change the selection to some particular value, you need to code for that explicitly.

[IanSav]

Hi Prl,

The Recordings.py has had all ts line endings changed from the Unix form <LF> to the DOS form <CR><LF>. I'm not sure whether the transformation back is handled automatically in Windows git, but if it isn't, git will see every line in the file as having been changed.

I always work in <LF> endings but my GIT client (SourceTree) insists on changing line endings in each direction.

Regards,
Ian.

[IanSav]

Hi Prl,

What change are you expecting?

I was expecting the selection list to be positioned to the value set by "default". If "default" is not in the list then the current value is used as the position. If both are unavailable then position at the start of the list.

Using ConfigSelection.setChoices() shouldn't change the current value of the selection unless the current value isn't in the new list. If the current selection isn't in the new list, then the current selection will be set to the default, which will be the default passed to setChoices() if the default passed is not None. If no default is passed, or it is None, the default will be the first element in the choices list if it's a list, or an essentially random element in the list if the choices are a dict (which isn't the case here).

I am still working my way around understanding Python and Enigma2 but I understood that passing a "default" to .setChoices sets the currently selected option in the list.

If you want the code to change the selection to some particular value, you need to code for that explicitly.

I will have a look around the code for some examples but do you have a suggestion of the code I could use to set the currently displayed item in the list?

Regards,
Ian.

[IanSav]

Hi Prl,

It looks like I did not fully understand the code. As per your suggestion I checked some other occurrences of ".setChoices" in the Screen code area and noted that the ".setChoices" was sometimes followed by a ".value = default" to set the current value. This is an action that I believed was being performed by the ".setChoices". Obviously I was incorrect.

My initial testing shows that the screen update issue is now resolved. Thank you for your assistance.

A little more tidying up and testing and I will be ready to submit the code.

Did you have any other comments or feedback on my proposed refactor and enhancements?

Regards,
Ian.

[prl]

I haven't looked at the code beyond checking about the behaviour of ConfigSelection.setChoices(), or done any testing. I'll have a look at diffs and the code in general and do some testing later this afternoon and get back about it.

[prl]

I'm not sure why you're changing the Screens/Setup.py API by renaming setupdom() and changing the set of methods in Setup, especially things like renaming refill() to reloadList() and removing addItems().

I'm not sure why in Screens/Setup.py, loadSetupDOM() (aka setupdom()) isn't written as something like:

Code: Select all

__setupdoms = {}

def setupdom(setup=None, plugin=None):
	global __setupdoms
	if plugin:
		src = resolveFilename(SCOPE_CURRENT_PLUGIN, plugin + "/setup.xml")
		msg = " from plugin '%s'" % plugin
	else:
		src = resolveFilename(SCOPE_SKIN, "setup.xml")
		msg = ""
	print "[Setup] XML source file: '%s'" % src
	print "[Setup] XML menu '%s'%s" % (setup, msg)
	if src in __setupdoms:
		return __setupdoms[src]
	try:
		setupfile = open(src, "r")
		setupdom = xml.etree.cElementTree.parse(setupfile)
		setupfile.close()
	except IOError:
		print "[Setup] ERROR: Unable to access XML file '%s'!" % src
		setupdom = xml.etree.cElementTree.fromstring("<setupxml></setupxml>")
	__setupdoms[src] = setupdom
	return setupdom

To make it more backward-compatible, you could test the last modified time of the setup.xml file against the modified timestamp saved in __setupdoms. Entries in __setupdoms would then be a tuple (or something) like: (dom, modifiedtime).

In Screens/Setup.py:

Code: Select all

###
### Should the isinstance also cover TrueFalse, OnOff, EnableDisable?
###
			if isinstance(self.item[1], ConfigYesNo) or isinstance(self.item[1], ConfigSelection):

I can't see why

Code: Select all

isinstance(self.item[1], ConfigBoolean)

wouldn't be better. The Fine Manual says of isinstance(): "Return true if the object argument is an instance of the classinfo argument, or of a (direct, indirect or virtual) subclass thereof."

In Screens/Setup.py:

Code: Select all

###
### Should the other queued tasks also be removed?
###

I don't know what that refers to.

I'm not sure why a whole bunch of lines in the imports in Screens/Setup.py are swapped around so much. It makes it harder to see what the actual differences are. Moving the Tools.Directories import to after all the Component imports makes sense, and the line is being changed anyway.

In Screens/Recordings.py there seem to be further changes to the class API I can't see a clear motivation for (like ok() -> keyOK()).

In Screens/Recordings.py, pathStatus(), the test of the pathname against "/media" is not a correct test of whether the the path is on flash. "/media" isn't the flash device, it's an in-memory tmpfs filesystem:

Code: Select all

tmpfs on /media type tmpfs (rw,relatime,size=64k)

The flash device is mounted on '/':

Code: Select all

ubi0:rootfs on / type ubifs (rw,relatime)

This means that the test on the device of "/media" will stop the path being set to "/media", but not, say, to "/".

Code: Select all

>>> "%04x" % os.stat("/media/hdd/movie").st_dev
'0801'
>>> "%04x" % os.stat("/media").st_dev
'000d'
>>> "%04x" % os.stat("/").st_dev
'000b'
>>>

IIRC, the paths in these config variables have been put through "os.path.realpath()" so pathname prefixes should work, so using os.stat() to compare devices would be unnecessary (and harder to get right). Testing for path.startswith("/") would also be unnecessary. In this pathStatus(), using path.startswith("/") would actually break the device-based test. The enigma2 binary runs with its current directory as "/home/root", so if in pathStatus(), path is set to "../../media", path.startswith("/") will fail, and self.status will remain set to PATH_OK, even though the object of path is "/media".

I'd have thought that it might be better to use the user's setting of "Root directory" instead of (or as well as) "/media" to restrict the folders in these path configs. Otherwise recording directories could be set to places that the media player can't get to without changing "Root directory".

It would be a good idea to continue to apply the existing "/media" restriction to "Root directory", though.

I suggest that error messages be handled slightly differently and the scope of the translation function _() be more carefully used.

so:

Code: Select all

	ERRMSGS = {
		PATH_OK:	"",
		PATH_READONLY:	"Directory '%s' not writable!",
		PATH_FLASH:	"Flash directory '%s' not allowed!"
	}

becomes:

Code: Select all

	ERRMSGS = {
		PATH_OK:	"",
		PATH_READONLY:	_("Directory '%s' not writable!"),
		PATH_FLASH:	_("Flash directory '%s' not allowed!")
	}

Code: Select all

			self.session.open(MessageBox, _(self.ERRMSGS.get(self.status, "") + "\n\nPlease select a writable directory.") % self["config"].getCurrent()[1].value, type=MessageBox.TYPE_ERROR)

becomes:

Code: Select all

			self.session.open(MessageBox, _(self.ERRMSGS.get(self.status, _("Unknown error on %s"))) % self["config"].getCurrent()[1].value + _("\n\nPlease select a writable directory."), type=MessageBox.TYPE_ERROR)

and:

Code: Select all

self.footnote = _(self.ERRMSGS.get(self.status, "Unexpected issue with '%s'!") % path)

becomes:

Code: Select all

self.footnote = self.ERRMSGS.get(self.status, _("Unexpected issue with '%s'!")) % path

In general, things like:

Code: Select all

_("some stuff %d" % val)

aren't going to do what you intend. They need to be:

Code: Select all

_("some stuff %d") % val

I can't do any testing, because more is involved than just the two posted files.

[IanSav]

Hi Prl,

Thanks for the review. Let me process it and get back to you.

Regards,
Ian.

[IanSav]

Hi Prl,

As Setup.py is not yet complete I will defer comments on Setup.py until Timeshift.py is refactored and then Setup.py can be completed.

In Screens/Recordings.py there seem to be further changes to the class API I can't see a clear motivation for (like ok() -> keyOK()).

This change was made to align Recordings.py with the base code in the parent definition in ConfigList.py. Aligning this function removed the need for an ActionMap etc in Recordings.py.

In Screens/Recordings.py, pathStatus(), the test of the pathname against "/media" is not a correct test of whether the the path is on flash. "/media" isn't the flash device, it's an in-memory tmpfs filesystem:

Code: Select all

tmpfs on /media type tmpfs (rw,relatime,size=64k)

The flash device is mounted on '/':

Code: Select all

ubi0:rootfs on / type ubifs (rw,relatime)

This means that the test on the device of "/media" will stop the path being set to "/media", but not, say, to "/".

Code: Select all

>>> "%04x" % os.stat("/media/hdd/movie").st_dev
'0801'
>>> "%04x" % os.stat("/media").st_dev
'000d'
>>> "%04x" % os.stat("/").st_dev
'000b'
>>>

Point made and accepted. I don't know what thoughts were going through my head at the time but for some reason I thought testing against "/media" would be sufficient. To make the code more resilient I now check against "/", "/dev", "/proc", "/sys", "/var", "/media".

IIRC, the paths in these config variables have been put through "os.path.realpath()" so pathname prefixes should work, so using os.stat() to compare devices would be unnecessary (and harder to get right).

I don't think "os.path.realpath()" is being used.

Testing for path.startswith("/") would also be unnecessary. In this pathStatus(), using path.startswith("/") would actually break the device-based test. The enigma2 binary runs with its current directory as "/home/root", so if in pathStatus(), path is set to "../../media", path.startswith("/") will fail, and self.status will remain set to PATH_OK, even though the object of path is "/media".

I didn't think that paths could be constructed with ".." etc. All the paths come from "MovieLocationBox" and are resolved. The code has now been adjusted to only reject the "<...>" special cases and all other paths are tested.

The test "path.startswith("/")" was actually to identify the various "<...>" default cases. Given your feedback I have made the code more resilient by specifically checking if the path does not start with "<" then test the acceptability of the path.

I'd have thought that it might be better to use the user's setting of "Root directory" instead of (or as well as) "/media" to restrict the folders in these path configs. Otherwise recording directories could be set to places that the media player can't get to without changing "Root directory".

I now check for all the O/S mounts such that any setting of "Root directory" including "/" can be appropriately protected.

It would be a good idea to continue to apply the existing "/media" restriction to "Root directory", though.

Done.

I suggest that error messages be handled slightly differently and the scope of the translation function _() be more carefully used.

so:

Code: Select all

	ERRMSGS = {
		PATH_OK:	"",
		PATH_READONLY:	"Directory '%s' not writable!",
		PATH_FLASH:	"Flash directory '%s' not allowed!"
	}

becomes:

Code: Select all

	ERRMSGS = {
		PATH_OK:	"",
		PATH_READONLY:	_("Directory '%s' not writable!"),
		PATH_FLASH:	_("Flash directory '%s' not allowed!")
	}

Code: Select all

			self.session.open(MessageBox, _(self.ERRMSGS.get(self.status, "") + "\n\nPlease select a writable directory.") % self["config"].getCurrent()[1].value, type=MessageBox.TYPE_ERROR)

becomes:

Code: Select all

			self.session.open(MessageBox, _(self.ERRMSGS.get(self.status, _("Unknown error on %s"))) % self["config"].getCurrent()[1].value + _("\n\nPlease select a writable directory."), type=MessageBox.TYPE_ERROR)

and:

Code: Select all

self.footnote = _(self.ERRMSGS.get(self.status, "Unexpected issue with '%s'!") % path)

becomes:

Code: Select all

self.footnote = self.ERRMSGS.get(self.status, _("Unexpected issue with '%s'!")) % path

In general, things like:

Code: Select all

_("some stuff %d" % val)

aren't going to do what you intend. They need to be:

Code: Select all

_("some stuff %d") % val

This is embarrassing. I know better yet I allowed this to get through. I can only hope that I would have picked this up in a later review when I was sure that the code logic was correct. Anyway, it is fixed now.

I can't do any testing, because more is involved than just the two posted files.

Understood. This was part of my peer review process. I like to have my code reviewed by peers to ensure the logic and quality of the code is sound. I appreciate your comments and believe that they will lead to a better quality code submission. When the code is closer to submission I can provide a more complete set of changes that can be tested.

I have attached the latest proposal for "Recordings.py" to hopefully demonstrate the correct implementation of your suggestions. If there are no further issues this will be the version used for the code submission and the basis for the "Timeshift.py" refactor. All feedback and comments welcome.

Regards,
Ian.

[prl]

...

In Screens/Recordings.py there seem to be further changes to the class API I can't see a clear motivation for (like ok() -> keyOK()).

This change was made to align Recordings.py with the base code in the parent definition in ConfigList.py. Aligning this function removed the need for an ActionMap etc in Recordings.py.

OK, that seems a reasonable motivation. It's just that it's harder to get changes accepted if they change the API other than by purely extending it. I got caught in the changes I made to EPGSearch by differences between the Beyonwiz and OpenVIX APIs for ConfigElement.removeNotifier().

In Screens/Recordings.py, pathStatus(), the test of the pathname against "/media" is not a correct test of whether the the path is on flash. ...

Point made and accepted. I don't know what thoughts were going through my head at the time but for some reason I thought testing against "/media" would be sufficient. To make the code more resilient I now check against "/", "/dev", "/proc", "/sys", "/var", "/media".

IIRC, the paths in these config variables have been put through "os.path.realpath()" so pathname prefixes should work, so using os.stat() to compare devices would be unnecessary (and harder to get right).

I don't think "os.path.realpath()" is being used.

You're quite right, "os.path.realpath()" isn't used in media player file system navigation. There are good reasons why it should be, and equally good reasons why it shouldn't be, but it isn't.

However, if you're going to check against a "banned list", it may be better to use the one that's already defined: LocationBox.defaultInhibitDirs. Then (ignoring "<...>" "paths" for simplicity) perhaps do something like:

Code: Select all

	realpath = os.path.realpath(path)
	if (
		realpath.startswith(LocationBox.defaultInhibitDirs) or
		realpath in ("/media", /) or
		not path.rstrip("/").startswith(config.movielist.root.value.rstrip("/"))
	):
		# reject path

The test against config.movielist.root is done using path rather than realpath to match the test in MovieSelection, not because I think that's completely correct.

config.movielist.root should probably be a selector over tuples of (displaypath, realpath) to get around the ambiguities of what the root actually refers to (e.g. if the root is "/media", there may exist a link "/media/elsewhere" that actually points to, say, "/".

Testing for path.startswith("/") would also be unnecessary. In this pathStatus(), using path.startswith("/") would actually break the device-based test. The enigma2 binary runs with its current directory as "/home/root", so if in pathStatus(), path is set to "../../media", path.startswith("/") will fail, and self.status will remain set to PATH_OK, even though the object of path is "/media".

I didn't think that paths could be constructed with ".." etc. All the paths come from "MovieLocationBox" and are resolved. The code has now been adjusted to only reject the "<...>" special cases and all other paths are tested.
The test "path.startswith("/")" was actually to identify the various "<...>" default cases. Given your feedback I have made the code more resilient by specifically checking if the path does not start with "<" then test the acceptability of the path.

If a path other than the "<...>" ones doesn't start with a '/' (which I'm pretty sure they do: they may not be "realpaths", but they are absolute paths) then that's probably a break in the way the path is being set. It should probably simply be rejected if it's not an absolute path. Relative paths won't work at all well.

I'd have thought that it might be better to use the user's setting of "Root directory" instead of (or as well as) "/media" to restrict the folders in these path configs. Otherwise recording directories could be set to places that the media player can't get to without changing "Root directory".

I now check for all the O/S mounts such that any setting of "Root directory" including "/" can be appropriately protected.

It would be a good idea to continue to apply the existing "/media" restriction to "Root directory", though.

Done.

That's also incorporated in the path validity test that I outlined above.

I have attached the latest proposal for "Recordings.py" ... All feedback and comments welcome.

I probably won't get to have a look at it until tomorrow, Tues.

[IanB]

Code: Select all

    >>> "%04x" % os.stat("/media").st_dev
    '000d'
    >>> "%04x" % os.stat("/").st_dev
    '000b'
    >>>

Code: Select all

       realpath = os.path.realpath(path)
       if (
          realpath.startswith(LocationBox.defaultInhibitDirs) or
          realpath in ("/media", /) or
          not path.rstrip("/").startswith(config.movielist.root.value.rstrip("/"))
       ):
          # reject path

This is still inadequate i.e.1. mount points created within these inhibited directories are unconditionally banned (yeah dumb but ....). i.e.2. unmounted mount points within /media will be allowed but they should not be permitted.

A better algorithm might be to get the .st_dev for all the inhibited directories (root point) then compare with the .st_dev of the target if it matches it really is within the same file system as the inhibited root point.

Also means the inhibited list could be somewhat cleaned up, i.e. /etc, /bin, ... are all the same .st_dev as /'s .st_dev

[prl]

...
This is still inadequate i.e.1. mount points created within these inhibited directories are unconditionally banned (yeah dumb but ....).

Not just dumb: it's something that I wouldn't want done at all. However, it also wouldn't do the right thing if someone mounted a tmpfs on, say, /my_media. It would still allow /my_media to be set as a recording destination, which is not going to work well. Perhaps a test on whether the fs is a tmpfs would help with that. But I think that may be starting to move from "foolproof" to "damnfoolproof".

i.e.2. unmounted mount points within /media will be allowed but they should not be permitted. ...

There is also a test on directory existence that is separate to restriction testing. One of the things IanSav has done in these updates is to make that existence testing work properly.

[IanSav]

Hi,

I have tweaked my code to use the "defaultInhibitDirs" list plus "/" and "/media" to populate a unique list of "os.stat(...).st_dev" entries rather than my previously hand selected list. I think this should be enough protection.

Regards,
Ian.

[IanSav]

Hi Prl,

I believe I have finished the "Recording.py" and "Timeshift.py" refactors so I can now clean up "Setup.py" and get them all ready for peer review then testing.

I'm not sure why you're changing the Screens/Setup.py API by renaming setupdom() and changing the set of methods in Setup, especially things like renaming refill() to reloadList() and removing addItems().

These functions have not been renamed they have been replaced with optimised code that now performs only the function as named. The functions no longer do a hodgepodge of repeated actions needlessly.

I'm not sure why in Screens/Setup.py, loadSetupDOM() (aka setupdom()) isn't written as something like:

Code: Select all

__setupdoms = {}

def setupdom(setup=None, plugin=None):
	global __setupdoms
	if plugin:
		src = resolveFilename(SCOPE_CURRENT_PLUGIN, plugin + "/setup.xml")
		msg = " from plugin '%s'" % plugin
	else:
		src = resolveFilename(SCOPE_SKIN, "setup.xml")
		msg = ""
	print "[Setup] XML source file: '%s'" % src
	print "[Setup] XML menu '%s'%s" % (setup, msg)
	if src in __setupdoms:
		return __setupdoms[src]
	try:
		setupfile = open(src, "r")
		setupdom = xml.etree.cElementTree.parse(setupfile)
		setupfile.close()
	except IOError:
		print "[Setup] ERROR: Unable to access XML file '%s'!" % src
		setupdom = xml.etree.cElementTree.fromstring("<setupxml></setupxml>")
	__setupdoms[src] = setupdom
	return setupdom

To make it more backward-compatible, you could test the last modified time of the setup.xml file against the modified timestamp saved in __setupdoms. Entries in __setupdoms would then be a tuple (or something) like: (dom, modifiedtime).

The section "setupdom" is now gone as external code no longer reaches into "Setup.py" to access "setupdom". It was never a good idea to expose an internal data structure to external access / manipulation.

The loadSetupDOM() code now has the optimisations you suggested.

In Screens/Setup.py:

Code: Select all

###
### Should the isinstance also cover TrueFalse, OnOff, EnableDisable?
###
			if isinstance(self.item[1], ConfigYesNo) or isinstance(self.item[1], ConfigSelection):

I can't see why

Code: Select all

isinstance(self.item[1], ConfigBoolean)

wouldn't be better. The Fine Manual says of isinstance(): "Return true if the object argument is an instance of the classinfo argument, or of a (direct, indirect or virtual) subclass thereof."

Suggestion implemented.

In Screens/Setup.py:

Code: Select all

###
### Should the other queued tasks also be removed?
###

I don't know what that refers to.

Question comment removed as issue does not exist.

I'm not sure why a whole bunch of lines in the imports in Screens/Setup.py are swapped around so much. It makes it harder to see what the actual differences are. Moving the Tools.Directories import to after all the Component imports makes sense, and the line is being changed anyway.

This was done to make cleaning the includes while coding easier. The cleaned up list is now back in a matchable order.

Thank you again for your assistance with the refactor effort.

Regards,
Ian.

[prl]

...
The section "setupdom" is now gone as external code no longer reaches into "Setup.py" to access "setupdom". It was never a good idea to expose an internal data structure to external access / manipulation.
...

I agree about the principle. However, it's usually a good idea to preserve the API, because it may not only be enigma2 base code that calls the function.

[IanSav]

Hi Prl,

...
The section "setupdom" is now gone as external code no longer reaches into "Setup.py" to access "setupdom". It was never a good idea to expose an internal data structure to external access / manipulation.
...

I agree about the principle. However, it's usually a good idea to preserve the API, because it may not only be enigma2 base code that calls the function.

I searched the Beyonwiz, OpenViX, OE-Alliance, OpenATV, Enigma2 plugins and OE-Alliance plugins repositories and there are no other references to "setupdom".

In fact all I found was lots of places where it could / should have been accessed but wasn't. (Versions of the "Setup.py" code are copied or mimicked across Enigma2.) Once the new base code is in place I plan to wander through Enigma2 and try to move all set up based code to use the base "Setup" class. I expect that this should reduce the overall code size while making the set up experience more uniform. All improvements in the base class will be available to all areas of the code that use it.

Regards,
Ian.

[prl]

... I agree about the principle. However, it's usually a good idea to preserve the API, because it may not only be enigma2 base code that calls the function.

I searched the Beyonwiz, OpenViX, OE-Alliance, OpenATV, Enigma2 plugins and OE-Alliance plugins repositories and there are no other references to "setupdom".[/quote]
Fair enough. Personally, I would have kept a setupdom() with equivalent functionality.

In fact all I found was lots of places where it could / should have been accessed but wasn't. (Versions of the "Setup.py" code are copied or mimicked across Enigma2.) ...

That doesn't surprise me at all. Inappropriate cut/paste is so common in the code that sometimes I wonder whether some of the authors have ever heard of subclassing, or even of subroutines.

[IanSav]

Hi Prl,

Fair enough. Personally, I would have kept a setupdom() with equivalent functionality.

I accept what you are saying.

I am enhancing the XML options available to allow the base class to be used in many more applications. While this shouldn't break any potential legacy links to the "setupdom" it does mean that all the "clones" will miss out on the new functionality.

I think it would be appropriate to move the code on and try and encourage the use of the newer mechanisms. Part of my code update will be some documentation on how to use the new "Setup.py" (and future "Menu.py") to encourage its adoption by the Enigma2 community at large.

Regards,
Ian.