Closed sgsteph closed 8 years ago
Finished looking at the code. Besides code stuff, I noticed the following issues when interacting with the app:
Need to update the README.md
tk-multi-importcut/app.py
The app entry point. This class is responsible for initializing and tearing down
the application, handling menu registration, etc.
[x] L27: comment update ("Toolkit" should be capitalized in comments throughout)
# Toolkit's code reload mechanism will work properly.
[ ] L36: PEP8 update. As per PEP8 this should probably be:
def menu_callback(): app_payload.dialog.show_dialog(self)
But I know historically we use lambda
in other apps as well. So for consistency we leave it or for "correctness" we change it?
We use lambda like that in lot of places, so I think this is one these PEP8 rules that we don't follow
[ ] L55-58: bugfix. Won't this create a "menu item" like Import Cut_with_args
? If this is for the shell engine then wouldn't we want to use short_name
instead so the menu item is import_cut_with_args
?:
self.engine.register_command(
"%s_with_args" % short_name,
self.load_edl_for_entity,
settings)
Actually, I see this doesn't cause a shell engine menu like Import Cut_with_args
, I'm guessing because the shell engine does some of this for you automatically(?), but might be cleaner to provide the right name anyway?
I think this should be left for later, when we will revisit the Premiere integration
[x] L62-63: comment update. Consistent capitalization of EDL should also be used throughout the app.
Allow import cut to run with pre-selected EDL file path, SG entity,
and frame rate for the EDL file.
[ ] L66-68: parameter type question. It feels very weird and dirty to be passing a dict
in as a str
. I'm assuming there's a process that is driving this but can we do better than this? At the very least the var name should be something like sg_entity_str
as I would automatically assume sg_entity
is a dict
. An explanation of why it's coming in as a str
would help as well.
For the shell engine workflow, it feels like it would be more consistent to pass in entity_type
and entity_name
or @entity_id
so it matches with the standard syntax for the tank
command. I think it's awkward to have to provide the dictionary as a string. But I know I'm missing a piece of this.
The frame_rate
I would also assume to be an int
not a str
. So we need to document that better at least. Though again, would love to discuss this a bit to see if there's a better way.
Agree with your comments, but, again I think this should be left for later, when we will revisit the Premiere integration. Chances to break it are high
python/tk-multi-importcut/edl_cut.py
These are all comments on code not specifically part of the PR. Sorry, it's a fair amount of stuff with varying degrees of importance. Some of this is potential group discussion but any disagreements with my own opinions just let me know. I could very well be wrong! 👼
My suggestions are written (usually) to match the existing code style but if any of the below style comments change for consistency then of course my suggestion should follow the same format.
[ ] Line Width: The line widths seem to be a little erratic. This is mostly visible with comments but there's also code that has varying line widths as well. For example:
I tend to use 80 characters with a bit of margin, so I guess close to your 100. I would say readability is the key here, and the only thing which should be prohibited is super long lines exceeding by far 80 chars
[x] Inline Comments: While pep8 says they're ok, consensus says try not to and I think generally things are cleaner without them. Multi-line comments shouldn't be on the same line as code.
Agree, seems to be one off, or did you spot it in other places ?
[ ]: 🔥 💣 Not trying to start a big thing with this but first and foremost is we just need to be consistent and there's some inconsistency happening in general. What is our preference since both are valid pep8 styles?
After testing various styles (hence different styles here), I really like the latter, which is very readable, has the advantage to have the closing symbols on a dedicated, instead of being mixed up with the code, and help keeping lines short
()
s rather than using \
at the end of each line to indicate a continuation.
agree and removed them[x] L61:
# Emitted when the data manager is busy doing something. Optionally with an
# integer describing a known "range" of progress so a progress bar can be shown
[ ] L180:
get_clip_name
is never used._I think _sg_version, _shot_name are not standard members of EditEvent and are added on the fly when accessing them here, and setting up lambdas. Again something may be to revisit in the editorial fw, but _sg_version and _shotname shouldn't be in the EditEvent base class, as it has nothing to do with edtiorial, and is just comments interpretation
preferred_match
[x] L203:
# to worry about compiling it ourselves for performances considerations
[x] L289 no need for parentheses:
self._logger.warning("Couldn't find any entry in %s" % edl_file_path)
[x] L315:
use defaultdict
(standard in 2.5) here so we don't have to do the tedious if/else
blocks:
from collections import defaultdict
versions_names = defaultdict(list)
versions_names[v_name].append(edit)
Or if we don't want to bring in another module use setdefault()
:
versions_names.setdefault(v_name, []).append(edit)
Nice ! Thanks for the tip !
[ ] L349: Would a log message be useful here to indicate that the linked Version's Shot doesn't match what we thought the Shot was from the EDL?
This is expected behavior so I don't feel like a log message is needed
[ ] L393:
We shouldn't assume the entity type name won't contain spaces. For example "Asset Library" would
produce the field name sg_asset library
(notice the space).
Thanks for the heads up, I strongly think that SG field names shouldn't contain any special character, including spaces, and only allow underscores. Very tempted to create a field with an emoji in it and see what happens... Could be wrong, but here the code would not break, even if there is a space in the name ?
[x] L395: This would read better on 2 lines:
if (field and field["data_type"]["value"] == "entity" and
self._sg_entity_type in field["properties"]["valid_types"]["value"]):
Changed it but find it very hard to read 👎
[ ] L431:
I think the last or ""
is redundant and won't ever be hit.
_Meaning that we have the guarantee that any Entity has either a sg_status_list or sgstatus field ? Being paranoïd, I prefer to keep it, as it does no harm
[kp] Not exactly... meaning that if the Entity has no sg_status_list
field it will fall back on sg_status
, and if there's no sg_status
field, it will return ""
as part of the sg_entity.get("sg_status", "")
so the or
clause won't ever be reached.
[x] L487: We do this in a few different places in the code. Seems like a useful candidate for a util function?
shot_name = utils.get_display_name(entity=my_shot, default_value="")
The pattern is used for other fields too, so you could even make it more generic:
value = utils.get_field_value_with_fallback(entity=my_shot, fields=["sg_status_list", "sg_status"], default_value="wtg")
[ ] L508:
Is it worth doing the same thing you did for the constants._SHOT_FIELDS
here? Are there any
other entity types where it's worthwhile storing field queries like this?
I introduced the constant for Shots because they are retrieved in two places. I think other Entities are only retrieved from single places
(self.entity_name)
self._sg_cut
isn't previously defined as an instance variable. We should set it in the __init__()
.[ ] L614: Reference to internal ticket #?
Nope, no ticket to reference I'm afraid
[ ] L620:
FYI, entity
gets you the display name of the entity in the dictionary, eg. {'type': 'Version', 'id': 2, 'entity': {'type': 'Shot', 'id': 860, 'name': 'bunny_010_0010'}}
so you don't technically need entity.Shot.code
.
_Yes, but I find it utterly confusing to access the name of an Entity with something else than it's canonical name field, e.g. sg_shot["name"] vs sgshot["code"]. Would recommend to always canonical fields as best practice
[ ] L663:
Curious why if sg_cut_item["shot"]
is True
, we still check sg_cut_item["shot"]["type"] == "Shot"
?. Seems like it's redundant.
Some legacy leftover I think from when CutItem were CustomEntities and so the field was a custom field. Could users make the "shot" field accept other Entities than Shots ?
[kp] OK. FYI the current Shot
field on CutItem does not allow changing the entity type.
[ ] L689:
Use defaultdict
or dict.setdefault
In this case we really want to know if the key already exists or not, so I don't think defaultdict
would help ?
cut_diff
is never used.cut_diff
is never used.cut_diff
is never used.[x] L764:
# Process Cut Items left over
So this is all of the Cut Items that were in the last Cut but are not in this one? Could we add a little more info to this comment explaining how leftover Cut Items exist? Basically if they were not matched to an edit
()
s around multi-line conditionals vs using the \
character for every line continuation. I think this is something else we should be consistent with. [ ] L768:
Do we have to check that the CutItem.shot
is an entity of type "Shot"? Even if we're being safe, the field definition should enforce this.
Can it be changed by users ?
[kp] As of Shotgun v7.0, no the field is locked to only allow Shots. But assume there may be some legacy to support here based on previous comments.
[x] L773:
# We found a matching existing Shot.
cut_diff
is never used.[x] L789: Maybe explain a little more in the comment. So these are leftover Shots that were in the last Cut that are not in the current Cut?
Replaced the block with a warning, as this shouldn't happen, and was some legacy code leftover
cut_diff
is never used.[x] L834:
\
s for multi-line conditionals. if (sg_cut_item["shot"] and sg_shot and
sg_cut_item["shot"]["id"] == sg_shot["id"] and
sg_cut_item["shot"]["type"] == sg_shot["type"]):
# We can have multiple cut items for the same shot
# use the linked version to pick the right one, if
# available
if not sg_version:
# No particular version to match, score is based on
[ ] L846:
Since we have a _get_cut_item_score()
method it feels like we should keep all of the scoring
in there rather than modifying the results here for bonuses. Maybe pass in a bonus (and
a bonus reason?) so you just get the score back?
Should the bonuses be local constants? BONUS_NO_MISMATCH
, BONUS_VERSION_MATCH
, etc.
Feel like that is is a bit overkill, and the code is easy to understand maintain in its current state
[x] L863:
Will keep looking but we keep a reference to the Cut Item since it's linked to the same Shot
[x] L881:
# Prevent this one from being matched multiple times
Comment on line above code.
[x] L893:
So the best score is 3 if all of them match
[x] L902:
# Compute the cut order difference (edit.id is the "cut order" in an EDL)
(
(
[x] L946:
# When testing this app it is time consuming to create
# all required Versions in SG. Uncomment the following line to
# create missing Versions automatically for you, which can be
# handy for testing.
[x] L986-990: This feels a little awkward. Could we use this instead?
for sg_link in sg_links:
links.append("%s/detail/%s/%s" % (self._app.shotgun.base_url,
sg_link["type"],
sg_link["id"])
str(None)
returns "None" which isn't what we want to set?[ ] L1053-1054: Is the use of no slate and a start frame of 1 convention? Is this documented anywhere?
Not sure... This is a movie, so starting a frame 1 makes sense though
[x] L1074-1077:
self._sg.upload(
sg_cut["type"],
sg_cut["id"],
self._edl_file_path,
"attachments"
)
[x] L1116:
:param update_shots: Boolean indicating whether or not Shot fields should be updated
[x] L1126: Indentation of continued lines should match with variable name not parenthesis:
(sg_shot,
min_cut_order,
min_head_in,
min_cut_in,
max_cut_out,
max_tail_out,
shot_diff_type) = items.get_shot_values()
or
(sg_shot, min_cut_order, min_head_in, min_cut_in, max_cut_out,
max_tail_out, shot_diff_type) = items.get_shot_values()
(
[x] L1142-1146:
# Cut diff types should be the same for all repeated entries except maybe for
# rescan / cut changes. However, we do the same thing in both cases, so it doesn't
# matter. Head in and tail out values can be evaluated on any repeated shot
# entry so we arbitrarily use the first entry
[ ] L1161:
On create, SG entity records automatically have their created_by
and updated_by
set to the
currently authenticated user. Actually updated_by
isn't settable at all. Only created_by
is
settable and only when calling create()
.
Coming from pre-authenticated user TK to avoid created by/updated by set to 'toolkit 1.0'. Keeping it as it is for the time being
[x] L1230-1232:
else:
# Cut change or rescan or no change.
# Add code and status in the update so it will be
# returned with batch results.
entity
key.[ ] L1356-1357:
Again, created_by
is automatically set to the currently authenticated user, so is this necessary?
Providing updated_by
will have no effect.
Coming from pre-authenticated user TK to avoid created by/updated by set to 'toolkit 1.0'. Keeping it as it is for the time being
tk-multi-importcut/python/cut_diff.py
Some general comments that apply to this as a whole:
new_tail_out()
, shot_tail_out()
, and default_tail_out()
. As well as cut_in()
and new_cut_in()
, etc.
Sometimes we reference the CutItem, sometimes the Shot, and sometimes the Edit. It's easy to get confused by what is a previous value and what is part of the new change about to be applied. It's logical math but with a few different pieces
and similar terminology, it's easy to lose track. My concern is this should be understandable by an engineer with little knowledge about how editorial works. We should have all of the information in here to enable them to work.[x] head_duration
, tail_duration
, duration
: Some are inclusive, others aren't - do we have an explanation of which are inclusive/exclusive and why?
They are all inclusive, I added some comments about the simplified formula which led you to think some were exclusive
[x] L78-79
If a shot is repeated, that is, it appears more than once in the cut (e.g. for
flashback effects), a single "media item" is associated with all the entries
[x] L102
# Emitted when the repeated property for this item is changed
[x] L145 Inline comment feels out of place
# List of other entries for the same Shot
self._siblings = None
RuntimeError
[x] L361-362
# Default case: retrieve the value from the Shot or fall back to the default
[x] L384 Again, I don't understand how this is the new tail_out when it's just pulling from the Shot.
Same answer as above
[x] L426
associated cut item, or None
[x] L441
associated cut item, or None
[x] L471
# get its tc_cut_out
[x] L694
# If this is not the last entry
[x] L760
# a shot. If not, then this is not a VFX shot entry
[x] L793-794 Is this grouping defined/described anywhere?
Added some explanation in the docstring
!
(sorry Stéphane)[x] L820
The name _check_changes()
implies something will be evaluated but that nothing will be actually changed. However, this is not the case as the _diff_type
of the current instance is changed (as well as other possible instance variables). I would rather see this named something more like _process_diff_changes()
or a similar name that implies changes will be made.
_Renamed to check_and_set_changes
_
[x] L841
retrieve()
can take a default value so:
omit_statuses = self._user_settings.retrieve("reinstate_shot_if_status_is", [])
_What if reinstate_shot_if_status_is
is set in settings to None
? I didn't write this, so not sure it is safe to change it_
[x] L852
# If any of the previous values are not set, then assume they all changed (initial import)
[x] L853-856
()
over \
for line continuation if (self.cut_order is None or self.cut_in is None or self.cut_out is None or
self.head_duration is None or self.tail_duration is None or self.duration is None):
self._diff_type = _DIFF_TYPES.CUT_CHANGE
return
[ ] L858-862 Can we remove this now? It's in the history so we can always get it back.
I prefer to keep it around until we know for sure we don't want it. Coders will have to know it existed at some point to search in the history
[ ] L910, L912
We have the self.repeated
property to use, why do we bypass it here?
repeated
used to check the length of siblings. Could be changed now, not sure if it is really needed ?
tk-multi-importcut/python/dialog.py
Some general comments that apply to this as a whole:
:
:param:
definitions [ ] L26-29 Unused imports
they are actually used to promote widgets from Designer
:param:
definition[x] L386-388
# We always want to be able to import against the Project. We want Project
# to always be the last button, so we append it after sorting is done.
max_count
, should we ensure that we get Project in there as the last button before we truncate?:param:
definitionMOV
is a little ambiguous as it could imply only .mov
files are accepted. Maybe say "Movie"?
sorry, sticking to the design, and matching the label in the drop areafor
loop with a little added smarts to trim it down?
added helper method and moved most of the logic in itstr()
here since the formatting will automatically convert it.
do you feel strongly about this ? I like the explicit cast, even if not neededstr()
here since the formatting will automatically convert it.
same comment as aboveos.path.basename()
as we've already applied it hereos.path.basename()
as we've already applied it here[ ] L1006-1011 What if we created an entity either here or in Shotgun in preparation for doing an import? sorry, I don't get it ?
[kp] I mean what if the app was started and then the user creates an entity in SG realizing they'll need it for the import. I don't think that will show up in the list because we have potentially already loaded up the list of entities.
type_name
is unused. Was it intended for the log message or the sequence_label
?
_both I guess, this is the case now, and I renamed the widget to selected_entitylabel[x] L1267
# ignore close event to stop it from being processed
evt.ignore()
[ ] L1327
Instance params normally defined for the first time in __init__
.
Sorry, don't get it
[kp] I just meant that instance variables are usually best defined for the first time in the __init__()
not later in the code. (minor).
e
is unused # stop watching it. Check if the file is watched, re-attach it if not
tk-multi-importcut/python/cut_summary.py
Some general comments that apply to this as a whole:
:
:param:
definitions [ ] L91
_update_min_and_max()
doesn't this cover any updates that might be needed?You might be right. However, this code was changed in a pending branch, so not changing it here fro the time being
<10
, so the optimisation wouldn't be big here[ ] L197-214 This approach seems more succinct. (Didn't double-check the calculations though so I may be missing something).
sg_shot = next((cut_diff.sg_shot for cut_diff in self if cut_diff.sg_shot), None)
min_cut_order = min(cut_diff.edit.id for cut_diff in self if cut_diff.edit)
min_head_in = min(cut_diff.new_head_in for cut_diff in self
if cut_diff.new_head_in is not None)
min_cut_in = min(cut_diff.new_cut_in for cut_diff in self
if cut_diff.new_cut_in is not None)
max_cut_out = max(cut_diff.new_cut_out for cut_diff in self
if cut_diff.new_cut_out is not None)
max_tail_out = max(cut_diff.new_tail_out for cut_diff in self
if cut_diff.new_tail_out is not None)
Except it involves looping multiple times over the list of CutDiffs, this is why it's not used here
[x] L219-224
# Special cases for diff type:
# - A Shot is NO_LINK if any of its items is NO_LINK (should be all of them)
# - A Shot is OMITTED if all its items are OMITTED
# - A Shot is NEW if any of its items is NEW (should be all of them)
# - A Shot is REINSTATED if at least one of its items is REINSTATED (should
# be all of them)
# - A shot needs RESCAN if any of its items need RESCAN
if
condition since we don't want to override RESCAN
?[x] L431-435
# Force a lowercase key to make Shot names case-insensitive. Shot names
# we retrieve from EDLs may be uppercase, but actual SG Shots may be
# lowercase.
# We might not have a valid Shot name if we have an edit without any
# Shot name or Version name. To avoid considering all these entries
[ ] L437
Could use defaultdict
here (though the existing code also works in this instance I think)
I don't feel like defaultdict
would make the code simpler here ?
[ ] L439 Do we need to do this again if we've already done it? For ex. if we have 3 cut_diffs, the should already have repeated=True from previous operations. Could this then just be:
if len(self._cut_diffs[shot_key]) == 2:
To cover the case where the previous entry in here wasn't repeated but now it is?
You are certainly right, but using >1
makes it clearer imho, even if after we added a second entry we would just have to set in theory repeated
to True only for the entry we just added.
[ ] L444 This is already the default, do we need to set it again?
This is not really needed, but setting it here seems safer
[x] L448-453 An explanation of which types we're keeping track of and why would be helpful here. I found it confusing to figure out on my own. I think this indentation is clearer in this case FWIW ;)
if diff_type not in [_DIFF_TYPES.NEW,
_DIFF_TYPES.OMITTED,
_DIFF_TYPES.REINSTATED,
_DIFF_TYPES.RESCAN] or not cut_diff.repeated:
if diff_type in self._counts:
...
I think my indentation was actually clearer, as it highlights if in some values or repeated
, where with your indentation the or not repeated
is barely visible ;) I added a constant so now everything fits on a single line, which is better
defaultdict
cut_diff
rather than doing a full recompute? I may be missing some logic nuance here.
This code was removed in a later branch, Things could be optimised, but we are dealing at most with 10 entries in the list, so I'm not sure it is worth it at this stage [x] L473-474
# We might have empty names here. To avoid considering all entries
[ ] L477 Can this signal be triggered but the names are the same still? What's the edge case / QT quirk?
It depends on the piece of UI used to edit it I guess. Seems safer to double check here
[ ] L481-483 This is more pythonic:
try:
self._cut_diffs[old_shot_key].remove(cut_diff)
except ValueError:
raise RuntimeError("Can't retrieve shot %s in internal list" % old_shot_key)
I would argue that the current code is clearer. And are you sure this a ValueError
which should be caught and not a KeyError
?
[kp] personal preference I think. You're right, this would raise a KeyError
if the old_shot_key
was missing and a ValueError
if cut_diff
wasn't in the list for some reason.
cut_diff.siblings
so when it's changed, we do this automatically in it's own slot? This slot is about name changing so this feels like it's a little out of scope. Not a big deal, just wondering.
This was kind of an esoteric thought (hallucination?).[ ] L492-497
cdiff
variable is unused cdiff = self.add_cut_diff(
sg_shot["code"],
sg_shot=sg_shot,
edit=None,
sg_cut_item=sg_cut_item,
)
sg_shot["code"]
be old_shot_key
?_Should be the same, and it seems better to use the the "name" from the sg_shot
_
[ ] L506 Add comment:
# Now we need to recreate the info for this cut_diff from scratch with the new name
# so we reset the values.
Sorry, I fail to understand the comment with the line number ?
[kp] This was intended to be a higher-level comment separating the block of code above it from the many lines below it indicating what the general next step is. I got a little confused at first which is what prompted it but it's not critical by any means.
[ ] L523-524
This comment feels buried here, can we move it to the top of the if
statement:
# If there is only one entry, it could be an omitted shot (with no edit).
# In that case the shot is not omitted anymore.
if count == 1 and not self._cut_diffs[new_shot_key][0].edit:
This is part of things I changed in a later branch, not changing it here
[ ] L525
It may be clearer if the var name is something like omt_cdiff
?
This is part of things I changed in a later branch, not changing it here
[x] L546
Add comment for else
block
+ # new_shot_key not already in self._cut_diffs
[ ] L556-564
filters = [
["project", "is", self._sg_project],
[self._sg_shot_link_field_name, "is", self._sg_entity],
["code", "is", new_name]
]
existing_linked_shot = self._app.shotgun.find_one("Shot", filters, _SHOT_FIELDS)
order
key for this query so when we do this it's consistent for the app rather than relying on whatever order the database decides to return?The whole app assumes Shot names are unique and by default we should get the highest id shouldn't we ?
[ ] L570-574
Should we add an order
key for this query so when we do this it's consistent for the app rather than relying on whatever order the database decides to return?
The whole app assumes Shot names are unique and by default we should get the highest id shouldn't we ?
[kp] Perhaps. But we're being explicit in our code elsewhere so this felt more consistent.
[ ] L586
cut_diff
is unused. Do we need to have it as a parameter then?
In theory, yes it could be removed. But as further changes are coming from later branches, I'm a bit wary to change it here and now
[ ] L592 Again, wondering how the signal would be triggered if these were the same?
Not sure, but seems safer to double check here
defaultdict
[ ] L674 Use helpful variable names:
for shot_name, cut_diffs in self._cut_diffs.iteritems():
I think I changed it in a later branch
[x] L683-687
# We count these per shots
defaultdict
defaultdict
[ ] L810
"WARNING, the following edits couldn't be linked to any Shot :\n%s\n" % (
Leaving it as is, unless this is definitely broken English ?
tk-multi-importcut/python/cut_diff_widget.py
Some general comments that apply to this as a whole:
:param:
definitions :param:
definitions (widespread, no further notes about it):
:param:
definition[x] L213
Some parameters are ignored as we have the CutDiff instance as a member
[x] L226
# issued from it.
:param:
name [x] L349-350
Asynchronously request a thumbnail download for the linked SG Version.
Fall back on the linked SG Shot if the Version doesn't exist or doesn't
have a thumbnail.
tk-multi-importcut/python/search_widget.py
:param:
definition[x] L68
# edit widget
[ ] L101
Missing :param:
definition
As mention in the top comment, this should be replaced by the official search widget now available in TK fw, so not changing it here
tk-multi-importcut/python/create_entity_dialog.py
entities page. A user can enter an entity name with a description. On submit
the entity is created and the stacked widget moves to the next page.
tk-multi-importcut/python/cut_diffs_view.py
Some general comments that apply to this as a whole:
:param:
definitions :param:
definitions:param:
definitions (widespread, no more notes about it) Called when the user clicks on the top view selectors in the Cut summary
tk-multi-importcut/python/cuts_view.py
Display only Cuts whose name matches the given text.
Display all of them if text is empty.
Called when a cut card is selected.
Ensure only one is selected at a time.
[ ] L192
Should this be initially defined in the __init__()
?
This is the only place where it is actually called
for sort in _SORT_METHODS:
tk-multi-importcut/python/downloader.py
:param sg_attachment: Either a URL or an Attachment dictionary
# todo: test this code. But how do we create a situation
# where the thumbnail (is it always a thumbnail?) ia an
# Attachment and not a string path to an aws bucket?
A local install / VM won't ship to AWS. Though I think by default this will give you a local Shotgun url.
e
is unusedtk-multi-importcut/python/extended_thumbnail.py
:param:
definitions [x] L37
Set the text, color and if this thumbnail should have a strike-through
include blank line after description
:param:
definition:param:
definitions tk-multi-importcut/python/logger.py
The short_name strips the two first parts of the logger path, allowing
us to not print the <uuid>.<app name> part in logs.
:param:
definition
blank line between docstring description and :param:
definitions # Special case for tk-shotgun and tk-desktop:
# - tk-shotgun : messages are displayed in a pop up window after
# the app is closed, which is confusing
# - tk-desktop : messages are sent through a pipe to the desktop server
# and are logged in the tk-desktop log. We have our own
# logging file and sometimes tk-desktop will hang when
# writing data, so don't log anything
tk-desktop
but I do see logging working when launching from tk-desktop
so what is this doing exactly?tk-multi-importcut/python/processor.py
Some general comments that apply to this as a whole:
:param:
definitions Return the current Shotgun Project we are importing the Cut into.
This can be None if this app was started outside of a Project
context, and a Project has not been selected yet.
Return the name of the Entity that the Cut is linked to, if any
_Actually, this is the target for the Cut import, so this docstring would be slightly wrong_
Return the display name of Entity's type that the Cut is linked to, if any
_Actually, this is the target for the Cut import, so this docstring would be slightly wrong_
Return the current Shotgun Cut we are displaying Cut changes for
:returns: A Cut dictionary or None
.
at the end of the comment just to clarify it doesn't bleed in with the comment on the next line.tk-multi-importcut/python/settings_dialog.py
[x] L118
# Determine whether the Shot status fields are enabled
# and update the setting if the user turns them on/off w/the checkbox.
[ ] L141-142 SG Isn't the Status with the 0 index "" indicating that Shotgun will use the default defined for the field?
I think we need an explicit value, there is some code which validates the value is set to something
[x] L238
parentheses are unnecessary around e
self._logger.error(_CORRUPT_SETTINGS_MSG % e)
[ ] L389 It would be great if this was an autocompleter. There is no placeholder text so it's not obvious what the box expects.
Should we add a ticket for that ? Other improvements could be added ?
tk-multi-importcut/python/widgets/animated_stacked_widget.py
[x] L48
Thin wrapper around setCurrentIndex ensuring first_page_reached
and last_page_reached are emitted when needed
:param index: MISSING DEFINITION
:param:
definition[x] L76-77
# Older versions of Qt don't contain Q*Animation classes
# so just change the page
=
[x] L101-102
# Older versions of Qt don't contain Q*Animation classes
# so just change the page
[x] L106-107
# Keep them around for garbage collection purposes
# Might not be needed, but who knows ...
[x] L127 Remove extra spaces
self.__anim_grp.finished.connect(lambda: this_page.hide())
tk-multi-importcut/python/widgets/drop_area.py
**kwargs
to be consistent:params:
definition**kwargs
to be consistent[x] L41-42
indent multi-line :param:
definition for readability
:param ext_list: list of extensions to restrict drop area to accept, these
should be included WITH a dot so, for example,
[".edl", ".mov"]
QEvent
parameters are typically named event
. For consistency, let's avoid evt
and e
since event
is the most common use across the app. :params:
definitionQEvent
parameters are typically named event
. (see above):params:
definitionQEvent
parameters are typically named event
. (see above):params:
definition[ ] L107
do we mean to have this catch-all except:
? Or safe to change to except Exception:
?
I think we want to catch anything which can be raised
contents
isn't pre-defined. Even though the expected results in the if/else
block above are enforced in dragEnter()
, it would be a good idea to preset this to None
so if the logic changes in dragEnter()
we won't get a "referenced before assignment" error.tk-multi-importcut/info.yml
Despite the massive amount of notes, this feels really well done. The polish here will put it in very good shape IMO.
There were some general style-related issues that came up that weren't necessarily addressed individually at every occurrence. Ideally these should be addressed app-wide. The main goal at this point is consistency. Whether indents are done one way or another (as long as it's reasonable and readable), isn't as important as being consistent throughout the app.
That said, there are also some discussions to have about various indenting situations, inline comments, variable naming, level of nitpicking we want to do for these, etc. Those will be left to the larger group discussions (but let's have them!).
:
(I know in more sophisticated countries, this is the correct way to do things).Ensuring docstrings exist for everything.
:params:
definition. This method does something.
:params foo: a string telling us what to do
This method does something.
After it does something it makes you coffee and will tell you a joke.
:params foo: a string telling us what to do
:params:
definitions for every method that accepts params(optional)
in the :params:
definition (eg. :params foo: (optional) foobar to make the wheels turn
.:params:
definitions should be indented to line up with the definition.:params foo: a string telling us what to do. If an empty string is passed in, then
we will just sit here twiddling our thumbs.
QEvent
passed in as parameter should be named event
. This is simply for consistency.<font>
tags. Would it be more efficient to use CSS for these? For arbitrary text elements, instead of using <font>
we could use <span>
? I'm not very familiar with how well QT handles this stuff. I know it's quirky. That's it. Code review complete! 🎉
Went through all comments. Didn't address some of them as some further changes are pending on other branches and I didn't want to cause un-necessary conflicts. Some answers:
<font>
tags are used when a single label holds a string with different styling (bold vs regular). We would need to have two widgets to do that through the style sheet.CutDiff
and CutDiffItem
?Okay I'm good for this to 🚢. I responded to some items in bold above. Thanks for all of the answers and extensive review.
CutDiff
and CutDiffItem
but will defer to other opinion on the matter to see if it's really worthwhile. Maybe create a ticket for this to track.Though a lot of this was smaller stuff, and the code was pretty clear to start, I think this is a huge improvement. Good to merge!
Lot of changes in there, goals were:
A couple of bugs were fixed and some UI improvements were added.
Some things could still be better, but let's check if we reached a good first step.
Please note that commits from Jun 23 and later were never reviewed so extra care must be taken for them.