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/ 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. > 199: I'm not certain, but I would've guessed that api.cancel should be > called here rather than api.reset. I am quite certain that reset should be called. I wanted to move api to the initial state. Of course this is just after evaluation so cancel would be the same as reset since cancel moves api to the previous state. For all actions which fails I will do reset, so if user clicks on install/update/remove button the user will start with the initial state of the api. > 219, 224, 319, 345: commented lines I removed all commented lines. > 290-295: as noted below, I hope to provide an api so that this is > unnecessary, but it looks fine for now great > 459-460: I think this can be simplified as plan = > api.describe().get_changes() changed > imageplan.py: > 85: please split this list into two separate variables changed > 522-538: this needs to be done here so you can present the user with > download sizes? Yes. After evaluation stage we are printing dialog which shows numbers, this webrev for progress was send before to pkg-discuss as fix for the bug: http://defect.opensolaris.org/bz/show_bug.cgi?id=3089 The original webrev (current one was a little bit changed): http://cr.opensolaris.org/~migi/22_09_progress_enhanc_3089_v1/ > progress.py: > please resync with the gate/v8 Done > packagemanager.py: > lines 353, 362: remove commented lines Done > 1064: some typos on this line I think changed > 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... > > 1194-1208: I hope to provide an interface to update_all which eliminates > the need for this, but for now this looks good, except that plan_install > returns True or False, not 1 or 0 (this might be one of the late changes > that went into v8) Changed all bits in the code to reflect this. > > 1311-1328: find_root no longer raises a ValueError, it can raise an > api_errors.ImageNotFoundException, please don't change the changes what > were made here in changeset be1ad23e7704 unless there are good reasons My mistake, I was syncing with the older revision. Currently all changes which were in the gate are also in this webrev. > 1730: I believe _get_version_installed has been removed from the code base The same as above. Shawn did change the packagemanager, but the webrev was against previous revision. > 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. 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
