Created attachment 60776 [details] [review]
Proposed patch

Just one comment: in startup_list_disable, shouldn't it break on the "if (g_str_has_prefix (client->desktop_file, system_dirs[i]))" blocks?

Created attachment 60782 [details] [review]
Updated patch

It's a bit better with the break, indeed :-)

This looks ok to me, please commit and close the bug

The patch leaves this line in it:
      path = g_build_filename (PREFIX, "share", "autostart", basename, NULL);
(and the corresponding free)

That looks broken.  It doesn't seem to serve any purpose and you reuse the path
variable, so the free at the end will make bad things happen.

I'd just remove the above line and the free.

Thanks for catching this, Ray.
I committed the patch with your suggestion.

Thanks

Actually, there seem to be a few other problems.

You do get the basename of the desktop file, but you don't ever use it.

So for instance, you have

+           if (g_file_test (path, G_FILE_TEST_EXISTS))

but in this case path isn't a path to a desktop file, but instead a path to an autostart directory.  I don't think that's what you want.

Also, I'm not entirely sure I understand the logic.  Is it something like, "If the program is currently disabled by a desktop file in the user's home directory, and that desktop file shadows one in a system directory, just delete the desktop file and use the system one instead".

If that's the logic, what if the desktop file in the system directory is also disabled?  Is the idea that users shouldn't be able to enable desktop files if the sysadmin has disabled them?

(I'm talking about the startup_list_enable function)

If the desktop file in the system directory is disabled, it would stay as is, since we can't modify it from the user's account.

About the idea of enabling files that are disabled in the system directory, that is not implemented right now, so the user can enable/disable what he/she wants.

(In reply to comment #7)
> Actually, there seem to be a few other problems.
> 
> You do get the basename of the desktop file, but you don't ever use it.
> 
> So for instance, you have
> 
> +           if (g_file_test (path, G_FILE_TEST_EXISTS))
> 
> but in this case path isn't a path to a desktop file, but instead a path to an
> autostart directory.  I don't think that's what you want.

Wow. Sorry for sucking so much in this patch... Fixed.

> Also, I'm not entirely sure I understand the logic.  Is it something like, "If
> the program is currently disabled by a desktop file in the user's home
> directory, and that desktop file shadows one in a system directory, just delete
> the desktop file and use the system one instead".
> 
> If that's the logic, what if the desktop file in the system directory is also
> disabled?  Is the idea that users shouldn't be able to enable desktop files if
> the sysadmin has disabled them?

Well, it doesn't change the behavior that we had. It just makes it more complete. I agree that it's not perfect, though.