| Summary: | install error callback to redirect errors to the GLib logging framework | ||
|---|---|---|---|
| Product: | poppler | Reporter: | Christian Persch (GNOME) <chpe> |
| Component: | glib frontend | Assignee: | poppler-bugs <poppler-bugs> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | medium | CC: | uckelman |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
proposed patch
new patch updated patch make the strings a bit more informative updated patch |
||
After discussing this on IRC, we came up with a different scheme where instead of creating new API, we just redirect the messages to the GLib logging framework. Since there's no single entry point in the public poppler-glib API, we just use a library constructor for this. Created attachment 92383 [details] [review] new patch Created attachment 92384 [details] [review] updated patch Created attachment 92386 [details] [review] make the strings a bit more informative Patch tested with evince, works fine. By default, nothing is printed; run with G_MESSAGES_DEBUG=Poppler (or =all) to see the messages. Comment on attachment 92384 [details] [review] updated patch I guess this one is obsolete now Comment on attachment 92386 [details] [review] make the strings a bit more informative Review of attachment 92386 [details] [review]: ----------------------------------------------------------------- Yes, a lot simpler, and we don't need new API nor even change evince. ::: glib/poppler.cc @@ +90,5 @@ > + category == errCommandLine || > + category == errNotAllowed) > + return; > + > + g_log (G_LOG_DOMAIN, G_LOG_LEVEL_INFO, Shouldn't we use "Poppler" as log domain? @@ +95,5 @@ > + "%s at position %" G_GOFFSET_FORMAT ": %s", > + cat_str[category], (goffset) pos, message); > +} > + > +static void poppler_constructor (void) __attribute__((__constructor__)); Why do you need this prototype? I think you can annotate the method directly, for example: static void __attribute__((__constructor__)) poppler_constructor (void) { setErrorCallback (error_cb, NULL); } Created attachment 92409 [details] [review] updated patch (In reply to comment #6) > > + g_log (G_LOG_DOMAIN, G_LOG_LEVEL_INFO, > > Shouldn't we use "Poppler" as log domain? It does; G_LOG_DOMAIN is #define'd to "Poppler" in Makefile.am (INCLUDES). > > +static void poppler_constructor (void) __attribute__((__constructor__)); > > Why do you need this prototype? I think you can annotate the method > directly, for example: > > static void __attribute__((__constructor__)) poppler_constructor (void) > { > setErrorCallback (error_cb, NULL); > } Fixed. (In reply to comment #7) > Created attachment 92409 [details] [review] [review] > updated patch > > (In reply to comment #6) > > > + g_log (G_LOG_DOMAIN, G_LOG_LEVEL_INFO, > > > > Shouldn't we use "Poppler" as log domain? > > It does; G_LOG_DOMAIN is #define'd to "Poppler" in Makefile.am (INCLUDES). Oh, right! > > > +static void poppler_constructor (void) __attribute__((__constructor__)); > > > > Why do you need this prototype? I think you can annotate the method > > directly, for example: > > > > static void __attribute__((__constructor__)) poppler_constructor (void) > > { > > setErrorCallback (error_cb, NULL); > > } > > Fixed. Pushed to git master, thank you very much. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Created attachment 91474 [details] proposed patch The poppler library outputs some warnings (PDF syntax warnings/error etc) to stderr by default. For evince, we want to suppress that output by default, so we need some poppler-glib API to wrap setErrorCallback from Error.h (we don't want to use poppler internal APIs directly in evince).