@Richard Antalik (ISS) could you take a look at that?

Hi Brendon! Long-time no see.

Richard told me he'd review this task, and that he needed help to do add-on review as it's his first time.

Just updated the zip to PS 1.3, with a few bug fixes and other improvements.

Thanks Brendon. ?

Have you looked at https://wiki.blender.org/wiki/Tools/Git and https://wiki.blender.org/wiki/Developer_Intro/Overview ?

Yup, I use git daily on the job. Also already made small contributions to the VSE, looking to do more.

Are you prepared to maintain the addon within Blender through the 2.8 series?

Yes, we edit with Blender with my teammates anyway.

I started editing videos with the latest master and no problem so far.
When you're about to review the add-on, please ping me so I can update the code beforehand: I'm making changes almost daily lately. The code structure and style is going to be the same (one file per operator), but e.g. I put all utility functions in a single module and added a draw module recently.

Or you can find the most up-to-date code here: https://github.com/GDquest/Blender-power-sequencer

Hi, I updated the add-on above, here's a copy:

I removed the updater and tested that it was working. Should be good for review now. Thanks!

This is fairly large for an add-on so I only did a high-level review.
Here are some issues I've run into.

• Includes files not part of the add-on:
  • Binary file GTAGS, other files in this directory too ... )
  • pyproject.toml

• Don't include the full GPL LICENSE, use a short header at start of all files. As we do everywhere else in Blender.
  
  ... this goes for sub-modules that are including their own LICENSE files too.

• Why is pyperclip module needed? Blender has it's own clipboard access.
  
  https://docs.blender.org/api/master/bpy.types.WindowManager.html#bpy.types.WindowManager.clipboard

• No need for changelog.md, we normally rely on git commit history.
• README.md - not sure of it's purpose when bundled with Blender, it's unlikely users will even find this. Better to have the add-on link to good online documentation.
• In handlers the functions are converted to a string to check their identity. It would be better to store them in a list and remove them directly, or if thats not possible - check their __module__ and __name__, so you don't accidentally have naming collisions w/ other add-ons.
• Noticed redundant object subclasses, this is redundant and was only needed for Python2 compatibility.

I updated my build script to clean up all the extra files, added the license headers, and made all the changes:

Noticed redundant object subclasses

I only found one with grep in ProgressBar, removed it. Were there any others?

Why is pyperclip module needed?

It was old code, I didn't know at the time, that's fixed.

@Nathan Lovato (gdquest) could you commit to a temporary branch? tracking changes between updates to a zip-file doesn't work well, it's inconvenient to see what's changed.

Use temp- in the prefix, eg temp-power-sequencer

Sure, I'll do that after work! Thanks

It's up in the addons-contrib repo, branch temp-power-sequencer, I added a power_sequencer directory: https://developer.blender.org/diffusion/BAC/browse/temp-power-sequencer/

Went over the code in some more detail, generally seems OK.

Note that there are checks in the poll functions we wouldn't accept in Blenders code (C or Python):

For example:

@classmethod
def poll(cls, context):
    try:
        next(s for s in context.selected_sequences if s.type == "META")
    except StopIteration:
        return False
    return context.sequences

The reason for this is an operator may be exposed in a toolbar on in a panel, making every redraw have to loop over all sequence strips (potentially twice in this case), just to check if the button should show as enabled or not.

When the operators are called from menu items it's not an issue in practice, however UI layouts change and it's sometimes useful to show buttons in panels/toolbars.

Occasionally we have reports that Blender runs slow and it turns out to be add-ons doing inefficient tests like this.

Don't think this is a blocker for committing to add-ons though.

Thanks, I'm aware of that and wouldn't do it in the official source code. This looks like something I did a long time ago, I'll clean it up.

While I'm around, is it okay to check the length of lists of sequences? Richard told me that in Blender's sources we can't use the sequence lists at all for performance reasons. E.g. this wouldn't be okay: return len(context.selected_sequences) > 1

I'm asking because I'm looking to contribute some code from the add-on to the VSE, so if it's not okay I can start changing that in the addon, which is great to get people to test the tools and find bugs before creating patches.

Re: checking lengths:

• from contexts all lists are created fully before access. So context.selected_sequences will create and free a everything list no matter what you do with it.
• in the case of RNA collections, we use a sequence type, where:
  • bool(view_layer.objects) is O(1)
  • len(view_layer.objects) is O(N)

... so always use bool(seq) instead of len(seq) > 0

Got it, makes total sense. You tend to forget how things work at a lower level working with Python, at least until you hit performance issues. So even if poll gets context as an argument, until you access a given list or collection, it isn't even initialized, right. I'll be mindful of that when contributing to the VSE.

I just updated the code in the temp branch to remove all the len() checks and other try/except in operator polls, tested that the addon works:

• Is there anything left to clean up, or can I delete the temp branch and push the addon?
• If so, can I push the addon straight to the blender-addons repository?

Thanks for taking the time to review my work :)

In T68584#769812, @Nathan Lovato (gdquest) wrote:

• Is there anything left to clean up, or can I delete the temp branch and push the addon?
• If so, can I push the addon straight to the blender-addons repository?

Yes, think this is fine to push now. The temp branch can be deleted.

Done!