Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added error handling to Preset and Profile file reading operations #3342

Merged
merged 4 commits into from
May 16, 2020

Conversation

kartchnb
Copy link
Contributor

@kartchnb kartchnb commented Apr 3, 2020

Added code to catch exceptions when loading profile and preset files.
Otherwise, OpenShot will silently fail to perform certain operations
when invalid or malformed files are encountered.

Added code to catch exceptions when loading profile and preset files.
Otherwise, OpenShot will silently fail to perform certain operations
when invalid or malformed files are encountered.
Copy link
Contributor

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kartchnb, this makes sense to me and overall looks good. I tested it vs. 2.5.1 and with a malformed preset (XML) file, this code is the difference between the Export dialog opening with an error logged, and just refusing to open at all, so definitely a needed change.

I popped in a couple of comments (at least one of which is entirely rhetorical) and one minor code-organization suggestion, but ultimately I leave it up to you — I'd be fine with this going in as-is (rebased onto develop).

Comment on lines +497 to +501
try:
xmldoc = xml.parse(preset_path)
title = xmldoc.getElementsByTagName("title")
if _(title[0].childNodes[0].data) == selected_target:
profiles = xmldoc.getElementsByTagName("projectprofile")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the try: added in, this is starting to get really deeply nested, so the only thing I'd suggest is maybe going with:

Suggested change
try:
xmldoc = xml.parse(preset_path)
title = xmldoc.getElementsByTagName("title")
if _(title[0].childNodes[0].data) == selected_target:
profiles = xmldoc.getElementsByTagName("projectprofile")
try:
xmldoc = xml.parse(preset_path)
title = xmldoc.getElementsByTagName("title")
if _(title[0].childNodes[0].data) != selected_target:
continue
profiles = xmldoc.getElementsByTagName("projectprofile")

Then everything that follows can be outdented a level. (Or, outdented back to its previous level, really.)

Comment on lines +387 to +393
try:
xmldoc = xml.parse(preset_path)
type = xmldoc.getElementsByTagName("type")

if _(type[0].childNodes[0].data) == selected_project:
titles = xmldoc.getElementsByTagName("title")
videocodecs = xmldoc.getElementsByTagName("videocodec")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, re: nesting

Suggested change
try:
xmldoc = xml.parse(preset_path)
type = xmldoc.getElementsByTagName("type")
if _(type[0].childNodes[0].data) == selected_project:
titles = xmldoc.getElementsByTagName("title")
videocodecs = xmldoc.getElementsByTagName("videocodec")
try:
xmldoc = xml.parse(preset_path)
type = xmldoc.getElementsByTagName("type")
if _(type[0].childNodes[0].data) != selected_project:
continue
titles = xmldoc.getElementsByTagName("title")
videocodecs = xmldoc.getElementsByTagName("videocodec")

@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 4, 2020

I'd be fine with this going in as-is (rebased onto develop).

Said rebasing is now completed.

@kartchnb
Copy link
Contributor Author

kartchnb commented Apr 4, 2020 via email

@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 4, 2020

Thanks for your comments, Frank. It looks like you've already incorporated my changes, so I'll just respond to your first comment. As-is openshot.Profile() doesn't reliably throw RuntimeErrors on malformed Profile files at all. Along with this change, I submitted a pull request for libopenshot which makes openshot.Profile much more robust in detecting and ignoring bad Profiles. in both of your cases, RuntimeError would be correctly thrown.

Oh! Excellent, I'll definitely take a look for that.

If you're interested, the reason I made this change in the first place is because, in my personal build of OpenShot, I want to be able to have a project-specific Profile saved in my project directory. I've found the need to create a lot of my own one-off Profiles and, rather than cluttering up my Profiles directory, thought it would be better to just associate it directly with the project. Since OpenShot Profiles don't have an identifying extension, I have to read through every file in my project directory to check for a valid Profile, necessitating this change.

Yeah, I'm tempted to say "Well, let's add an extension!" but that's not really a solution to the issue unless we wanted to define a unique extension for the profile file format — which I, at least, really don't think I do.

