Created attachment 118998 [details] [review]
printer renaming

Comment on attachment 118998 [details] [review]
printer renaming

Review of attachment 118998 [details] [review]:
-----------------------------------------------------------------

Thank you for working on this,

I have few comments on the patch. I'll push it once you apply them.

::: src/cups-pk-helper-mechanism.c
@@ +912,5 @@
> +        gboolean      ret;
> +
> +        _cph_mechanism_emit_called (mechanism);
> +
> +        if (!_check_polkit_for_printer (mechanism, context, old_printer_name, NULL))

We need to check more policies here. See _check_polkit_for_printer_class() and _cph_mechanism_get_action_for_name() + add "printer-set-default", "printer-enable" and "job-edit" or "job-not-owned-edit" if there are jobs to move (and select appropriate one).

::: src/cups.c
@@ +2170,5 @@
> +
> +        num_jobs = cupsGetJobs (&jobs, old_printer_name, 0, CUPS_WHICHJOBS_ACTIVE);
> +        for (i = 0; i < num_jobs; i++) {
> +                if (jobs[i].state == IPP_JSTATE_PENDING
> +                    || jobs[i].state == IPP_JSTATE_PROCESSING) {

Move the logical OR to the end of the previous line please.

@@ +2209,5 @@
> +
> +        if (response != NULL) {
> +                if (ippGetStatusCode (response) <= IPP_OK_CONFLICT) {
> +                        attr = ippFindAttribute (response, "printer-error-policy", IPP_TAG_NAME);
> +                        if (attr)

Do all such checks against NULL value please.

@@ +2239,5 @@
> +                        }
> +                }
> +        }
> +
> +        ippDelete (response);

Include this in the "if (response != NULL)" block.

@@ +2260,5 @@
> +                                                       printer_info,
> +                                                       printer_location)) {
> +                for (i = 0; i < num_dests; i++) {
> +                        if (!cph_cups_is_class (cups, dests[i].name))
> +                                continue;

Negate the condition and place the following block into it please (you can join the 2 ifs to one then).

@@ +2300,5 @@
> +                        start_sheet = sheets[0];
> +                        end_sheet = sheets[1];
> +                }
> +        }
> +        cph_cups_printer_class_set_job_sheets (cups, new_printer_name, start_sheet, end_sheet);

You can move this to the inner if so we don't run it when not needed.

@@ +2304,5 @@
> +        cph_cups_printer_class_set_job_sheets (cups, new_printer_name, start_sheet, end_sheet);
> +        cph_cups_printer_set_enabled (cups, new_printer_name, !printer_paused);
> +        cph_cups_printer_class_set_shared (cups, new_printer_name, printer_shared);
> +        cph_cups_printer_class_set_users_allowed (cups, new_printer_name, users_allowed);
> +        cph_cups_printer_class_set_users_denied (cups, new_printer_name, users_denied);

Cast the "users_allowed" and "users_denied" to "(const char * const *)" so we don't get warnings about that.

@@ +2334,5 @@
> +                g_strfreev (sheets);
> +        if (users_allowed)
> +                g_strfreev (users_allowed);
> +        if (users_denied)
> +                g_strfreev (users_denied);

g_strfreev() and g_free() don't need the NULL checks they check themselves.

See the comments above please.

(In reply to Marek Kasik from comment #2)
> ::: src/cups-pk-helper-mechanism.c
> @@ +912,5 @@
> > +        gboolean      ret;
> > +
> > +        _cph_mechanism_emit_called (mechanism);
> > +
> > +        if (!_check_polkit_for_printer (mechanism, context, old_printer_name, NULL))
> 
> We need to check more policies here. See _check_polkit_for_printer_class()
> and _cph_mechanism_get_action_for_name() + add "printer-set-default",
> "printer-enable" and "job-edit" or "job-not-owned-edit" if there are jobs to
> move (and select appropriate one).

I've looked at this again, the _check_polkit_for_printer_class() will be enough here.

Created attachment 119369 [details] [review]
printer renaming

Fixed.

Comment on attachment 119369 [details] [review]
printer renaming

Review of attachment 119369 [details] [review]:
-----------------------------------------------------------------

I have 3 requests yet:

::: src/cups.c
@@ +2240,5 @@
> +                }
> +        }
> +
> +        if (response != NULL) {
> +                ippDelete (response);

I meant to move this to the above block which is also "if (response != NULL)". The ippDelete() actually checks the given pointer for NULL too but I prefer to have the free functions in the same block where we check the freed pointer for NULL if present.

@@ +2294,5 @@
> +                cph_cups_printer_set_default (cups, new_printer_name);
> +        cph_cups_printer_class_set_error_policy (cups, new_printer_name, error_policy);
> +        cph_cups_printer_class_set_op_policy (cups, new_printer_name, op_policy);
> +
> +        if (job_sheets) {

Check this against NULL please.

@@ +2322,5 @@
> +
> +
> +        cupsFreeDests (num_dests, dests);
> +
> +        if (ppd_link) {

Check this against NULL too.

Created attachment 119533 [details] [review]
printer renaming

Fixed.

Thank you for the patch Martin. I've pushed it into master.

Btw, I plan to do a new release of cups-pk-helper before next stable Gnome comes out (3.20). I will look at some other bugs and improvements we could get there.