Greatly cleanup PluginController::loadPluginInternal.

Review Request #121881 - Created Jan. 6, 2015 and submitted

Milian Wolff
This hopefully resolves some issues in understanding the flow of
this method. Some code could also be removed and the warning
messages better formatted.

Nicolai Hähnle
Milian Wolff
Review request changed

Status: Closed (submitted)

Sergey Kalinichev

Still see no reason why you didn't remove checkForDependencies? IMO it makes things more complicated. You can do the same by using just loadDependencies.
Or at least you could have added a doxygen comment for checkForDependencies, or renamed it to e.g. hasUnresolvedDependencies. Because currently it's not clear what it does, and what returns in each situation.

  1. Renaming it is a good idea indeed, and I'll also extend the comments.

    The reason it exists is that loadDependencies does just that, i.e. it loads all dependencies. But assume the following is the case:

    Plugin A depends on B, C, D.

    Assume D does not exist (not installed) or is explicitly disabled by the user

    Now, if we'd just use loadDependencies, then B and C would be loaded and then D fails, so A also fails. This is not good, we don't want to load B nor C b/c we know D is going to fail and thus also A. This is what the checkForDependencies step does. Does this make things clearer?

  2. Yes, now I finally get it. Also I think it's worth to be mentioned in the comments then.

  3. did it now, thanks for your feedback Sergey!