Michal Pryc wrote: > Brock Pytlik wrote: >> Michal Pryc wrote: >>> Brock, >>> The webrev is at: >>> >>> http://cr.opensolaris.org/~migi/06_10_2008_proposed_changes_to_consume_new_api_v1/ >>> >>> > > Brock, > Thanks a lot for comments, new webrev, which cleanly can be applied on > the gate: > http://cr.opensolaris.org/~migi/07_10_2008_packagemanager_api_v2/ >
inatallupdate.py Nits: line 348: think a new docstring is needed packagermanager.py Line 1303: I'm not clear why this is always doing a full refresh, but that is the safe, if slower, operation. Other than that, the API usage looks good to me. 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
