Provide gtk::MessageDialog manual setter for secondary markup and text #375

Merged
merged 1 commit into from Sep 6, 2016

Projects

None yet

4 participants

@hfiguiere
Contributor

format_secondary_text() and format_secondary_markup() aren't wrapped due varargs.

Implemented plain setters so that we can set the secondary text/markup. Use the Rust format!() to format it.

@hfiguiere
Contributor
hfiguiere commented Aug 26, 2016 edited

Also replaced ptr::null_mut() by ptr::null() and changed its type to be correct (T is turned to *const T). It was working because these are NULL to terminate the varargs

@EPashkin
Member

@hfiguiere, thanks for PR.
Please, add ignoring these function and 'new' for MessageDialog to Gir.toml and do regen

@EPashkin
Member
EPashkin commented Aug 26, 2016 edited

On second thought, as gtk_message_dialog_new_with_markup can be done in same way, seems better do all GtkMessageDialog manually and remove it from generation.
Also seems all message parameters better be Option<&str>
@gkoz, @GuillaumeGomez what you think?

@GuillaumeGomez
Member

&str parameters should always be Option<&str> because we have to be able to provide NULL equivalent. If possible, I'd prefer to keep generation as much as possible.

@gkoz
Member
gkoz commented Aug 26, 2016 edited

Seems good as it is to me. With regard to Option<&str> it's consistent with whether message_format is declared as nullable. new_with_markup can't be implemented in the same way because we're passing the whole string as a single argument and it will get escaped inappropriately. Ignoring new feels somewhat beside the point and also I'm pretty sure the generator won't handle varargs ever.

@hfiguiere
Contributor

Only gtk_message_dialog_format_secondary_text() has message_format declared as nullable. Hence that's the only one I have declared using Option<&str>, while it is true the doc says "or NULL" for gtk_message_dialog_format_secondary_markup()

@gkoz
Member
gkoz commented Aug 29, 2016

@GuillaumeGomez @EPashkin since there were some differences in opinions we need at least one of you to chime in.

@GuillaumeGomez
Member

I maintain my previous position. :)

@gkoz
Member
gkoz commented Aug 29, 2016

To expand on this note that removing the secondary text is achievable via set_secondary_text(None) and making set_secondary_markup also take Option just penalizes its users: if someone asks why they have to pass Some to it (markup is a special case of text implying the text is not empty) I don't see how I could justify it.

@EPashkin
Member

Agree with @gkoz, 2 Option unneeded as functions paired and change one parameter.
So I'm ๐Ÿ‘ for accept this PR as is.

@hfiguiere hfiguiere Provide gtk::MessageDialog manual setter for secondary markup and text
1b284d2
@hfiguiere
Contributor

@GuillaumeGomez I now have taken into account your suggestion. Both accept an Option<&str> for the parameter.

@GuillaumeGomez
Member

@hfiguiere: Great, thanks!
@EPashkin & @gkoz: All good for you?

@gkoz
Member
gkoz commented Sep 6, 2016

@GuillaumeGomez It'd be nice to know your reasoning for making both arguments optional, considering the majority preference to not do it and inconsistency with metadata we have. To reiterate my unanswered question: since the set_secondary_markup method is an extension of set_secondary_text why should one have to pass Some(...) to set_secondary_markup?

@GuillaumeGomez
Member

Like I previously said, all &str should be Option<&str> to allow to make the difference between NULL and an empty string. Now if there is a specific reason in here which demonstrate that a NULL pointer cannot be handled by gtk, then you can remove it.

@gkoz
Member
gkoz commented Sep 6, 2016

In this context my point is what would be the reason to pass either None or "" to set_secondary_markup? Neither has any markup.

@GuillaumeGomez
Member

Then whatever.

@hfiguiere
Contributor

In both set_secondary_text() and set_secondary_markup(), passing NULL will reset the state as to "no secondary text". This has an impact on the message dialog.

@hfiguiere
Contributor

As I said before, only set_secondary_text() is annotated as allowing NULL, but according to the documentation, both supports it.

@gkoz
Member
gkoz commented Sep 6, 2016

After digging through the history the lack of an allow-none annotation in the 'markup' case appears to be unintentional (they added the annotation with a script, which failed to handle a multiline docstring or something). This makes my argument against Option weaker so I'll just merge.
@hfiguiere sorry for the delay.

@gkoz gkoz merged commit 5f09817 into gtk-rs:master Sep 6, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GuillaumeGomez
Member

Victory. ๐Ÿ˜†

๐ŸŽ‰

Thanks for your work @hfiguiere!

@hfiguiere hfiguiere deleted the hfiguiere:message-dialog-secondary branch Sep 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment