Edit #36096 Toolkit Code Review

[manneohrstrom]

Code Review from a Toolkit best practices standpoint. This review takes a high level perspective.

[manneohrstrom]

General comments for import cut

I have done a big pass on the code and here are my summary notes:

I have tried to maintain a high level perspective, particularly trying to consider "best practice" for official tk apps. (I do have quite a few questions about the code itself on a detailed level, but I have tried to maintain a high level perspective here to keep things somewhat organized).

• The use of custom fonts is non-standard. Normally, toolkit apps are designed in such a way that they work in the style of whatever host environment they execute within. Why are custom fonts needed? If this is an attempt to work around shortcomings in DCC integrations, I strongly suggest we look at fixing the integrations rather than patching things up on the app level.
• There are no source file for icon resources. Please add the relevant source files, whether it is graffles, ai files or inkscape. There needs to be a way to go in and edit icons and other resources. For official toolkit apps, we normally keep all resources in a single graffle app.
• Consider a quick cleanup pass on the resources folder - I found several files that were just duplicates of each other. There isn't a consistent naming convention (e.g pencil_24.png, pencil.png, mov_added_icon_30px.png, etc). some entity type thumbnail pngs have background color, others don't.
• In resources/create_entity_dialog.ui: Top level class name should be CreateEntityDialog, not create_entity_dialog.
• The dialog is in a single UI file with EIGHT different stacked widgets. I think at some point it may make sense to split these up into separate widgets for ease of maintenance. I also noticed there is a lot of wizard logic in the code - could the QT built in wizard be used instead?
• There is no configuration information at all in the manifest. this needs to be added!
• There is a missing fw-qtwidgets dependency in the manifest.
• There is no wiki documentation.
• I am worried that the app icon is going to be hard to look at in some contexts with a lighter bg - for example https://support.shotgunsoftware.com/hc/en-us/articles/219039798 - ideally icons should be designed to provide a high contrast viewing experience both on a light and a dark backdrop.
• I see a lot of messages and strings and stuff hardcoded in multiple places in the code. To make future localization easier, i suggest putting them all into a single place, maybe a second constants file just for UI copy?
• The use of the @QtCore.Slot decorator is non-standard and not found anywhere else in the official toolkit apps. Why is this neeed? In a KISS world, I would ideally like to see these decorators removed unless they serve a specific purpose.
• I noticed many places with obvious special business logic called out - special recipes, hard coded number and constants, custom sg fields etc. Most of the time, these areas are uncommented. There needs to be very clear comments explaining why these hard coded values and recipes are there - otherwise it will become very challenging to maintain the app. I am surprised this wasn't covered by the detailed CRs. Please do a pass and annotate.

Comments around code reuse

• The app seems to maintain its own system for worker threads and shotgun operations. I strongly suggest looking into migrating across to the Shotgun Async data retriever and generic threadpool workers that all the official tk apps now use. I strongly recommend setting up a single threadpool on the app level and use that for all async operations. Jeff is a good person to talk about this with.
• Related to the above, it's potentially worth considering using the SG model for faster data retrieval of cut values.
• The app is using its own system for logging. Now that 0.18 is out, it would be great to start looking at migrating to the 0.18 logging setup. We have discussed at great length and carefully tried to consider this particular use case in the context of the logging feature set, so hopefully it should be relatively straight forward to make the change. Details about logging here.
• I noticed many places where there is hard coded logic for behaviour around display names, defaults, status colors etc. There is a globals module that can be used for this now. I strongly suggest looking into using this - cases not yet supported by the module could be added for the benefit of all toolkit users. Again, Jeff is a good person to talk to as he has recently been a 'client' of this module.
• The tk-framework-editorial also needs to be reviewed as part of this and released publicly at the same time as we release import cut. I took a quick look and perhaps most importantly, there needs to be sphinx API docs. I guess we should go through a similar CR process like we are doing here.
• I noticed code that seems like it should really belong inside the editorial fw and not in the import cut app - logic for diffing cuts, logic that wraps around and objectifies the shotgun cut data model etc. - if we can get that out into a single officially supported framework that's a big win!
• There seems to be many contenders for reuse - cases where other apps could benefit from what exists within import cut. I don't have the full picture here, but i noticed things like the card widget system, the animated stack widget, etc. We should get a clear plan together here and begin moving this across.

[sgsteph]

Thanks Manne, some quick answers to your questions:

• Custom fonts: this is a temporary workaround I think. Agree they shouldn't be kept here.
• Missing icon source files: Agree, svg were once provided for every single icon. This was lost when icons were recently changed. I would prefer svg files to Graffles, as it is supported by different applications.
• Cleanup on resource files: Agree naming and icons (bg vs no bg ) should be consistent.
• There is no usual TK settings for this app. They are all controlled with user settings. Looks like this comment https://github.com/shotgunsoftware/tk-multi-importcut/blob/master/info.yml#L12 should be made more visible.
• A user documentation is being written and we should generate a Sphinx documentation from docstrings.
• The wizard is a bit special and not a real wizard, some screens were recently merged together, so it might be worth waiting a little before switching to a better UI approach, if there is one.
• qtwidgets fw seems to be in the info.yml https://github.com/shotgunsoftware/tk-multi-importcut/blob/master/info.yml#L37 ?
• We already talked about @QtCore.Slot, it is actually standard, but not used in TK apps. This is just my point of view, but I like to clearly see what is used as a slot. A bit of efficiency is gained from using these decorators (faster, smaller memory footprint). I guess the decision to keep or discard them should be left to the team who will take ownership of this app.
• Special undocumented constants and other magic stuff: will check what you mentioned in your CR comments.
• Using latest TK core features and latest framework features: sure new features should be checked and used if possible (some of them were not available when the app was written). So far, we just focused on making the existing code as clean as possible, which was already quite a piece of work :)
• I'm not sure if a lot of the logic should make it into the editorial fw, may be just making sure we have a standard way to publish Cuts in Shotgun ? The framework itself should definitely be reviewed as well.