<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add an API to add a custom tab into the print dialog"
href="https://bugs.webkit.org/show_bug.cgi?id=151998">bug 151998</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #267000 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add an API to add a custom tab into the print dialog"
href="https://bugs.webkit.org/show_bug.cgi?id=151998#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Add an API to add a custom tab into the print dialog"
href="https://bugs.webkit.org/show_bug.cgi?id=151998">bug 151998</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=267000&action=diff" name="attach_267000" title="Proposed patch based on GtkPrintOperation implementation">attachment 267000</a> <a href="attachment.cgi?id=267000&action=edit" title="Proposed patch based on GtkPrintOperation implementation">[details]</a></span>
Proposed patch based on GtkPrintOperation implementation
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=267000&action=review">https://bugs.webkit.org/attachment.cgi?id=267000&action=review</a>
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?
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:152
> +static GtkWidget* webkitPrintOperationCreateCustomWidget(WebKitPrintOperation *printOperation)</span >
WebKitPrintOperation* printOperation
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:213
> + g_object_class_install_property(gObjectClass,</span >
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));
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:269
> + * Returns: (transfer none): A custom widget that gets embedded in</span >
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.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:339
> + customTabLabel = _("Application");</span >
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. :)
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:343
> +</span >
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. :)
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.cpp:562
> + * @label: (allow-none): the label to use, or %NULL to use the default label</span >
If you follow my suggestion and get rid of the default label, this would no longer be (allow-none).
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:71
> void (*_webkit_reserved1) (void);</span >
Careful not to break ABI here. Since you're adding two virtual signals, you have to remove two of the padding pointers.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:105
> + (WebKitPrintOperation *printOperation,</span >
WebKitPrintOperation *print_operation, star moved, and with an underscore. Since it's a public header, welcome back to GNOME style. :)
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitPrintOperation.h:106
> + const gchar* label);</span >
const gchar *label</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>