On Tue, 2010-05-25 at 00:34 +0000, Matt Layman wrote: > Review: Needs Fixing > Jamie, > > I really like the spirit of this branch because this is a much needed fix, but there are number of things that prevent me from merging this into the trunk. My comments follow: > > general: > * Adding a dependency of dbus means that you need to update the docs/DEPENDENCIES file to include python-dbus. Ah sorry, will fix. > entertainerlib/client/client.py: > * I think this really introduces a dependency in the client that shouldn't be there. I would suggest that the proper way to enable the screensaver before the client closes down is to use the instance variable you created in the media player. This could be done by adding a __del__ method to the media player that calls enable_screensaver. This would localize your dependency to the one place that needs it. Yes, this is a much better way. > entertainerlib/client/media_player.py: > * Import order is wrong. dbus would go after clutter. Due to alphabetic importing? > * There are tab characters strewn about in your file which is a no-no based on our style guidelines. Has this code been tested? I made a little test file that used a mix of spaces and tabs, and it flat out failed because of indentation errors. That makes me highly suspect of the code that is here. Yes, testing and working fine. Not sure where tab characters came from, I'll check my editor settings. > * Class names are typically nouns. You've provided a class name that seems more like a method. As a matter of fact, there are many handle_* methods in Entertainer. Could you select a more appropriate name? The first thing that popped into my head was ScreenSaverInhibitor, but that's just a suggestion. > * On the subject of names, the instance name in the media player is not meaningful either. 'ss' tells me almost nothing if I know the context, and absolutely nothing if I don't know the context. Entertainer's code mostly tends towards verbosity at the cost of conciseness. If following my suggestion for a class name, `self.screen_saver_inhibitor` or `self.inhibitor` would be better. > * The class should be a new style class (i.e. derive from `object`). > * The class requires a docstring. > * The existing docstrings require full stops at the end of each sentence. > * Inconsistent use of single versus double quotes. Please stick with one or the other. Most of the code leans towards single quotes, but it's not universally true. But the code should at least have consistency in its code block. > * The wrapping behavior is inconsistent with Entertainer conventions. If a line of code is meant to wrap, it should only be indented 4 spaces from the previous line that it is continuing. If that source line has multiple physical lines of wrap, then each line after the original should line up with each other. OK. > * Why is dbus_interface only assigned for the first time during a call to disable_screensaver? That would lead to a problem if the code ever called enable_screensaver first. Why can't it be moved to init? It can and will be moved. > * Why is ss_cookie converting from string to an instance of Inhibit? In init, I think ss_cookie should be None. Agreed. > * Why does disable_screensaver need to be called in enable_screensaver if there is no cookie? I guess maybe that would be necessary if you had multiple instances of this class, but if you localized the instance to the media_player, then I think that logic becomes unnecessary. If no cookie is set, then it should be safe to assume that Entertainer has not disabled the screensaver, right? In that case, you could just return. Yes, the localization will remove the need for the disable call. > * No print statements in production code because a normal user would never see them. If you consider the messages important, please add a logger and log the statements. > * No general exception catching. If dbus is throwing an exception that was causing problems while testing, catch that exception and log it if necessary. > * Hmmm... I put myself in the user's shoes and wondered what my reaction would be if I saw "could not UnInhibit signal" in the logs. I would think that's a bug. How about something like "could not reenable the screensaver"? It seems a lot less scary to me (not to mention helpful for any would be human debugger). > > Hopefully you're well aware by now that I only take the time to give an exhaustive review because I care. If you have any questions or disagreements with me, please let me know. Yes and thanks for the review. Almost all of your points I should of caught myself and so I'm sorry they got through. I'll fix them up and note them down for next time. > Thanks, > Matt Regards, Jamie.