GNOME Bugzilla – Bug 572357
Use gtk.show_uri for help
Last modified: 2009-08-31 20:39:16 UTC
Patch replaces gnome.help_display_desktop with gtk.show_uri. Also checks for errors on help and prints a (translated) string. Bumps pygtk dependency to 2.13.0
Created attachment 129024 [details] [review] Use gtk.show_uri for opening help
Thomas: can you try also removing gnome.program_init() in main.py and see if there's any side-effect? Hopefully no and we can remove that.
Created attachment 129025 [details] [review] also remove gnome.program_init() and PKG_CHECK_MODULES for python-gnome/python-gnome-desktop Seems to work fine. I also removed the PKG_CHECK_MODULES for python-gnome/python-gnome-desktop. I assumed they were only for import gnome? New patch includes all changes.
Created attachment 129544 [details] [review] Updated patch This fixes a few issues with your patch. We can't commit because of string freeze, though.
What I love about code reviews is that is it a chance to learn and grow. So may I ask a few questions? The changes you made over my patch seems to be: 1) Making it optional to show the help button. But I am confused since this is only called from one place and there it is hardcoded to be true. What purpose does this serve? 2) Add a dialog to show if an error occurred when showing the help. When will gtk.show_uri() even fail? Yelp seems to handle errors it encounters itself so this will seems to mainly be if yelp is not working?. Is that case worth all the extra code? And if it is only for when yelp is broken/missing then perhaps the text could be more helpful? 3) An unrelated code style fixup and variable name change from 'flags' to 'type'. Seemingly unrelated to the patch in general?
(In reply to comment #5) > What I love about code reviews is that is it a chance to learn and grow. So may > I ask a few questions? Sure, I actually wanted to explain what I changed, but it was late ;-) > The changes you made over my patch seems to be: > 1) Making it optional to show the help button. But I am confused since this is > only called from one place and there it is hardcoded to be true. What purpose > does this serve? Actually, pessulus code can also be used from sabayon. This was done for the sabayon case. Except that, hrm, this is stupid. I'll change this again. > 2) Add a dialog to show if an error occurred when showing the help. When will > gtk.show_uri() even fail? Yelp seems to handle errors it encounters itself so > this will seems to mainly be if yelp is not working?. Is that case worth all > the extra code? And if it is only for when yelp is broken/missing then perhaps > the text could be more helpful? That's not that much code, and it's actually some generic error dialog code, so I think it's worth it. It can happen if yelp is not installed, I guess. > 3) An unrelated code style fixup and variable name change from 'flags' to > 'type'. Seemingly unrelated to the patch in general? Yep. I should commit this part separately.
Created attachment 129960 [details] [review] Updated patch, better :-)
I'm curious. What is gnome-python-desktop-2.0 dep for?
bug-buddy integration
Ah. Thought that was handled in gtk now. Sorry about the noise
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Hey vuntz, Kind of surprised to see you take full credit for the patch for adding an error handling dialog, but whatever. http://git.gnome.org/cgit/pessulus/tree/NEWS#n47 http://git.gnome.org/cgit/pessulus/commit/?id=8bff1c4a58eae9add62cf17b44dea859b8299565
For NEWS, I just take the information from the commit logs, and I guess I forgot to pass --author when committing this one. Just a simple mistake... (you can check that I usually pass --author for patches by looking at my modules)
fair enough, no problem then :)