On Tuesday 11 of December 2012 17:11:06 Denys Vlasenko wrote:
> On 12/11/2012 01:45 PM, Jakub Filak wrote:
> > - related to trac#916
>
>
> > +/* Reserve 7 bits for configuration of dialog version */
>
> I didn't understand the above comment..
I wanted to explain why I use 0x100 instead of 1 << 2 as
we usually do. I'm trying to anticipate future needs :) bad idea, right?
> .
>
> > +typedef enum {
> > + ASK_YES_NO_SAVE_RESULT = 0x001,
>
> It looks like you don't check this bit anywhere,
> you only set it sometimes. Did I miss something?
No, you are right. I don't check this bit anywhere,
I just pass it as an argument instead of '/* flags */ 0'.
This construction looks more natural to me.
>
> > + ASK_YES_NO_YESFOREVER = 0x100,
>
> What ASK_YES_NO_YESFOREVER means?
I'm sorry for that, I though that I comment it well
with run_ask_yes_no_yesforever_dialog() function.
I'll add a comment here.
ask_yes_no_yesforever means that you can reply with "Yes", "No"
an "Yes && Don't ask me again".
How it works?
If you check the checkbox the "No" button will be disabled
and you will be able to click only the "Yes" button.
Once you answer "Yes && Don't ask me again", the dialog won't appear
next time you call the function and Non 0 value is returned immediately.
>
> > +} ask_yes_no_dialog_flags;
> > +
> > +static int run_ask_yes_no_save_generic_result_dialog(ask_yes_no_dialog_flags flags,
> > + const char *key,
> > + const char *message,
> > + GtkWindow *parent)
> > {
> > const char *ask_result = get_user_setting(key);
> >
> > - if (ask_result && string_to_bool(ask_result) == false)
> > - /* Do you want to be asked? -> No, I don't. Do whatever you want */
> > - return true;
> > + if (ask_result)
> > + {
> > + const bool ret = string_to_bool(ask_result);
> > + if (flags & ASK_YES_NO_YESFOREVER && ret == false)
>
> I would use ()s: ((flags & ASK_YES_NO_YESFOREVER) && ret == false)
> there is a subtlety in C's precedence rules in these operators somewhere...
> I only remember that < > <= >= == != are safe, but &, <<, ? : etc
> - can't memorize those :)
will do
>
> > + /* Do you want to be asked? -> No, I don't. Do whatever you want */
> > + return true;
> > +
> > + return ret;
> > + }
>
>
>
> > +/*
> > + * Function shows a dialog with 'Yes/No' buttons and a check box allowing to
> > + * remember the answer. The "Don't ask me again" response is stored in
> > + * configuration file under 'key'.
> > + */
>
> The comment needs to be in *.h file. I would imagine users read _that_ file.
> For one, they may _miss_ *.c file on their system.
>
>
>
>
> Yes, like you did it here:
>
> > --- a/src/gtk-helpers/internal_libreport_gtk.h
> > +++ b/src/gtk-helpers/internal_libreport_gtk.h
> > @@ -115,6 +115,24 @@ GList *find_url_tokens(const char *line);
> > #define run_ask_yes_no_yesforever_dialog libreport_run_ask_yes_no_yesforever_dialog
> > int run_ask_yes_no_yesforever_dialog(const char *key, const char *message, GtkWindow *parent);
> >
> > +/*
> > + * Runs a dialog with 'Yes'/'No' buttons and 'Don't ask me again' check box and
> > + * waits until the dialog is closed. This variant of dialog allows user to
> > + * click both of buttons if the check box is checked and stores a string which
> > + * corresponds to an answer.
>
> Is the answer stored always, or only if checkbox is 'checked'?
I'll write a clearer comment.
>
> > + *
> > + * Uses libreport's user settings. Don't forget to call load_user_settings()
> > + * before the first call of this funcion and call save_user_settings() after
> > + * the last call of this function.
> > + *
> > + * @param key Key under which the response is stored. Not NULL
> > + * @param message Displayed message. Not NULL
> > + * @param parent Transient parent or NULL
> > + * @returns Non 0 if the answer is "Yes"; otherwise 0
> > + */
> > +#define run_ask_yes_no_save_result_dialog libreport_run_ask_yes_no_save_result_dialog
> > +int run_ask_yes_no_save_result_dialog(const char *key, const char *message, GtkWindow *parent);
> > +
> > #ifdef __cplusplus
> > }
> > #endif
>