It's better that profiles be placed in a well-defined location, in the same way preset XML files also still live in a certain directory. Not all XML files are presets, so even having an extension doesn't help determine which files need to be handled, unless there's also a location to look for them.

However, I notice from this PR that you're working with OpenShot 2.4.4 as your baseline. As of OpenShot 2.5.0 (and the current 2.5.1), generated project files (thumbnails, titles, blender-rendered images) are now consolidated into a Project Name_assets directory that's written alongside the Project Name.osp file when the project is saved. The natural place to store per-project profiles would be as Project Name_assets/profiles/profile_name files. Then you'd only have to look at any files present in Project Name_assets/profiles/, and you should be able to assume they're all profile definitions.

@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 4, 2020

Thanks for your comments, Frank. It looks like you've already incorporated my changes, so I'll just respond to your first comment.

I only incorporated the changes from our develop since you'd branched into your PR, to resolve the pending merge conflicts — I didn't apply any of the changes I suggested above, I prefer to leave those up to you. But they're still relevant. (Well, except the rhetorical one about Expat, which never was.)

@kartchnb
Copy link
Contributor Author

kartchnb commented Apr 5, 2020 via email

@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 5, 2020

Ah. A profiles directory inside the project directory would be ideal. Looks like I'll have to figure out how to update to OpenShot 2.5.

Definitely go to 2.5.1, not 2.5.0, if you do update. 2.5.0 has a bug in the project data handling for non-ASCII pathnames, which 2.5.1 was rushed out to fix.

Comment on lines +394 to +395
for title in titles:
project_types.append(_(title.childNodes[0].data))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for title in titles:
project_types.append(_(title.childNodes[0].data))
project_types.extend([_(t.childNodes[0].data) for t in titles])

Oops, only just noticed this one. Far more Pythonic to say...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I realize you didn't write that line originally, but if we're touching it, might as well make it pretty.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is c++ innovation that were adopted in Python, so it is not "Pythonic" in any way, while it still may look like this. This kind of code is harder to read than the original one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Masking append to extend behind "beautify" of the code.

Copy link
Contributor

@ferdnyc ferdnyc May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the list is constructed by a loop regardless, I agree it's less impactful. however .extend() isn't just a different way of writing .append() in a loop. List concatenation is more efficient than repeatedly appending individual items, primarily because the memory allocation required to enlarge the destination list can be done all at once.

The larger the list you're adding to, and the larger the list of items you're extending it with, the more benefits this has. And while you benefit the most if the lists are preexisting, even when looping to construct the list of additions, doing that first and then adding it to the destination list is preferable.

IOW, if your objection is primarily to the comprehension, then this version without it is still preferable, because it will avoid repeated small allocations to increase the size of project_types

title_types = []
for t in titles:
    title_types.append(_(t.childNodes[0].data))
project_types.extend(title_types)

The comprehension is just a cleaner way of writing that concisely, though I realize not everyone cares for them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listA.extend(listB) is literally ten times as fast as .append() in a loop. And even if you loop to create the second list, doing that first and .extend()ing afterwards takes only half as long as the looped .append() on the destination list.

These timeit runs use exaggeratedly large lists, so real-world this won't have nearly as big of an impact, but...

# listA = a million integers from 0...999999
# listB = another million, from 2000000...2999999
#
# Looping listA.append() for each item in listB
>>> timeit.repeat('for n in listB:\n    listA.append(n)', setup='listA = [x for x in range(0, 1000000)]\nlistB = [x for x in range(2000000, 3000000)]', number=100, globals=globals())
[6.985341801017057, 7.013206199044362, 6.989441379031632, 7.000579444982577, 6.9871047679916956]

# listA.extend() with the entire contents of listB
>>> timeit.repeat('listA.extend(listB)', setup='listA = [x for x in range(0, 1000000)]\nlistB = [x for x in range(2000000, 3000000)]', number=100, globals=globals())
[0.6946912699495442, 0.6940702060237527, 0.694431746029295, 0.6942416160018183, 0.6937777320272289]

# listA.extend() with a list-comprehension copy of listB:
>>> timeit.repeat('listA.extend([n for n in listB])', setup='listA = [x for x in range(0, 1000000)]\nlistB = [x for x in range(2000000, 3000000)]', number=100, globals=globals())
[3.4050588610116392, 3.4058208650094457, 3.4050478630233556, 3.404870489030145, 3.405109421000816]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, again, I'm not saying this actually matters in this particular case. It doesn't. We're talking about small lists here, where it really doesn't make the tiniest bit of difference how you build them. But certain code patterns have more efficient alternatives that are typically preferable. Taking a blanket position of, "append() is clearer than extend()" ignores the fact that they're not even close to equivalent in terms of performance/efficiency, because there are times when that will matter.

Comment on lines +396 to +409
for codec in videocodecs:
codec_text = codec.childNodes[0].data
if "vaapi" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-vaapi.svg")
elif "nvenc" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-nvenc.svg")
elif "dxva2" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-dx.svg")
elif "videotoolbox" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-vtb.svg")
elif "qsv" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-qsv.svg")
elif openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-none.svg")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urrrrvery single one of these thing.childNodes[0].data references here, above, and below, can all be just thing.firstChild.data, FWIW.

(Also not your code originally, of course. But you just had to go and touch it, so now you get the commentary. 😉 )

Comment on lines +501 to +502
for profile in profiles:
profiles_list.append(_(profile.childNodes[0].data))
Copy link
Contributor

@ferdnyc ferdnyc Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for profile in profiles:
profiles_list.append(_(profile.childNodes[0].data))
profiles_list.extend([_(p.firstChild.data) for p in profiles])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second time you masking append to extend This isn't good.

Comment on lines +506 to +507
for profile_name in self.profile_names:
profiles_list.append(profile_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for profile_name in self.profile_names:
profiles_list.append(profile_name)
profiles_list.extend([n for n in self.profile_names])

Comment on lines +561 to +563
except ExpatError as e:
# This indicates an invalid Preset file - display an error and continue
log.error("Failed to parse file '%s' as a preset: %s" % (preset_path, e))
Copy link
Contributor

@ferdnyc ferdnyc Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't actually display an error anywhere except the console output — which means only in the logfile, for Windows users, since they don't even get console output. (Ditto macOS and Linux users who launch OpenShot graphically, but at least they have the option.) That's probably a good thing, most of the time — we don't really want to be throwing error dialogs up at users because of a profile parsing issue. There are already spots in the code that show dialog messages for errors loading icons, something I've argued against because what is the user expected to do about that!?

But in this case, especially if it's one of the user's own profiles, it might be good to give them some indication that there's an issue, because that's something they can correct. So, it might be worth showing an unobtrusive error message (localized, also) in the status bar of the main window. It's a sadly-underutilized means of communicating information without interrupting the user or forcing them to deal with the message immediately. You can see it used in the main window's code for handling the save frame / snapshot preview feature, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's right, and also in the Recent Project list handling. This bit here:

log.info("File not found at {}".format(file_path))
self.statusBar.showMessage(_("Project {} is missing (it may have been moved or deleted). It has been removed from the Recent Projects menu.".format(file_path)), 5000)
self.remove_recent_project(file_path)
self.load_recent_menu()

Copy link
Contributor

@ferdnyc ferdnyc Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's code in __init__, I believe, that sets up self.statusBar for the class.

Come to think of it, you could just use get_app().win.statusBar to access it from anywhere. Only thing I'm not 100% sure about is whether that'll work while the export dialog is open. If not, then meh, the log message is fine.

@jonoomph
Copy link
Member

jonoomph commented May 9, 2020

@kartchnb I've fixed the conflict in this branch (against develop), and I'm ready to merge this one. I just wanted to double check you are ready for this to be merged? Any other changes that are still pending on this one? 👍

@kartchnb
Copy link
Contributor Author

kartchnb commented May 11, 2020 via email

@jonoomph
Copy link
Member

@kartchnb Can you double check this PR 1 more time please. I just resolved another conflict, and want to ensure I didn't mess up your changes. Thx!

@jonoomph
Copy link
Member

Merging now. I'll double check all works good once merged.

@jonoomph jonoomph merged commit ea3ee9c into OpenShot:develop May 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants