Hi Brock, Brock Pytlik wrote: > > inatallupdate.py > Nits: > line 348: think a new docstring is needed Right :)
> packagermanager.py > Line 1303: I'm not clear why this is always doing a full refresh, but > that is the safe, if slower, operation. We do not provide any checkbox to allow user control if this is full refresh or not, so safer is to perform full refresh. > Other than that, the API usage looks good to me. Great to hear. The new webrev, which cleanly applies to the gate and as an addition to the previous one have proper dialogs for error handling and was cleaned up for better pylint: http://cr.opensolaris.org/~migi/09_10_2008_packagemanager_api_v2/ I did smoke tests on most of the actions, installing/uninstalling/update all/refresh catalogs/add,remove,modify repositories/remove some of the BE and switch the default BE/pkg search and everything worked as expected. I am planning to integrate this Today. best Michal > Brock >> Comments inline. >> >>> inatallupdate.py: >>> 74-76: i don't understand why installupdate is creating its own api >>> object instead of using the one created in packagemanager.py. There >>> should really only be one api object per image in existence, doing >>> otherwise could cause "bad things" to happen. (At the very least, having >>> multiple api objects defeats all the locking which is in the api) >> To make sure user will always run the initial state of the api. The >> gui doesn't allow to call api from two places, since once dialogs are >> poped up other dialogs are not usable so this prevents to call api >> from two different places at once and it prevents locking. It is very >> simple to change this, so please let me know if you prefer to have one >> api object. > For November, let's stick with what you have now. After November, I'd > suggest changing it, it seems like the kind of thing that's easy to make > a mistake with, and difficult to debug. (As a side note, we recently > encountered this in our own code b/c right now client.py creates an > image, and then the API object creates an image, which caused some > confusion yesterday. As soon as we can, we're going to pull out the > image creation in client.py to prevent it from happening going forward.) >> >> [snip] >> >>> 1070: why not use the API info method? >> The method from the api doesn't give all necessary information, like >> list of the files/directories/packages dependent etc... > Ok, after November, let's figure out what info is needed that's not > currently being provided and we'll improve the API to handle it. > >> [snip] >>> 1794-1796: I'd guess, like with the update manager, you want to pass in >>> a callback as the fourth argument which will get called when the state >>> of cancelability for the api changes >> Not necessary. When someone is clicking the cancel button we are >> checking if the api is cancellable at this stage, so callback would do >> the same instead we could enable/disable buttons but this would just >> complicate things too much IMHO. >> > Hmm, so right now the user will see an active cancel button, but when > they click on it, it might not work or might then gray out? For > November, this is fine. As we go forward, I have questions that this is > the right interface for users. > > Brock >> I need to test more this bits and add better handlers for exceptions >> (proper dialogs) >> >> best >> Michal >> > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
