Review of GSoC 2017 Package Manager
Open, HighPublic

Description

Blender Version
Tested: 956d8e790c8a7f562d8d0f7f56d5e7226581e286

Usage issues:

  • The Add-ons panel has been removed from the UI, and has been replaced with the new Packages panel. However, the Packages panel is empty and doesn't show any of Blender's bundled add-ons from addons or addons-contrib. This means that previously existing functionality has been removed.
  • When using "blendermonkey.com/bpkg" as repository URL, refreshing gives a requests.exceptions.MissingSchema: Invalid URL 'blendermonkey.com/bpkg/repo.json': No schema supplied. However, the user interface accepted this URL as correct. Either don't accept package URLs without schema, or add https:// or http:// when one is missing. Since we're talking about downloading, installing, and running software on users' machines, I would strongly recommend only accepting HTTPS URLs.
  • There is no way to edit a repository URL.
  • Ending a repository URL in a slash (like "http://localhost:8000/") causes a double slash in the final URL, as evident from the message "Could not parse repository downloaded from 'http://localhost:8000//repo.json'. Are you sure this is the correct URL?"
  • When added, a repository is unchecked. However, its repo.json is still downloaded. When pressing F8 to reload the code, all are checked. When unchecking one and saving user prefs, F8 still re-checks all repositories. It's thus unclear what the checkbox in front of the URL means.
  • Repositories should be shown in an easier to identify way. Right now, only the first bit of the URL is shown, which doesn't identify enough. The name of the repository can be obtained from its repo.json file.
  • After pressing F8 to reload the code, the partial repository names are shown instead of their partial URLs. However, this is too late, and should be done as soon as the name is known.
  • SSL certificate errors aren't shown as such, but are shown as generic connection errors. This is a security issue.
  • Repositories that fail to refresh aren't disabled, so every time a refresh is done an error is given. The broken state of the repository should be reflected in the UI.
  • No packages are shown, even when refreshing seemed succesful (i.e. didn't raise any error):
    A full restart of Blender is needed to show the packages.
  • It would be nice if there was an "Enabled" choice next to "Available", "Installed", and "Updates". This would actually be nice as the default filter.
  • Choosing "Enabled" as "category" shows an empty list of add-ons. However, choosing "All" shows that some add-ons are actually enabled.
  • Uninstalling a built-in add-on gives the error "Could not find file owned by package: '/home/sybren/.config/blender/2.79/scripts/addons/mesh_relax.py'. Refusing to uninstall." The path is wrong for the add-on (it's not installed there, it's inside Blender's addons dir itself). Furthermore, such a path check could have done before, and the button "Uninstall" disabled (with some indication as to why it was disabled).
  • Clicking "Preferences" gives an error:
  • Searching for "cloud" shows an empty result. However, the repo.json I tested with does contain the Blender Cloud add-on. Refreshing doesn't help.
  • Using this repo.json caused multiple repositories to appear:
    These could also be the repositories I deleted earlier, which of course should stay deleted. On further inspection, their JSON files indeed still exist in ~/.config/blender/2.79/config/repositories.

I haven't been able to test the functionality further, due to these problems.

Code issues:

bl_operators/package.py

  • File should be formatted according to PEP-8.
  • Remove # {{{ and # }}} markers.
  • 25-26: the configuration of mp_context needs comments to explain why this is necessary.
  • 28-29: remove unused code, rather than commenting it out.
  • 31: _main_has_run isn't used anywhere.
  • 42: "see bpkg_manager.subproc" -- that module doesn't exist, so the comment should be updated.
  • 77: Returning 'PASS_THROUGH' when the ESC key was handled may not be the best idea, since it'll allow other receivers to receive the key too. It should be clear from the UI that pressing ESC will cancel this action, and it shouldn't cancel anything else at the same time.
  • 107 + 126: maybe put those 10s in a well-named constant and use it in both places? If they are not conceptually the same timeout, put them into two well-named constants instead.
  • 197+270: This constructs the entire dict of all packages, only to find a single package. This seems rather inefficient, but if it's the only way to do this, this should be documented in a comment.
  • 308: can be simplified to if not self.repositories:
  • 320: this should be done in an overload of def _finish():, since it also should be run on a normal finish of the operator.
  • 337: code no longer in use should be removed. Otherwise, add a comment why the commented-out code is important enough to stay.
  • 397: shorten to if not item.name:, which also works when item.name is not a string (but None).
  • 438: the logic to find the local JSON file for a repository shouldn't be in this operator, it should be in its own function in a suitable module.
  • 439: remove superfluous parentheses.
  • 509: apparently it's possible to have multiple versions of the same add-on enabled at the same time. This MUST be documented well to the user, including the reasoning why this is possible at all. It seems very counter-intuitive to allow this, so maybe it shouldn't even be allowed.
  • 552: the classes tuple is const constructed outside the else block, but refers to classes defined inside that else block. This will cause an error when importing bpy raises an ImportError.

bpkg/__init__.py

  • The file is not formatted according to PEP-8.
  • 27+34+53: logger is created but not used.
  • 32: "refresh" parameter is undocumented.
  • 37: unclear why this is still a TODO, since you already use addon_utils.modules().
  • 64: here an OrderedDict() is returned, whereas normally a generic dict is returned. Furthermore, when a single package has no name, it breaks the entire function.
  • 84: the function has "list" in the name, but it returns a dict. Furthermore, it doesn't declare that it returns a dict, nor does it document the structure of that dict. The only explanation of what it does is hidden in a private function.
  • 90: Use the logging module. If you really want to print(), make sure what is printed is suited for, and understandable by, end users.

bpkg/actions.py

  • The file is not formatted according to PEP-8.
  • 10: Document when infinity is returned.
  • 30: Security issue: never use a path from a 3rd party source without sanitation. You don't know what people put into their repo.json files.
  • 37: Remove commented code, or reinstate the try/except block.
  • 45: No need to add from err to all the re-throws.
  • 53: The HTTP 1.1 specification denotes this fields as an integer, so don't parse it as a float.
  • 69: Here a potential division by infinity happens, which results in 0 to be reported, not infinity.
  • 92: Split up ZIP and non-ZIP handling into different functions.
  • 127: Remove superfluous parentheses.

bpkg/actions.py

  • Remove commented-out code.
  • Document the module in a docstring, not in a comment.
  • Document pkg_errors

bpkg/messages.py

  • The file is not formatted according to PEP-8.
  • 44: The SubprocWarning class is never used, so remove it.
  • 52: The SubprocError class isn't used as superclass for all xxxError messages.

bpkg/subproc.py

  • The file is not formatted according to PEP-8.
  • 45+51: The same list is constructed twice. Do that once and reuse it.
  • 48: The function doesn't return any value in other cases, so explicitly returning None here is confusing.
  • 54: Here an exception object is sent as "message", which is inconsistent with other use (on line 47). It also violates the type declaration given in messages.py. Handle the exception properly in the subprocess itself, and convert it to a meaningful message to send to Blender. This also applies to the other messages that send an exception object.

bpkg/types.py

  • The file is not formatted according to PEP-8.
  • Document why explicit conversion from/to dict is necessary at all.
  • In the from-dict conversion, errors should be reported much more explicitly. Instead of just catching a KeyError at the caller level, report exactly which key is missing in which repository file, which package, etc.
  • 21-36: Use literals when possible, so instead of str() use '', instead of list() use [], etc.
  • 40+46: Document what the property types are when they contain a non-None value.
  • 109 and similar: These properties can also return None, so it's better to declare them returning typing.Optional[str] etc.
  • 114 and similar lines: Don't use type(x) to do type checking, use isinstance() instead.
  • 115 and similar: Use %r in a format string when the type of the object is relevant.

bpkg/utils.py

  • The file is not formatted according to PEP-8.
  • 6: Just set ext='' as default value, instead of setting it to None and then replacing it with ''.
  • 18: This function doesn't seem to do anything.
  • 30: do not use pathlib.Path for anything except filesystem paths. This code will actually add an OS-specific path separator into a URL, even though URLs always use forward slashes.
  • 33: Unclear what this function does. There is no documentation. It's also unclear why repository loading code is chucked away in the "utils" module.
  • 64 + 73-76: Don't create instance attributes outside of __init__(). Just initialise it to a reasonable nil value and check for that, rather than using attribute existence.
  • 64: use self.path.with_name(self.path.name + '~') instead of converting to string and back to Path.
  • 93: this function already exists earlier in the file.

Details

Type
Bug

Related Objects

Mentioned Here
rBc24bb59a44b1: Handle special categories
rB9cec3f721299: Use EnumProperty for repository list
rB6debd2134af5: remove print statement
rBc9ec8ba88fb8: Add explanation for multiprocessing context
rBbfd4b7370736: Remove fold markers
rB480060696d92: Remove old abort method
rB684d870dc46b: Replace hardcoded 10s with constant
rB94c48c8f9238: Simplify package list refreshing
rBf226dd3030c2: simplify if
rB3907c90c5387: Remove commented code
rB08ce79dd85ac: Shorten if, remove commented code
rB0415d0461724: Move enable/disable into Package class
rB233d8f2fafd7: Remove old loggers, document refresh parameter
rB6058b5cbaaa6: Clarify TODO note
rBda7efd96f79b: Package name is not allowed to be None, no need to check for it
rB6be55223dcf4: Improve documentation and remove extraneous function
rB0c4fd02c27aa: pep8-ify bpkg/__init__.py
rB705695bf4d98: pep8-ify bl_operators/package.py
rBb9338dde5ac0: Improve docstring
rB6d382129f999: Make sure classes tuple is only set when it should be
rB16da5d84c0c4: Wait until we've determined url is a real url to derive filenames from it
rBb62b62d51b10: Remove old commented code
rB90ea56b2fbb8: Break exception chain
rB6c3786713e4b: Split code for installing zipped and unzipped packages into separate functions
rB01a4ea98c5b5: Remove unneccesary parens
rBce0396c8782f: pep8-ify bpkg/actions.py
rB688cb2d6e01b: Clean up bpkg/display.py
rBa2301ec26057: Remove unused message, make sure all errors inherit from SubprocError
rBc2ed14532265: pep8-ify bpkg/messages.py
rBb259a8597ce9: Build list once
rB7fbb720265cf: Remove unneccesary None
rB2be8b4de2a93: Write own error message
rB1ab4d5fec48e: Document reason for dict conversion
rB03a4bd6132e2: More precise error messages
rBe881a0e2a46a: Document non-none types
rB8e815f4ce25b: Use typing.Optional
rB280b80349a1a: pep8-ify bpkg/subproc.py
rB0f17f1937f86: Use isinstance
rB74a19feba88e: pep8-ify bpkg/types.py
rB0a5353e853aa: documentation
rBb35e5240f60f: fix utils
rB956d8e790c8a: Revert accidentally committed partial change

Wow, thanks for a such a detailed review :)
Some responses inline:

bl_operators/package.py

File should be formatted according to PEP-8.

705695bf4d98c3b4ca86d4a95da48d9f7d70a8e4

Remove # {{{ and # }}} markers.

bfd4b737073616d682c6f6ecc82b5211708edb38

25-26: the configuration of mp_context needs comments to explain why this is necessary.

c9ec8ba88fb87f94a512b4aa820796758146cf8c

28-29: remove unused code, rather than commenting it out.
31: _main_has_run isn't used anywhere.
42: "see bpkg_manager.subproc" -- that module doesn't exist, so the comment should be updated.

77: Returning 'PASS_THROUGH' when the ESC key was handled may not be the best idea, since it'll allow other receivers to receive the key too. It should be clear from the UI that pressing ESC will cancel this action, and it shouldn't cancel anything else at the same time.

480060696d92b52801879201925e7c957331989f

107 + 126: maybe put those 10s in a well-named constant and use it in both places? If they are not conceptually the same timeout, put them into two well-named constants instead.

684d870dc46bdd8fb77a616483a36aff255f50a1

197+270: This constructs the entire dict of all packages, only to find a single package. This seems rather inefficient, but if it's the only way to do this, this should be documented in a comment.

94c48c8f9238901589e801f5bf4b8c28fc0f6049

308: can be simplified to if not self.repositories:

f226dd3030c2598973a9d60446fc6a072786a7d9

320: this should be done in an overload of def _finish():, since it also should be run on a normal finish of the operator.

bfd4b737073616d682c6f6ecc82b5211708edb38

337: code no longer in use should be removed. Otherwise, add a comment why the commented-out code is important enough to stay.

3907c90c53872b985898562f6287477c15c290ac

397: shorten to if not item.name:, which also works when item.name is not a string (but None).

08ce79dd85ace357c7e60a018cd9c7f3c7b7b7e5

438: the logic to find the local JSON file for a repository shouldn't be in this operator, it should be in its own function in a suitable module.
439: remove superfluous parentheses.

9cec3f7212999bfb8a6d2a5f601f5d894941802e

509: apparently it's possible to have multiple versions of the same add-on enabled at the same time. This MUST be documented well to the user, including the reasoning why this is possible at all. It seems very counter-intuitive to allow this, so maybe it shouldn't even be allowed.

0415d04617249feb21c2d310811e9c9c6d881709

552: the classes tuple is const constructed outside the else block, but refers to classes defined inside that else block. This will cause an error when importing bpy raises an ImportError.

6d382129f9996e539cc0fd7061cd97798b06d489

list_packages() only re-constructs the list when tag_reindex is True, otherwise it uses a cached version. I've now simplified this, hopefully making the code clearer too.

bpkg/__init__.py

The file is not formatted according to PEP-8.

0c4fd02c27aaa2a982ecbb991d3cd66a54330711

27+34+53: logger is created but not used.
32: "refresh" parameter is undocumented.

233d8f2fafd7888e01c6f148836a02315aea7b3e

37: unclear why this is still a TODO, since you already use addon_utils.modules().

6058b5cbaaa628a2cba2253382d7a4e1d5cceded

64: here an OrderedDict() is returned, whereas normally a generic dict is returned. Furthermore, when a single package has no name, it breaks the entire function.

da7efd96f79bf1cdd85931be79ef6437ca1749ad

84: the function has "list" in the name, but it returns a dict. Furthermore, it doesn't declare that it returns a dict, nor does it document the structure of that dict. The only explanation of what it does is hidden in a private function.

6be55223dcf4584ffe09240a282f27f6d8c6c42b

90: Use the logging module. If you really want to print(), make sure what is printed is suited for, and understandable by, end users.

6debd2134af5d8ec60233507334ccfab922d74e2

bpkg/actions.py

The file is not formatted according to PEP-8.

ce0396c8782

10: Document when infinity is returned.

b9338dde5ac071ff9d1243d2a3178ba8b53e575b

30: Security issue: never use a path from a 3rd party source without sanitation. You don't know what people put into their repo.json files.

If it works as a real URL with a server at the other end, is that enough to trust it? The existing code wouldn't actually write to the derived path if the URL didn't function as a URL, but I've moved the URL parsing after it's been tested to make this clearer.

16da5d84c0c4d5d630c4d42085f108ed7e0a40f8

37: Remove commented code, or reinstate the try/except block.

b62b62d51b10c1e52c2be2dcdae657638d7e5be6

45: No need to add from err to all the re-throws.

90ea56b2fbb

53: The HTTP 1.1 specification denotes this fields as an integer, so don't parse it as a float.

69: Here a potential division by infinity happens, which results in 0 to be reported, not infinity.

92: Split up ZIP and non-ZIP handling into different functions.

6c3786713e4

127: Remove superfluous parentheses.

01a4ea98c5b

bpkg/display.py

Remove commented-out code.
Document the module in a docstring, not in a comment.
Document pkg_errors

688cb2d6e01

bpkg/messages.py

The file is not formatted according to PEP-8.

c2ed1453226

The SubprocWarning class is never used, so remove it.
The SubprocError class isn't used as superclass for all xxxError messages.

a2301ec2605

bpkg/subproc.py

The file is not formatted according to PEP-8.

280b80349a1

45+51: The same list is constructed twice. Do that once and reuse it.

b259a8597ce

48: The function doesn't return any value in other cases, so explicitly returning None here is confusing.

7fbb720265c

54: Here an exception object is sent as "message", which is inconsistent with other use (on line 47). It also violates the type declaration given in messages.py. Handle the exception properly in the subprocess itself, and convert it to a meaningful message to send to Blender. This also applies to the other messages that send an exception object.

2be8b4de2a9

For internally-generated exceptions, I know the message is suitable for users, and I don't think there's anything useful I can add to it here. Should I just convert it to a string?

bpkg/types.py

The file is not formatted according to PEP-8.

74a19feba88

Document why explicit conversion from/to dict is necessary at all.

1ab4d5fec48

In the from-dict conversion, errors should be reported much more explicitly. Instead of just catching a KeyError at the caller level, report exactly which key is missing in which repository file, which package, etc.

03a4bd6132e

21-36: Use literals when possible, so instead of str() use '', instead of list() use [], etc.

May I ask if there's a particular reason why? My thinking was I'm really trying to declare the type (like one would do in a strongly typed language like C), rather than explicitly initialize to a value such as empty string. It's just semantic, but it makes sense to me. I'd like to hear another point of view of course.

40+46: Document what the property types are when they contain a non-None value.

e881a0e2a46

109 and similar: These properties can also return None, so it's better to declare them returning typing.Optional[str] etc.
115 and similar: Use %r in a format string when the type of the object is relevant.

8e815f4ce25

114 and similar lines: Don't use type(x) to do type checking, use isinstance() instead.

0f17f1937f8

bpkg/utils.py

6: Just set ext='' as default value, instead of setting it to None and then replacing it with ''.

I've read that it's a good practice to do this when you don't wish to have the value be "static" to the function. (e.g. http://docs.python-guide.org/en/latest/writing/gotchas/#default-args)
On the other hand, I guess it shouldn't have any effect in this case..

18: This function doesn't seem to do anything.
30: do not use pathlib.Path for anything except filesystem paths. This code will actually add an OS-specific path separator into a URL, even though URLs always use forward slashes.
64 + 73-76: Don't create instance attributes outside of init(). Just initialise it to a reasonable nil value and check for that, rather than using attribute existence.
64: use self.path.with_name(self.path.name + '~') instead of converting to string and back to Path.
93: this function already exists earlier in the file.
The file is not formatted according to PEP-8.

b35e5240f60

33: Unclear what this function does. There is no documentation. It's also unclear why repository loading code is chucked away in the "utils" module.

0a5353e853a

utils is not really the right place, but as it's reasonable generic (just reads multiple repositories), and this was the handiest "bpy free zone" to keep it in so subprocesses can use it. To actually make things proper, there's more than a little reshuffling I'd like to do now that I have a better grasp of the requirements of multiprocessing.

320: this should be done in an overload of def _finish():, since it also should be run on a normal finish of the operator.

bfd4b737073616d682c6f6ecc82b5211708edb38

That commit has nothing to do with that remark.

438: the logic to find the local JSON file for a repository shouldn't be in this operator, it should be in its own function in a suitable module.
439: remove superfluous parentheses.

9cec3f7212999bfb8a6d2a5f601f5d894941802e

That commit doesn't remove the superfluous parentheses, and it still keeps the logic to go from repository name to filename in the same place.

53: The HTTP 1.1 specification denotes this fields as an integer, so don't parse it as a float.
69: Here a potential division by infinity happens, which results in 0 to be reported, not infinity.

These issues aren't dealt with yet.

For internally-generated exceptions, I know the message is suitable for users, and I don't think there's anything useful I can add to it here. Should I just convert it to a string?

Yes, that sounds reasonable.

May I ask if there's a particular reason why? My thinking was I'm really trying to declare the type (like one would do in a strongly typed language like C), rather than explicitly initialize to a value such as empty string. It's just semantic, but it makes sense to me. I'd like to hear another point of view of course.

Strictly speaking, you're not declaring type, you're calling a type. If you want to do real type declaration, wait until we move to Python 3.6 and use variable annotations ;-)
To me it's easier to recognise and visually differentiate between '' and [] than str() and list().

