Padraig O'Briain wrote: > I have created a new webrev > http://cr.opensolaris.org/~padraig/ips-5188-v4/ to address the points > Brock has raised. > > See inline for more detailed comments. > > On 12/03/08 09:53, jmr wrote: > >> Comments inline: >> >> JR >> >> Brock Pytlik wrote: >> >>> Padraig O'Briain wrote: >>> >>> >>>> On 12/01/08 19:28, Brock Pytlik wrote: >>>> [snip] >>>> >>>> >>>>> If the user changes the refresh period, to be shorter than the time >>>>> left to the next check under the previous policy, shouldn't the >>>>> timeout be changed or another call added to the idle list with a >>>>> shorter timeout? For example, if I set the refresh period to a day, >>>>> wait till one minute after the refresh is happened (so the next >>>>> refresh is set for 24 hours) then change the refresh period to be >>>>> every hour, I shouldn't have to wait 24 hours before the hourly >>>>> refresh period takes effect. >>>>> >>>>> >>>>> >>>>> >>>> This cannot happen. The shortest refresh period is 1 day and the >>>> maximum that we wait for is 1 day; see line 352. >>>> >>>> >>> This seems like code that's written to be broken in the future when >>> specs change. I see no reason that the shortest refresh period should >>> be a day going future. Personally, I might like to have it be an >>> hour, or 15 mins if I'm really excited about trying new bits. I also >>> happen to think the maximum waiting of a day is broken as well, but >>> I'm not interested in fighting that battle now. I do think that to >>> put code in that's explicitly dependent on never having a refresh >>> period shorter than 1 day begs to introduce bugs in the future. >>> > I have added some comments at line 83 to address your concern about > breakage if shorter refresh period is supported in the future. > > The comments are better than nothing. I just don't understand why we wouldn't do it right in the first place. Because I've now hit the put up or shut up point personally, I'll add this:
My first pass solution to this would be to add a self.refresh_number to the UMN class. When I added the next call to the timeout_add loop, I'd increment the self.refresh_number by 1, and pass it as an argument to the function. When the function is first executed, it checks to see whether self.reference_number matches the argument it was passed. If it does, it proceeds happily. If the argument is greater than self.reference_number, it fails an assert (because I can't see how this should ever happen), if the argument is less than self.reference_number, then it just returns False and exits. I'm sure there are details to be worked out with that implementation, a lock around self.refresh_number is probably necessary for example, but I don't see why those details would stand in the way of writing the code so that the maximum_wait isn't an issue. Also, having to manually sync MAXIMUM_WAIT and the other various intervals seems like a recipe for bugs as well. >> [snip] > I have called the GConf key "terminate_after_icon-activate". > The default behavior is to not terminate updatemanagernotifier after > the user activates the icon. > Good > Note that updatemanagernotifier does not have the concept of new > updates. It will just notify if changes are available, even if these > changes were available the last time it notified you. > > I have simplified this code as we are only interested in whether there > are any updates available. > That's fine for now. Supporting the ability to say, "stop telling me about these particular updates" is clearly an RFE. I'll file it. Brock > Padraig > > [snip] _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
