Comment 4 for bug 264750

Hi,

Thanks for the patch. I have a few comments.

First of all your changelog entry isn't very clear. You add two patches
and edit two more, but only mention two patches. Every change
should be documented clearly. Please explain for each what the
problem is you are fixing, and what the fix is.

As for the patches themselves,

-+ import gksu2
-+ gksu2.sudo('umit')
-+
++ try:
++ import gksu2
++ gksu2.sudo('umit')
++ except:
++ pass

it's not a good idea to have a bare "except:" clause, for instance CTRL-C
in that time will not do what it is supposed to. What is the problem
you are trying to catch, if it is one exception, e.g. ImportError then catch
it directly.

+-
+- ucontent.insert(uline, "sys.path.append('%s')\n" % modules)
 + modules = "/usr/share/umit"
++ ucontent.insert(uline, "sys.path = ['%s'] + sys.path\n" % modules)

I think that's the path conflict fix, is that correct? Inserting modules at the
start of the path is usually frowned upon, as it overrides other installed
copies, and restricts the user's flexibility. Is it necessary here? Also
"sys.path.insert(0, %s) " % modules might be a clearer way to write that.

Why the addition of writing to a file at the end?

These fixes look important, but I would have to understand them better before
I would be willing to upload. Links to where they are reported/fixed upstream
would be very valuable.

I'm un-subscribing the sponsors for now. Please re-subscribe them when ready.
I'll remain subscribed in case you have any questions.

Thanks,

James