I've read that it's a good practice to do this when you don't wish to have the value be "static" to the function. (e.g. http://docs.python-guide.org/en/latest/writing/gotchas/#default-args)
On the other hand, I guess it shouldn't have any effect in this case..

This is only appliccable when you have mutable default values.

There are more issues than I listed here; they are placed as comments/concerns in their respective commits.

Re-tested for final evaluation using c24bb59a44b1a2b3601a13635db8548100948b7c.

New usage issues

These are actually new (i.e. didn't exist in my previous review) or are checks that I didn't do then, but are still relevant for the package manager.

  • The "Edit Repositories" button shows a popup that doesn't have any close/cancel buttons. Clicking outside the popup closes it, but doesn't work as a "cancel". Changes made in the popup are not rolled back, but also not applied properly.
  • When an invalid repository is added (say http://localhost:8000/ while nothing is running there), the "Edit Repositories" popup still shows a green checkmark.
  • When a repository removes packages from their repo.json, they aren't removed in Blender's local copy. Only the new packages are added.
  • When a repository changes its name, after refreshing it's added as a new repository. Now there are two repositories, one with the old name and one with the new one. This is due to the repository JSON being stored by the repo's name rather than the URL.
  • It's possible to have two repositories with the same URL.
  • A package that has a different name in repo.json and in its own bl_info dict will break the package manager (PM). After installation the PM will show both, one not installed (the name in repo.json) and one installed (the name in bl_info dict).
  • It is impossible to uninstall a package. I've installed a test add-on (i.e. one that is not already bundled with Blender) via the package manager, and its "uninstall" button remains disabled. This is a blocking issue.

Previously mentioned usage issues

The Add-ons panel has been removed from the UI, and has been replaced with the new Packages panel. However, the Packages panel is empty and doesn't show any of Blender's bundled add-ons from addons or addons-contrib. This means that previously existing functionality has been removed.

This is solved.

When using "blendermonkey.com/bpkg" as repository URL, refreshing gives a requests.exceptions.MissingSchema: Invalid URL 'blendermonkey.com/bpkg/repo.json': No schema supplied. However, the user interface accepted this URL as correct. Either don't accept package URLs without schema, or add https:// or http:// when one is missing. Since we're talking about downloading, installing, and running software on users' machines, I would strongly recommend only accepting HTTPS URLs.

This issue is still there.

There is no way to edit a repository URL.

Solved

Ending a repository URL in a slash (like "http://localhost:8000/") causes a double slash in the final URL

Solved

When added, a repository is unchecked.

This issue is still here, albeit in a different form. Freshly added repositories seem to be disabled, until you press F8.

Repositories should be shown in an easier to identify way.

Solved

After pressing F8 to reload the code, the partial repository names are shown instead of their partial URLs. However, this is too late, and should be done as soon as the name is known.

This issue is still there, albeit in a slightly different form. When a URL is valid, but then is changed to another address that is invalid, the old name is still shown.

SSL certificate errors aren't shown as such, but are shown as generic connection errors. This is a security issue.

This issue is still there.

Repositories that fail to refresh aren't disabled, so every time a refresh is done an error is given. The broken state of the repository should be reflected in the UI.

This issue is still there, albeit in a different form. When an URL is given that's in some way invalid (i.e. hostname doesn't exist), this is checked when OK is pressed and the popup closes. After the error message pops up, you can click Edit Repositories, and then the just-added repository has been deleted. This makes it impossible to go back in and change your mistake (or keep a repository that's invalid but will become live soon, or add a repository while you're offline, or add a repository while on a dodgy internet connection).

No packages are shown, even when refreshing seemed succesful (i.e. didn't raise any error):

Solved

It would be nice if there was an "Enabled" choice next to "Available", "Installed", and "Updates". This would actually be nice as the default filter.

This still would be nice, although now the category "Enabled" works too.

Choosing "Enabled" as "category" shows an empty list of add-ons. However, choosing "All" shows that some add-ons are actually enabled.

Solved

Uninstalling a built-in add-on gives the error [...]

Solved

Clicking "Preferences" gives an error

This is still an issue, albeit in a different form. The "Preferences" button is now always shown, but instead of giving an error it just doesn't do anything when clicked when an add-on doesn't have preferences.

Using this repo.json caused multiple repositories to appear

Solved

Code issues

This is just to re-cap the code issues I've described before that are still open.

Concerns raised

The concerns I raised in 0f17f1937f86c729fe, 6c3786713e4b are still there.

bpkg/actions.py

53: The HTTP 1.1 specification denotes this fields as an integer, so don't parse it as a float.

This is solved

69: Here a potential division by infinity happens, which results in 0 to be reported, not infinity.

This issue is still there, albeit in a different form. Division by zero of integers will cause an exception. Rather than catching this exception for every chunk that was downloaded (which is slow), just check whether content_length > 0. Furthermore, there is no "infinity" being reported any more, which is inconsistent with the docstring of the function.

For internally-generated exceptions, I know the message is suitable for users, and I don't think there's anything useful I can add to it here. Should I just convert it to a string?

Yes, that sounds reasonable.

This issue is still there. Don't pass exception objects as if they are strings.

bpkg/utils.py

6: Just set ext='' as default value, instead of setting it to None and then replacing it with ''.

This issue is still there.

bpkg/types.py

  • Don't import typing in the middle of a class definition.

Package list generator

  • The project is not a full Python project. It doesn't contain a setup.py describing the project and allowing for installation.
  • The README doesn't state which version of Python is required.
  • The BadAddon class doesn't have a docstring.
  • The unit tests don't use py.test as I've suggested multiple times. There is also no code coverage reporting, so it's hard to tell how well the code has been tested.

Ton & Campbell, my last comment reflects the current state of the package manager.

@Sybren A. Stüvel (sybren) raises a lot of good points that need to be address. In addition, here are a few extra things I noticed:

UX Issues:

  • I do not see a way to filter by repository. This could be more of an issue with having pre-installed addons cluttering the view, but when I select the fakerepo from the final report, I see a bunch of my installed addons in addition to the ones from the repo. We should be able to deal with manually installed addons not cluttering the list of packages from the repos.
  • If you're going to have debug prints in the console for install/uninstall, you might as well also list the package name
  • The Official/Community/Testing icons are missing

Code Issues:

  • I thought you stopped using multiprocessing? Is there a reason to deal with the headaches of multiprocessing instead of just using subprocess?
  • I am seeing quite a few TODO and HACK comments still. Is there a plan for addressing those?

Some initial responses:

The "Edit Repositories" button shows a popup that doesn't have any close/cancel buttons. Clicking outside the popup closes it, but doesn't work as a "cancel". Changes made in the popup are not rolled back, but also not applied properly.

I'm not super happy with using a popup here (from a UX point of view). I thought of having the button put the UI in a different state which shows the repositories UIlist and a back button (which reverts the state). This is analogous to going to a new "page". However, as far as I know, this isn't done anywhere else in Blender. Any suggestions here?
If this "page" UI is acceptable, I think it would be nice to handle addon preferences the same way.

I do not see a way to filter by repository. This could be more of an issue with having pre-installed addons cluttering the view, but when I select the fakerepo from the final report, I see a bunch of my installed addons in addition to the ones from the repo. We should be able to deal with manually installed addons not cluttering the list of packages from the repos.

There should be one, on the left side under "Repositories". Unless you mean a way to filter out local packages?

I thought you stopped using multiprocessing? Is there a reason to deal with the headaches of multiprocessing instead of just using subprocess?

I'm still open to dumping multiprocessing (especially now that I don't feel quite so pressed for time). I kept it on @Sybren A. Stüvel (sybren)'s suggestion to wrap import bpy in a try/catch, as that was a quicker solution.

Could this be made into a diff on master? - It's easier to follow & keep track of the current state if comments are inlined in source.

Also means we can more easily apply on master and test.


Since a lot of work went into the current review maybe hold off creating the diff until most of the issues are handled.

The "Edit Repositories" button shows a popup that doesn't have any close/cancel buttons. Clicking outside the popup closes it, but doesn't work as a "cancel". Changes made in the popup are not rolled back, but also not applied properly.

I'm not super happy with using a popup here (from a UX point of view). I thought of having the button put the UI in a different state which shows the repositories UIlist and a back button (which reverts the state). This is analogous to going to a new "page". However, as far as I know, this isn't done anywhere else in Blender. Any suggestions here?

A "back" button doesn't sound appealing to me at all; it's not a wizard nor a website. Why not just "Ok" and "Cancel" buttons? Just make sure it works well, even when someone doesn't press any of those buttons and just closes the user prefs window altogether.

I do not see a way to filter by repository. This could be more of an issue with having pre-installed addons cluttering the view, but when I select the fakerepo from the final report, I see a bunch of my installed addons in addition to the ones from the repo. We should be able to deal with manually installed addons not cluttering the list of packages from the repos.

There should be one, on the left side under "Repositories". Unless you mean a way to filter out local packages?

I mean a way to say "show only packages from repository X".

@Campbell Barton (campbellbarton) +1 on your entire comment.