On Monday 26 of November 2012 10:01:42 Jiri Moskovcak wrote:
> ---
> libreport.spec.in | 1 +
> src/cli/cli-report.c | 8 ++--
> src/gtk-helpers/event_config_dialog.c | 6 +--
> src/gui-wizard-gtk/wizard.c | 18 ++++----
> src/include/Makefile.am | 1 +
> src/include/config_item_info.h | 45 +++++++++++++++++++
> src/include/event_config.h | 20 +++++++--
> src/lib/Makefile.am | 3 +-
> src/lib/config_item_info.c | 85
> +++++++++++++++++++++++++++++++++++ src/lib/event_config.c |
> 56 +++++++++++++++++++++-- src/lib/event_xml_parser.c | 19
> ++++----
> src/report-newt/report-newt.c | 8 ++--
> 12 files changed, 233 insertions(+), 37 deletions(-)
> create mode 100644 src/include/config_item_info.h
> create mode 100644 src/lib/config_item_info.c
>
> diff --git a/libreport.spec.in b/libreport.spec.in
> index 8eefb3d..cda8f23 100644
> --- a/libreport.spec.in
> +++ b/libreport.spec.in
> @@ -290,6 +290,7 @@ gtk-update-icon-cache %{_datadir}/icons/hicolor
> &>/dev/null || : %{_includedir}/libreport/report.h
> %{_includedir}/libreport/run_event.h
> %{_includedir}/libreport/file_obj.h
> +%{_includedir}/libreport/config_item_info.h
> # Private api headers:
> %{_includedir}/libreport/internal_abrt_dbus.h
> %{_includedir}/libreport/internal_libreport.h
> diff --git a/src/cli/cli-report.c b/src/cli/cli-report.c
> index f256e25..d6697e9 100644
> --- a/src/cli/cli-report.c
> +++ b/src/cli/cli-report.c
> @@ -617,7 +617,7 @@ char *select_event_option(GList *list_options)
> if (config)
> {
> ++pos;
> - printf(" %i) %s\n", pos, config->screen_name);
> + printf(" %i) %s\n", pos, ec_get_screen_name(config));
> }
> }
>
> @@ -717,7 +717,7 @@ int collect(const char *dump_dir_name, int batch)
> if (!config)
> VERB1 log("No configuration file found for collector '%s'",
> collector_name);
>
> - printf(" %d) %s\n", i, (config && config->screen_name) ?
> config->screen_name : collector_name); + printf(" %d) %s\n", i,
> (config && ec_get_screen_name(config)) ? ec_get_screen_name(config) :
> collector_name); }
I would use a temporary variable and not call the get function twice.
(this note is valid for all following similar construction)
>
> read_from_stdin(_("Select collector(s): "), wanted_collectors,
> sizeof(wanted_collectors)); @@ -901,7 +901,7 @@ int report(const char
> *dump_dir_name, int flags) char *reporter_name = (char *) li->data;
> event_config_t *config = get_event_config(reporter_name);
>
> - printf(" %d) %s\n", i, (config && config->screen_name) ?
> config->screen_name : reporter_name); + printf(" %d) %s\n", i,
> (config && ec_get_screen_name(config)) ? ec_get_screen_name(config) :
> reporter_name); }
>
> read_from_stdin(_("Select reporter(s): "), wanted_reporters,
> sizeof(wanted_reporters)); @@ -1007,7 +1007,7 @@ int run_events_chain(const
> char *dump_dir_name, GList *chain) {
> char *msg = xasprintf(_("Event '%s' requires permission to
> send possibly sensitive data." " Do you want to continue?"), -
> config->screen_name ? config->screen_name : event); +
> ec_get_screen_name(config) ? ec_get_screen_name(config)
> : event); const bool response = ask_yesno(msg);
> free(msg);
> if (!response)
> diff --git a/src/gtk-helpers/event_config_dialog.c
> b/src/gtk-helpers/event_config_dialog.c index a067511..f135c9a 100644
> --- a/src/gtk-helpers/event_config_dialog.c
> +++ b/src/gtk-helpers/event_config_dialog.c
> @@ -236,8 +236,8 @@ static void add_event_to_liststore(gpointer key,
> gpointer value, gpointer user_d event_config_t *ec = (event_config_t
> *)value;
>
> char *event_label;
> - if (ec->screen_name != NULL && ec->description != NULL)
> - event_label = xasprintf("<b>%s</b>\n%s", ec->screen_name,
> ec->description); + if (ec_get_screen_name(ec) != NULL &&
> ec_get_description(ec) != NULL) + event_label =
> xasprintf("<b>%s</b>\n%s", ec_get_screen_name(ec), ec_get_description(ec));
> else
> //if event has no xml description
> event_label = xasprintf("<b>%s</b>\nNo description available",
> key); @@ -296,7 +296,7 @@ int show_event_config_dialog(const char
> *event_name, GtkWindow *parent) GtkWindow *parent_window = parent ? parent
> : g_event_list_window;
>
> GtkWidget *dialog = gtk_dialog_new_with_buttons(
> - /*title:*/ event->screen_name ? event->screen_name
> : event_name, + /*title:*/ ec_get_screen_name(event)
> ? ec_get_screen_name(event) : event_name, parent_window,
> GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT,
> GTK_STOCK_CANCEL,
> diff --git a/src/gui-wizard-gtk/wizard.c b/src/gui-wizard-gtk/wizard.c
> index f351988..9f1be9f 100644
> --- a/src/gui-wizard-gtk/wizard.c
> +++ b/src/gui-wizard-gtk/wizard.c
> @@ -273,12 +273,12 @@ static void show_event_opt_error_dialog(const char
> *event_name) "reporting will probably fail if you continue " "with the
> current configuration.\n\n" "Read more about the configuration at:
> https://fedorahosted.org/abrt/wiki/AbrtConfiguration"), -
> ec->screen_name);
> + ec_get_screen_name(ec));
> char *markup_message = xasprintf(_("Wrong settings detected for
> <b>%s</b>, " "reporting will probably fail if you continue " "with the
> current configuration.\n\n" "<a
> href=\"https://fedorahosted.org/abrt/wiki/AbrtConfiguration\">Read more
> about the configuration</a>"), -
> ec->screen_name);
> + ec_get_screen_name(ec));
> GtkWidget *wrong_settings = g_top_most_window =
> gtk_message_dialog_new(GTK_WINDOW(g_wnd_assistant), GTK_DIALOG_MODAL |
> GTK_DIALOG_DESTROY_WITH_PARENT,
> GTK_MESSAGE_WARNING,
> @@ -292,7 +292,7 @@ static void show_event_opt_error_dialog(const char
> *event_name) free(markup_message);
>
> GtkWidget *act_area =
> gtk_dialog_get_content_area(GTK_DIALOG(wrong_settings)); - char *
> conf_btn_lbl = xasprintf(_("Con_figure %s"), ec->screen_name); + char *
> conf_btn_lbl = xasprintf(_("Con_figure %s"), ec_get_screen_name(ec));
> GtkWidget *configure_event_btn =
> gtk_button_new_with_mnemonic(conf_btn_lbl);
> g_signal_connect(configure_event_btn, "clicked",
> G_CALLBACK(on_configure_event_cb), (gpointer)event_name);
> free(conf_btn_lbl);
> @@ -1016,9 +1016,9 @@ static event_gui_data_t *add_event_buttons(GtkBox
> *box, if (cfg)
> {
> /* .xml has (presumably) prettier description, use it: */
> - if (cfg->screen_name)
> - event_screen_name = cfg->screen_name;
> - event_description = cfg->description;
> + if (ec_get_screen_name(cfg))
> + event_screen_name = ec_get_screen_name(cfg);
> + event_description = ec_get_description(cfg);
>
> char *missing =
> missing_items_in_comma_list(cfg->ec_requires_items); if (missing)
> @@ -1078,8 +1078,8 @@ static event_gui_data_t *add_event_buttons(GtkBox
> *box, if (func)
> g_signal_connect(G_OBJECT(button), "toggled", func,
> xstrdup(event_name));
>
> - if (cfg && cfg->long_descr)
> - gtk_widget_set_tooltip_text(button, cfg->long_descr);
> + if (cfg && ec_get_long_desc(cfg))
> + gtk_widget_set_tooltip_text(button, ec_get_long_desc(cfg));
>
> event_gui_data_t *event_gui_data = new_event_gui_data_t();
> event_gui_data->event_name = xstrdup(event_name);
> @@ -2431,7 +2431,7 @@ static bool get_sensitive_data_permission(const char
> *event_name)
>
> char *msg = xasprintf(_("Event '%s' requires permission to send
> possibly sensitive data." "\nDo you want to continue?"),
> - event_cfg->screen_name ? event_cfg->screen_name
> : event_name); + ec_get_screen_name(event_cfg) ?
> ec_get_screen_name(event_cfg) : event_name); const bool response =
> ask_yes_no_save_result(msg, "ask_send_sensitive_data"); free(msg);
>
> diff --git a/src/include/Makefile.am b/src/include/Makefile.am
> index 948662e..a9af359 100644
> --- a/src/include/Makefile.am
> +++ b/src/include/Makefile.am
> @@ -8,6 +8,7 @@ libreport_include_HEADERS = \
> run_event.h \
> libreport_curl.h \
> \
> + config_item_info.h \
> file_obj.h \
> internal_libreport.h \
> internal_abrt_dbus.h
> diff --git a/src/include/config_item_info.h b/src/include/config_item_info.h
> new file mode 100644
> index 0000000..dee6bc2
> --- /dev/null
> +++ b/src/include/config_item_info.h
> @@ -0,0 +1,45 @@
> +/*
> + Copyright (C) 2011 ABRT team
> + Copyright (C) 2010 RedHat Inc
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 2 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License along
> + with this program; if not, write to the Free Software Foundation,
> Inc., + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +*/
> +
> +#ifndef LIBREPORT_CONFIG_ITEM_H
> +#define LIBREPORT_CONFIG_ITEM_H
> +
> +typedef struct
> +{
> + char *name; //the event name (from it's filename)
> + char *screen_name; //ui friendly name of the event: "Bugzilla" "RedHat
> Support Upload" + char *description; // "Report to..."/"Save to file".
> Should be one sentence, not long + char *long_desc; // Long(er)
> explanation, if needed
> +
> +} config_item_info_t;
I see get_/set_ function for all members, therefore this structure can be
implemented as opaque data type.
> +
> +config_item_info_t *new_config_info();
> +void free_config_info(config_item_info_t *info);
> +
> +void ci_set_screen_name(config_item_info_t *ci, const char *screen_name);
> +void ci_set_name(config_item_info_t *ci, const char *name);
> +void ci_set_description(config_item_info_t *ci, const char *description);
> +void ci_set_long_desc(config_item_info_t *ci, const char
> *long_description); +
> +extern const char *ci_get_screen_name(config_item_info_t *ci);
> +extern const char *ci_get_name(config_item_info_t *ci);
> +extern const char *ci_get_description(config_item_info_t *ci);
> +extern const char *ci_get_long_desc(config_item_info_t *ci);
> +
> +#endif
> diff --git a/src/include/event_config.h b/src/include/event_config.h
> index 438c723..60e5b94 100644
> --- a/src/include/event_config.h
> +++ b/src/include/event_config.h
> @@ -22,6 +22,7 @@
> #include <stdbool.h>
> #include <glib.h>
> #include "problem_data.h"
> +#include "config_item_info.h"
>
> #ifdef __cplusplus
> extern "C" {
> @@ -68,10 +69,7 @@ void free_event_option(event_option_t *p);
> //structure to hold the option data
> typedef struct
> {
> - char *name; //the event name (from it's filename)
> - char *screen_name; //ui friendly name of the event: "Bugzilla" "RedHat
> Support Upload" - char *description; // "Report to..."/"Save to file".
> Should be one sentence, not long - char *long_descr; // Long(er)
> explanation, if needed
> + config_item_info_t *info;
>
> char *ec_creates_items;
> char *ec_requires_items;
> @@ -87,6 +85,19 @@ typedef struct
> } event_config_t;
>
This could be an opaque structure as well.
> event_config_t *new_event_config(void);
> +config_item_info_t *ec_get_config_info(event_config_t * ec);
> +const char *ec_get_screen_name(event_config_t *ec);
> +void ec_set_screen_name(event_config_t *ec, const char *screen_name);
> +void ec_set_name(event_config_t *ec, const char *name);
> +
> +const char *ec_get_description(event_config_t *ec);
> +void ec_set_description(event_config_t *ec, const char *description);
> +
> +const char *ec_get_name(event_config_t *ec);
> +const char *ec_get_long_desc(event_config_t *ec);
> +void ec_set_long_desc(event_config_t *ec, const char *long_desc);
> +bool ec_is_configurable(event_config_t* ec);
> +
> void free_event_config(event_config_t *p);
>
>
> @@ -99,6 +110,7 @@ void free_event_config_data(void);
> event_config_t *get_event_config(const char *event_name);
> event_option_t *get_event_option_from_list(const char *option_name, GList
> *event_options);
>
> +
> extern GHashTable *g_event_config_list; // for iterating through entire
> list of all loaded configs
>
> GList *export_event_config(const char *event_name);
> diff --git a/src/lib/Makefile.am b/src/lib/Makefile.am
> index 2865624..0a7a819 100644
> --- a/src/lib/Makefile.am
> +++ b/src/lib/Makefile.am
> @@ -49,7 +49,8 @@ libreport_la_SOURCES = \
> client.c \
> utf8.c \
> file_list.c \
> - file_obj.c
> + file_obj.c \
> + config_item_info.c
> libreport_la_CPPFLAGS = \
> -I$(srcdir)/../include \
> -DLOCALSTATEDIR='"$(localstatedir)"' \
> diff --git a/src/lib/config_item_info.c b/src/lib/config_item_info.c
> new file mode 100644
> index 0000000..1635e72
> --- /dev/null
> +++ b/src/lib/config_item_info.c
> @@ -0,0 +1,85 @@
> +/*
> + Copyright (C) 2011 ABRT team
> + Copyright (C) 2010 RedHat Inc
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 2 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License along
> + with this program; if not, write to the Free Software Foundation,
> Inc., + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +*/
> +
> +#include "internal_libreport.h"
> +//#include "config_item_info.h"
> +
> +
> +config_item_info_t *new_config_info()
> +{
> + config_item_info_t *info = (config_item_info_t
> *)xzalloc(sizeof(config_item_info_t)); + return info;
> +}
> +
> +void free_config_info(config_item_info_t *info)
> +{
> + if (info == NULL)
> + return;
> +
> + free(info->name);
> + free(info->screen_name);
> + free(info->description);
> + free(info->long_desc);
> +
> + free(info);
> +}
> +
> +void ci_set_screen_name(config_item_info_t *ci, const char *screen_name)
> +{
> + free(ci->screen_name);
> + ci->screen_name = xstrdup(screen_name);
> +}
> +
> +void ci_set_name(config_item_info_t *ci, const char *name)
> +{
> + free(ci->name);
> + ci->name = xstrdup(name);
> +}
> +
> +void ci_set_description(config_item_info_t *ci, const char *description)
> +{
> + free(ci->description);
> + ci->description = xstrdup(description);
> +}
> +
> +void ci_set_long_desc(config_item_info_t *ci, const char *long_description)
> +{
> + free(ci->long_desc);
> + ci->long_desc = xstrdup(long_description);
> +}
> +
> +inline const char *ci_get_screen_name(config_item_info_t *ci)
> +{
> + return ci->screen_name;
> +}
> +
> +inline const char *ci_get_name(config_item_info_t *ci)
> +{
> + return ci->name;
> +}
> +
> +inline const char *ci_get_description(config_item_info_t *ci)
> +{
> + return ci->description;
> +}
> +
> +inline const char *ci_get_long_desc(config_item_info_t *ci)
> +{
> + return ci->long_desc;
> +}
I don't fully understand C inlining rules but AFAIK at least one
declaration either without inline or with extern will emit a standard
function. Therefore inline keyword is pointless because of extern
keyword in header. ???
If you want to make the functions inlined and shared between
modules, you must define them in header and use statict inline keywords. ???
> diff --git a/src/lib/event_config.c b/src/lib/event_config.c
> index 2019e53..3c094bd 100644
> --- a/src/lib/event_config.c
> +++ b/src/lib/event_config.c
> @@ -16,6 +16,7 @@
> with this program; if not, write to the Free Software Foundation, Inc.,
> 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> */
> +
> #include "internal_libreport.h"
>
> GHashTable *g_event_config_list;
> @@ -29,9 +30,60 @@ event_option_t *new_event_option(void)
> event_config_t *new_event_config(void)
> {
> event_config_t *e = xzalloc(sizeof(event_config_t));
> + e->info = new_config_info();
> return e;
> }
>
> +config_item_info_t *ec_get_config_info(event_config_t * ec)
> +{
> + return ec->info;
> +}
> +
> +void ec_set_name(event_config_t *ec, const char *name)
> +{
> + ci_set_name(ec->info, name);
> +}
> +
> +void ec_set_screen_name(event_config_t *ec, const char *screen_name)
> +{
> + ci_set_screen_name(ec->info, screen_name);
> +}
> +
> +const char *ec_get_screen_name(event_config_t *ec)
> +{
> + return ci_get_screen_name(ec->info);
> +}
> +
> +const char *ec_get_description(event_config_t *ec)
> +{
> + return ci_get_description(ec->info);
> +}
> +
> +void ec_set_description(event_config_t *ec, const char *description)
> +{
> + ci_set_description(ec->info, description);
> +}
> +
> +const char *ec_get_name(event_config_t *ec)
> +{
> + return ci_get_name(ec->info);
> +}
> +
> +const char *ec_get_long_desc(event_config_t *ec)
> +{
> + return ci_get_long_desc(ec->info);
> +}
> +
> +void ec_set_long_desc(event_config_t *ec, const char *long_descr)
> +{
> + ci_set_long_desc(ec->info, long_descr);
> +}
> +
> +bool ec_is_configurable(event_config_t* ec)
> +{
> + return g_list_length(ec->options) > 0;
> +}
> +
> void free_event_option(event_option_t *p)
> {
> if (!p)
> @@ -50,9 +102,7 @@ void free_event_config(event_config_t *p)
> if (!p)
> return;
>
> - free(p->screen_name);
> - free(p->description);
> - free(p->long_descr);
> + free_config_info(p->info);
> free(p->ec_creates_items);
> free(p->ec_requires_items);
> free(p->ec_exclude_items_by_default);
> diff --git a/src/lib/event_xml_parser.c b/src/lib/event_xml_parser.c
> index 157b717..585e2e5 100644
> --- a/src/lib/event_xml_parser.c
> +++ b/src/lib/event_xml_parser.c
> @@ -336,11 +336,11 @@ static void text(GMarkupParseContext *context,
> * OR the label is still not set and we found the default
> value */
> if (parse_data->attribute_lang[0] != '\0'
> - || !ui->screen_name /* && parse_data->attribute_lang is ""
> - always true */ + || !ec_get_screen_name(ui) /* &&
> parse_data->attribute_lang is "" - always true */ ) {
> VERB2 log("event name:'%s'", text_copy);
> - free(ui->screen_name);
> - ui->screen_name = text_copy;
> + ec_set_screen_name(ui, text_copy);
> + free(text_copy);
> text_copy = NULL;
> }
> }
> @@ -355,10 +355,10 @@ static void text(GMarkupParseContext *context,
> * OR the description is still not set and we found the
> default value */
> if (parse_data->attribute_lang[0] != '\0'
> - || !ui->description /* && parse_data->attribute_lang is ""
> - always true */ + || !ec_get_description(ui) /* &&
> parse_data->attribute_lang is "" - always true */ ) {
> - free(ui->description);
> - ui->description = text_copy;
> + ec_set_description(ui, text_copy);
> + free(text_copy);
> text_copy = NULL;
> }
> }
> @@ -373,10 +373,10 @@ static void text(GMarkupParseContext *context,
> * OR the description is still not set and we found the
> default value */
> if (parse_data->attribute_lang[0] != '\0'
> - || !ui->long_descr /* && parse_data->attribute_lang is ""
> - always true */ + || !ec_get_long_desc(ui) /* &&
> parse_data->attribute_lang is "" - always true */ ) {
> - free(ui->long_descr);
> - ui->long_descr = text_copy;
> + ec_set_long_desc(ui, text_copy);
> + free(text_copy);
> text_copy = NULL;
> }
> }
> @@ -463,6 +463,7 @@ static void error(GMarkupParseContext *context,
>
> void load_event_description_from_file(event_config_t *event_config, const
> char* filename) {
> + VERB1 log("loading event: '%s'", filename);
> struct my_parse_data parse_data = { event_config, NULL, NULL, NULL };
> parse_data.cur_locale = setlocale(LC_ALL, NULL);
>
> diff --git a/src/report-newt/report-newt.c b/src/report-newt/report-newt.c
> index 8532e82..44d30bd 100644
> --- a/src/report-newt/report-newt.c
> +++ b/src/report-newt/report-newt.c
> @@ -60,8 +60,8 @@ static int select_reporters(GArray *reporters)
> {
> struct reporter *r = &g_array_index(reporters, struct reporter, i);
>
> - checkboxes[i] = newtCheckbox(20, i + 1, r->config &&
> r->config->screen_name ? - r->config->screen_name : r->name,
> 0, NULL, NULL); + checkboxes[i] = newtCheckbox(20, i + 1, r->config
> && ec_get_screen_name(r->config) ? +
> ec_get_screen_name(r->config) : r->name, 0, NULL, NULL);
> newtGridSetField(cgrid, 0, i, NEWT_GRID_COMPONENT, checkboxes[i], 0, 0, 0,
> 0, NEWT_ANCHOR_LEFT, 0); }
>
> @@ -104,8 +104,8 @@ static int configure_reporter(struct reporter *r, bool
> skip_if_valid) while ((error_table = validate_event(r->name)) ||
> (!skip_if_valid && first && r->config))
> {
> - text = newtTextboxReflowed(0, 0, r->config->screen_name ?
> - r->config->screen_name : r->name, 35, 5, 5, 0);
> + text = newtTextboxReflowed(0, 0, ec_get_screen_name(r->config) ?
> + xstrdup(ec_get_screen_name(r->config)) : r->name, 35, 5, 5,
> 0);
>
> num_opts = g_list_length(r->config->options);
> options = xmalloc(sizeof (newtComponent) * num_opts);