Paths

Table of Contentst

Refactor: Improve USERPREF_PT_addons readability
Needs RevisionPublic
Actions

Authored by Oliver J. Post (Ole) on May 12 2022, 2:58 PM.

Details

Reviewers

Summary

Splits up the draw method of USERPREF_PT_addons in order to improve readability. Some parts which were previously
scattered, like filtering visible addons, are now combined into their own method.

No functional changes, only refactoring.

Submitting a change like this was discussed with Julian Eisel in blender.chat to try out if improving
readability in this way is preferable and if there is time to review.

It's a big differential because of the splitting into methods, so if there is no time to review changes like
this I would understand.

This is my first commit, so if I did something wrong please let me know.

Diff Detail

Repository

rB Blender

Build Status

Buildable 22093
Build 22093: arc lint + arc unit

Event Timeline

Oliver J. Post (Ole) requested review of this revision.May 12 2022, 2:58 PM

Oliver J. Post (Ole) created this revision.

Harbormaster completed remote builds in B22093: Diff 51434.May 12 2022, 2:58 PM

Oliver J. Post (Ole) edited the summary of this revision. (Show Details)May 12 2022, 2:59 PM

Isaac Gicheha (Gicheha) added a project: User Interface.May 12 2022, 5:17 PM

The refactor seems generally OK, please remove all minor changes that could potentially change functionality, pointed out a few but didn't do a comprehensive check on every line.

release/scripts/startup/bl_ui/space_userpref.py
1937	Function calls have a reasonable amount of overhead in Python, this is calling the search filter function, accessing and the search term and making it lower-case for every-addon, even when search is disabled. It would be better to keep `search = wm.addon_search.lower()` assignment here and pass it into `_searchterm_in_addon` - only when it's not an empty string.
1944	Avoid annotations here (they can't be validated).
1980	This can be a staticmethod as `self` is only used to access staticmethods.
2068	Keep as-is. The check is intentionally spesific to show that an ignored value is only ever `None` - as opposted to an empty string, list etc.
2072	Keep as-is.
2081	Leave as-is. This changes functionality & allows exceptions to prevent other add-ons from drawing.
2089	prefer full name `category_filter` it's not much longer and reads better.

This revision now requires changes to proceed.May 13 2022, 9:04 AM

Applying requested changes:

Moved searchterm assignment out of _searchterm_in_addon method.
Removed return type annotations.
Made _draw_addon_info_box into staticmethod.
Revert if not to is None for addon_preferences and draw.
Revert except Exception
Renamed categ_filter to category_filter

Added _match suffix for _filter_visible_addons variables to clarify and to prevent naming conflict

Also, I went trough all lines again to identify any possible changes that impact functionality. Just to confirm, this does not change functionality, correct?

OLD:

bool(info["doc_url"]) + bool(user_addon)

NEW:

any(info["doc_url"], user_addon)

This is only used by an if statement, so the old code having the possible value of 2 should not matter.

Harbormaster completed remote builds in B22108: Diff 51472.May 13 2022, 4:46 PM

There are still changes that aren't related to relocating functions.

Calculating multiple variables, then checking them resulting in less efficient checks.
Using all(..) and any(..) in if statements when simple and / or are sufficient, or instead of early exiting.

This patch has too many opinionated changes, these are small but not appropriate if you intend to relocate functions as it makes it difficult to know what you change and if there are some subtle bugs introduced.

Note that the use of static methods here is awkward and hints that we might be better off moving all this drawing out of the class into it's own module, as we have for keymap drawing, see: release/scripts/modules/rna_keymap_ui.py.

release/scripts/startup/bl_ui/space_userpref.py
1883–1885	Can be a static method.
1917–1919	can be a static method.
1955	can be a static method.

This revision now requires changes to proceed.May 14 2022, 4:15 AM

This patch has too many opinionated changes

I do see now that me introducing these small changes made it a lot harder to review the code and spot possible bugs and I should have just kept the exact code from before and just split it up into methods.

What would you suggest is the best way forward for me to make it as easy to review as possible?

Go trough the requested changes you mentioned and try to identify any more of these kind of opinionated changes
Re-do the refactor from scratch, making sure to only split the existing code into methods
Re-do the refactor from scratch, and split only the existing code into functions contained in a separate file as done in release/scripts/modules/rna_keymap_ui.py

Extracting into a submodule may be nice, but then pushes us to re-arrange the modules (perhaps make addon_utils a package so we can have addon_utils.ui for eg.). This could be OK but is a bit disruptive, other developers may want to have input on this or disagree with the change - so I'd prefer to hold off potentially controversial changes at the moment.

Suggest the following:

Re-do the refactor, only split out functions.
Use static-methods, accepting that it's awkward and we may move the UI into a separate module in the future.
Any other changes can be a separate patch.

Checking USERPREF_PT_addons I don't think it's necessary to split this into so many functions either. A single function to draw an add-ons UI seems reasonable, but splitting up the UI into separate functions doesn't seem necessary.

Revision Contents
Changeset List

Path

Size

release/

scripts/

startup/

bl_ui/

space_userpref.py

394 lines

Diff 51434

View Options

release/scripts/startup/bl_ui/space_userpref.py

Refactor: Improve USERPREF_PT_addons readabilityNeeds RevisionPublicActions