[Webkit-unassigned] [Bug 151998] [GTK] Add an API to add a custom tab into the print dialog

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=151998

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #267000|review?                     |review-
              Flags|                            |

--- Comment #3 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 267000
  --> https://bugs.webkit.org/attachment.cgi?id=267000
Proposed patch based on GtkPrintOperation implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=267000&action=review

Sorry for the delay and thanks for working on this; I'm eager to add the API you need for Evolution. r- not because I'm opposed to the changes, but because I'm not sure this is the best API and I have many suggestions.

Please do propose this API on the mailing list for discussion. I'm not sure it's the best we can do:

 * Signals are OK, but not the easiest to work with.
 * Your API can only handle a single custom tab, whereas it would be good to allow any number of tabs.
 * I'm not thrilled that the tab label is a property, whereas the custom widget is returned via a signal; both are slightly clunky, and it's awkward that they're set separately. (It seems like the custom widget could be a property as well, rather than a signal?)
 * Perhaps the custom label property should be a GtkWidget, rather than a char* that we use to create a GtkLabel. I guess 90% of applications will want to use a plain label, but I don't see any real reason to restrict them from doing otherwise.

Consider this alternative API:

webkit_print_operation_add_custom_print_dialog_tab(const char* tabLabel, GtkContainer* tabContent, GCallback applyCallback)

Wouldn't that be simpler? That's just one function: we wouldn't need the two signals or the clunky tab label property; it's easy for the application to add multiple tabs, rather than artificially limiting applications to one custom tab; and it'd probably be a bit simpler to implement. Do you see any problems with this approach?

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:152
> +static GtkWidget* webkitPrintOperationCreateCustomWidget(WebKitPrintOperation *printOperation)

WebKitPrintOperation* printOperation

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:213
> +    g_object_class_install_property(gObjectClass,

You really do have to obey style checker for new properties, so indent four spaces:

g_object_class_install_property(
    gObjectClass,
    PROP_CUSTOM_TAB_LABEL,
    g_param_spec_string(
        "custom-tab-label",
        _("Custom tab label"),
        _("Label for the tab containing custom widgets."),
        nullptr,
        WEBKIT_PARAM_READWRITE));

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:269
> +    * Returns: (transfer none): A custom widget that gets embedded in

I'm really not sure if this is the right transfer annotation? The widget is going to be embedded into the widget tree, so it would be wrong for the caller to free it after this. But I think it's not typical to mark GtkWidget parameters with transfer full? It's important to get this right for bindings.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:339
> +                customTabLabel = _("Application");

I think this might be impossible to translate without context, so it would surely need a translator comment. But I would rather force the application to specify a label. If the property is unset, you could g_critical and return, or g_return_val_if_fail, rather than fallback to g_get_application_name, since I suspect that will rarely ever be the desired label, and "Application" will never be the desired label. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:343
> +

Nit of all nits, but I would not leave a blank line here. You're creating the label for no reason other than to add it to printDialog; those are closely-related enough to be on consecutive lines. IMO. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:562
> + * @label: (allow-none): the label to use, or %NULL to use the default label

If you follow my suggestion and get rid of the default label, this would no longer be (allow-none).

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:71
>      void (*_webkit_reserved1) (void);

Careful not to break ABI here. Since you're adding two virtual signals, you have to remove two of the padding pointers.

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:105
> +                                          (WebKitPrintOperation *printOperation,

WebKitPrintOperation *print_operation, star moved, and with an underscore. Since it's a public header, welcome back to GNOME style. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:106
> +                                           const gchar* label);

const gchar *label

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151231/47fc081e/attachment.html>