GNOME Bugzilla – Bug 679299
Allow incrementally parsing pango markup
Last modified: 2012-12-14 18:13:20 UTC
See patches. This is designed to be used with g_input_stream_read and other things. Perhaps the API isn't the best -- suggestions and other comments appreciated.
Created attachment 217899 [details] [review] markup: Always start with a <markup> tag It doesn't do anything bad, and will help us when we move to a stream-based approach.
Created attachment 217900 [details] [review] markup: Allow incrementally parsing pango markup This is intended for applications that need to parse pango markup from some sort of GIO stream.
(In reply to comment #1) > Created an attachment (id=217899) [details] [review] > markup: Always start with a <markup> tag > > It doesn't do anything bad Doesn't it now reject markup that actually has a markup tag?
(In reply to comment #3) > (In reply to comment #1) > > Created an attachment (id=217899) [details] [review] [details] [review] > > markup: Always start with a <markup> tag > > > > It doesn't do anything bad > > Doesn't it now reject markup that actually has a markup tag? Why would it? It would become <markup><markup>Foo</markup></markup>, which isn't invalid to the parser, and the start/end tag handlers don't change any state, so it's a noop.
I see. Well, that's weird, but fine. Are you going to use this in an application?
Yes.
Feel free to commit after adding proper "Since" tags and hooking up in documentation.
"Hooking up in documentation"?
Add to docs/pango-sections.txt
I did that in the attached patch, no?
Additionally, I'm not the biggest fan of the API right now -- GMarkup isn't intropsection-friendly. Do you think we want a PangoMarkupParser * that you explicitly create, call _parse() on, and free?
> Additionally, I'm not the biggest fan of the API right now -- GMarkup isn't > intropsection-friendly. Do you think we want a PangoMarkupParser * that you > explicitly create, call _parse() on, and free? I really like reusing GMarkupParseContext here. Though, we should have a GMarkupParser API too, such that apps (say, The GIMP, etc) can push that onto a ParseContext. I remember we had this discussion before, but can't find it. Agree that the API needs more work. CC'ing a few.
I guess one thing I'm not a fan of right now is the _finish method. It returns a boolean variable and an error, but there's not much you can do with it, as the context has been freed. Maybe we should explicitly split up _free and _finish?
Created attachment 217949 [details] [review] markup: Allow incrementally parsing pango markup This is intended for applications that need to parse pango markup from some sort of GIO stream. OK, I'm *much* more happy with this. After calling pango_markup_parser_finish, you are supposed to call g_markup_parse_context_free. This makes it consistent with the rest of our stack, and gives you a change to handle an error when finishing before everything is cleared for you. It also removes a lot of other dirty code.
Created attachment 217950 [details] [review] markup: Allow incrementally parsing pango markup This is intended for applications that need to parse pango markup from some sort of GIO stream. Restore the memory efficiency in the pango_parse_markup case where we have a NULL attr_list out pointer, and restore some accidentally removed invariant checks.
Ping on this?
LGTM. The only thing I don't like is that the constructor function can return an error. Also, Perhaps it should really return a subclass of GMarkupParserContext. Oh wait, you said that's not a true GObject I think. In which case the current patch is fine.
Attachment 217899 [details] pushed as 25d4222 - markup: Always start with a <markup> tag Attachment 217950 [details] pushed as 29020c7 - markup: Allow incrementally parsing pango markup
Thanks. Still like to see the error be dropped from the constructor...
Created attachment 231579 [details] [review] markup: Remove error from markup parser constructor
Created attachment 231580 [details] [review] markup: Remove error from markup parser constructor
Comment on attachment 231580 [details] [review] markup: Remove error from markup parser constructor Attachment 231580 [details] pushed as 7908fa2 - markup: Remove error from markup parser constructor