GNOME Bugzilla – Bug 618560
Split the highlighting engine into Syntax_analyzer + highlighting_engine
Last modified: 2020-11-13 23:29:10 UTC
Created attachment 160988 [details] [review] Preliminary patch. Splitting the syntax analysis out of the highlighting engine allow for better extensibility of gtksourceview. I attach a preliminary patch that works but need API review.
Review of attachment 160988 [details] [review]: Minor comment. ::: configure.ac @@ +17,3 @@ AM_MAINTAINER_MODE([enable]) +m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES([yes])]) let's put this in a separate patch, to push now
Created attachment 160992 [details] [review] first of three patches. I was able to split the patch for easier review. So, this first patch just copies gtksourcecontextengine.c/h into gtkhighlightengine.c/h.
Created attachment 160993 [details] [review] Second patch. This patch splits the engine. Second patch, here there is the split of the engine.
Created attachment 160994 [details] [review] Third patch. This modifies the rest of gtksourceview Third patch. This modifies all the other files to adapt for the splitted engine.
Review of attachment 160992 [details] [review]: mmm, I do not like gtkhighlightengine as name maybe gtksourcehighlightengine ?
Review of attachment 160994 [details] [review]: Minor comments from my part bellow. ::: gtksourceview/gtksourcebuffer.c @@ +309,3 @@ param_types[0] = GTK_TYPE_TEXT_ITER | G_SIGNAL_TYPE_STATIC_SCOPE; param_types[1] = GTK_TYPE_TEXT_ITER | G_SIGNAL_TYPE_STATIC_SCOPE; + //_types[2] = G_TYPE_POINTER; what's this for? You forgot to remove this? @@ +1399,3 @@ + **/ +void +gtk_source_buffer_set_analyze_syntax (GtkSourceBuffer *buffer, do we need this api? I would go for activate the analisys when it is needed, this is when the highlighting or the folding is activated ::: gtksourceview/gtksourcebuffer.h @@ +102,3 @@ +gboolean gtk_source_buffer_get_analyze_syntax (GtkSourceBuffer *buffer); +void gtk_source_buffer_set_analyze_syntax (GtkSourceBuffer *buffer, what I said before, In my opinion I would remove this api. ::: gtksourceview/gtksourceengine.h @@ +77,3 @@ gint offset, gint length); +void _gtk_source_engine_update (GtkSourceEngine *engine, I do not like too much the name update, any better ideas?
Nacho, thanks for the comments. For the moment I just want to say that there are a lot of comments in the code, since there are two or three functions over there I am not yet calling... About your comments. I don't like the name GtkHighlightEngine neither.... Maybe something like gregier proposed in his patch (GtkSourceSyntaxHighlighter). I don't particularly like the old name GtkSourceContextEngine. I 'd prefer GtkSourceSyntaxEngine... but I left this as it was... Furthermore, I don't see the point in having GtkSourceEngine base class. I would prefer to have a GtkSourceSyntaxHelper base class for GtkSourceSyntaxHighlighter and GtkSourceSyntaxFold.... The important point is not to use the name Engine in both classes (gtkhighlightengine and gtksourcecontextengine) since they are classes with different purposes.
We should see if it makes sense to have interfaces instead of base classes. For example we could have GtkSourceSyntaxAnalyserIface, GtkSourceSyntaxHandlerIface where the analyser is required to build some kind of syntax tree, emit signals on what should be highlighted and where to apply context classes. A handler can then be registered on a view, and hook into the analyser to actually do something useful with the syntax (like applying the highlighting, applying a context class, figuring out folding). Basicly, I'm not sure any kind of base class really makes sense (GtkSourceSyntaxHelper doesn't seem very useful). With regard to the patch, I still have to look at it in more depth, but if there are still a lot of comments and things not really working, maybe you can first make sure that those issues are fixed. Otherwise we'll be wasting a lot of review time on issues that you were already aware of.
Hi, you can find an updated patch in http://github.com/jaliste/gtksourceview (branch new_split_engine)... I'd appreciate if you can take a lot at it and give more comments on the work. The new patch hides the internals of the GtkSourceContextEngine, which means that the contents of gtksourcecontextengine-private.h are now minimal and could be moved to gtksourceengine.h or something like that (after we figure out how to refactor the API). I created a new struct called Annotation which contains all style and context class information. We could add accessors to this structure if needed and move all of it to a new file. For the API refactor, here are my thoughts: now the GtkSourceContextEngine (or any other AST parser) should implement three public methods, One for priv->buffer, One for priv->root_segment and one for priv->tags. He should also emit a signal syntax-tree-updated, and that's all the common api I can think of. So Please comment if a common base class (as it's now with gtksourceengine) or a GInterface is better suited (and therefore we need to refactor gtksourceengine into a interface gtksourceASTparser).
Created attachment 161872 [details] [review] Split Engine v2
Created attachment 163862 [details] [review] Delete unused stuff from gtksourcecontextengine.c Hi, I detected some unused code in gtksourcecontextengine.c. Here it is the patch that removes the unused code. Please review and apply if good.
Review of attachment 163862 [details] [review]: patch looks ok, though I wonder when that stopped being used... maybe the bug is not the dead code, but the fact that some parts went missing. We'll need to inspect the git history a bit
I rebased my branch in github.com against master (see github.com/jaliste/gtksourceview/tree/split_engine_rebased). I corrected (hopefully) all the issues I am aware of, so this should be ready for review. Please note that a new issue arises from the possibility of having syntax_analysis enabled and syntax_highlighting disabled AT THE SAME TIME, so IMO, this should be resolved after this code is merged (but I open to work it out before aswell). Description of the new issue: if the buffer has the syntax_highlighting disabled (but the syntax_analysis) enabled, and then I enable syntax_highlighting, the _gtk_source_highlighter_ensure_highlighting will ensure the highlighting only for the exposed region. This has as a side effect that the scrolling will be slower. Why this is new? If the syntax_analysis and syntax_highlighting were enabled (as they are by default) before loading the text, the idle worker function of the engine will analyze all the text and therefore the highlighter will also highlight all the text (by hooking to the highlight_update signal).
Created attachment 164011 [details] [review] Split Engine v3 This is jaliste's branch squashed into one patch for review.
Review of attachment 164011 [details] [review]: Mino issue ::: gtksourceview/gtksourcecontextengine.c @@ +8,1 @@ * GtkSourceView is free software; you can redistribute it and/or Please add a blank line here
Created attachment 164025 [details] [review] New version, more clean. So, doing diff with master realized I moved a lot of things in gtksourcecontextengine.c without any good reason. So here is an updated (squashed) patch that should be easier to review.
Created attachment 164091 [details] [review] Patch to be applied after the big one. When testing patch 164025, I realized of two crashes caused by the patch. This patch corrects them.
Review of attachment 164025 [details] [review]: first short round of comments on the buffer part ::: gtksourceview/gtksourcebuffer.c @@ +240,1 @@ It's not very clear to me why this is a property... when would a user api turn off analysis? I think it should just be internal and either always on or if we really want turn it off if all features that require analysis are off @@ +1560,3 @@ + highlighter = _gtk_source_highlighter_new (); + buffer->priv->highlighter = highlighter; + shouldn't this goes inside the if? does highlighter make sense if analyzer is null? In fact if always set the analyzer on the highlighter right after creating it I think it should be a constructor property passed in the constructor @@ +1568,3 @@ + buffer->priv->syntax_analyzer); + _gtk_source_highlighter_set_styles_map (buffer->priv->highlighter, + language->priv->styles); not sure I like this... maybe pass the whole lang to the highlighter. Also this and the style scheme look like things to pass directly in the constructor at first sight @@ +1619,3 @@ synchronous); + if (buffer->priv->highlighter != NULL && buffer->priv->highlight_syntax != FALSE) + _gtk_source_highlighter_ensure_highlight (buffer->priv->highlighter, start, end); did sync have any effect on the highlighting part?
Created attachment 164773 [details] [review] another try. Another try. This time the highlighter api is used internally by the contextengine. The gtksourcebuffer is not modified.
Review of attachment 164773 [details] [review]: Review comments below... I was expecting the patch to straightforward: along the lines of - in engine_init add the highlighter - in all the places where highlighting was done, call an highlighter method - move the called code verbatim in the highlighter There are instead many subtle changes that are harder to understand and that require more explanation ::: gtksourceview/gtksourcecontextengine-private.h @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8; coding: utf-8 -*- Not sure I like splitting this .h, especially called -private since the engine is all private by definition. Though I undestand that' the split is meant to be used by the highlighter... Does anyone else hav opinions on this? @@ +62,3 @@ + * Eventually, we may add a GInterface (implemented by Analyzers) + * so handlers can add custom data to the annotation. */ + Annotation *annot; I need to understand this annot business in more detail. I see there is a pointer here in each segment but also each real segment has a context and the context contains a _copy_ of the annotation (not a pointer). Can you explain in more detail and compare it to what we have now in terms of number of instances of this annotion struct, e.g. how many times the same info is duplicated etc. @@ +69,3 @@ + gchar *style; + GtkTextTag *style_tag; + guint style_inside; //??? this needs to be finished ::: gtksourceview/gtksourcecontextengine.c @@ +126,3 @@ #define TAG_CONTEXT_CLASS_NAME "GtkSourceViewTagContextClassName" +#define REAL_SUBPATTERN(subpat) ((RealSubPattern *)(subpat)) as I said before I am not really enthusiastic about this trick... I know Gtk does it with iters etc, but here we are dealing with internal objects and I'd rather just keep things simple... @@ +328,3 @@ + + /* This is NULL if and only if it's a dummy segment which denotes + * inserted or deleted text. */ what is "this"? In the old code this comment was for context @@ -695,3 +578,2 @@ * higher than highlighting tags created before */ gtk_text_tag_set_priority (new_tag, ce->priv->n_tags); - set_tag_style (ce, new_tag, style_id); why is this gone? (not saying that it wrong, but I would have expected that to each call removed a corresonding call to a method of the highlighter was added, so I'd like some explanation @@ -1265,5 +977,2 @@ return; - if (modify_refresh_region) - gtk_text_region_add (ce->priv->refresh_region, start, end); - why is this gone? (same as before) @@ -2281,3 +2009,3 @@ * GtkSourceEngine::update_highlight method. * - * Makes sure the area is analyzed and highlighted. If @asynchronous + * Makes sure the area is analyzed. If @synchronous the old comment seems more correct, we are still calling the highlighter @@ +2051,3 @@ + if (ce->priv->highlight_syntax) + { + _gtk_source_highlighter_ensure_highlight (ce->priv->highlighter, start, end); This seems to be changing the logic: before the highlighting was sync only under specific conditions, here you always call the highlighter and you do not tell me if it should be sync or not @@ +2241,3 @@ +buffer_notify_highlight_syntax_cb (GtkSourceContextEngine *ce) +{ + g_object_get (ce->priv->buffer, "highlight-syntax", &ce->priv->highlight_syntax, NULL); why is enable_highlight() gone? if this is toggled I would assume you still need the highlighter to (un)highlight things @@ -2569,5 +2267,2 @@ if (ce->priv->buffer != NULL) { - g_signal_handlers_disconnect_by_func (ce->priv->buffer, - (gpointer) buffer_notify_highlight_syntax_cb, - ce); why removed? you still have the callback @@ +2323,3 @@ * never happen, _gtk_source_context_data_finish_parse checks main context. */ g_assert (main_definition != NULL); + ce->priv->tags = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); why is this moved? @@ +2465,2 @@ g_type_class_add_private (object_class, sizeof (GtkSourceContextEnginePrivate)); + remove blank line @@ +2477,3 @@ +_gtk_source_context_engine_get_buffer (GtkSourceContextEngine *ce) +{ + if (ce) this seems weird, it is unusual for a method of an object to accept null. I think the check should be in the caller @@ +2481,3 @@ + else + return NULL; +} leave blank line @@ +2486,3 @@ +{ + if (ce) + return ce->priv->root_segment; same ::: gtksourceview/gtksourcehighlighter.c @@ +1,1 @@ +/* -*- mode: c; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8; coding: utf-8 -*- Do I need to review this in detail or is just the engine code moved? Are there logical changes? Apart from that I still see some cleanup needed (extra blank lines between function, commented off codelines etc) ::: tests/test-widget.c @@ +72,2 @@ gpointer user_data); static void margin_toggled_cb (GtkAction *action, If possible also sync test-widget.py @@ +136,3 @@ static GtkToggleActionEntry toggle_entries[] = { + { "SyntaxHighlight", NULL, "Synta_x Highlighting", NULL, + "Toggle syntax highlighting", "Enable syntax highlighting"
Review of attachment 164773 [details] [review]: ::: gtksourceview/gtksourcecontextengine-private.h @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8; coding: utf-8 -*- Well, the only purpose is to not expose internal structs of the engine to the highlighter and other handlers using the engine. @@ +62,3 @@ + * Eventually, we may add a GInterface (implemented by Analyzers) + * so handlers can add custom data to the annotation. */ + Annotation *annot; For each segment you only have one Annotation, which is the annotation struct in the Context struct. When creating a new segment, the annot pointer is point to the Annotation struct in the associated Context pointer. In this way, I do not copy extra information (the information is in the Context struct but packed differently) and the annotation is only allocated once for each Context! So, the only extra thing is the Annotation pointer in the Segment struct. @@ +69,3 @@ + gchar *style; + GtkTextTag *style_tag; + guint style_inside; //??? done. I forgot to delete the comments. sorry. ::: gtksourceview/gtksourcecontextengine.c @@ +126,3 @@ #define TAG_CONTEXT_CLASS_NAME "GtkSourceViewTagContextClassName" +#define REAL_SUBPATTERN(subpat) ((RealSubPattern *)(subpat)) I took the trick from gtk because jessevdk suggested I should hide the internals of the engine to the highlighter, so they would be REALLY split logically and not only separate the files. @@ +328,3 @@ + + /* This is NULL if and only if it's a dummy segment which denotes + * inserted or deleted text. */ comment deleted, thanks @@ -695,3 +578,2 @@ * higher than highlighting tags created before */ gtk_text_tag_set_priority (new_tag, ce->priv->n_tags); - set_tag_style (ce, new_tag, style_id); set_tag_style is a task of the highlighter. So the highlighter does this now either after catching the update-highlight signal or by the source_highlighter_ensure_highlight @@ -1265,5 +977,2 @@ return; - if (modify_refresh_region) - gtk_text_region_add (ce->priv->refresh_region, start, end); - refresh_region is moved to the highlighter. @@ -2281,3 +2009,3 @@ * GtkSourceEngine::update_highlight method. * - * Makes sure the area is analyzed and highlighted. If @asynchronous + * Makes sure the area is analyzed. If @synchronous Agreed. @@ +2051,3 @@ + if (ce->priv->highlight_syntax) + { + _gtk_source_highlighter_ensure_highlight (ce->priv->highlighter, start, end); agreed, corrected in new patch. @@ +2241,3 @@ +buffer_notify_highlight_syntax_cb (GtkSourceContextEngine *ce) +{ + g_object_get (ce->priv->buffer, "highlight-syntax", &ce->priv->highlight_syntax, NULL); this is also moved to the highlighter. @@ -2569,5 +2267,2 @@ if (ce->priv->buffer != NULL) { - g_signal_handlers_disconnect_by_func (ce->priv->buffer, - (gpointer) buffer_notify_highlight_syntax_cb, - ce); That's an error, added back. thanks. @@ +2465,2 @@ g_type_class_add_private (object_class, sizeof (GtkSourceContextEnginePrivate)); + done @@ +2477,3 @@ +_gtk_source_context_engine_get_buffer (GtkSourceContextEngine *ce) +{ + if (ce) Well. I forgot to delete these two new functions. There are not needed anymore since the engine is calling to the highlighter. ::: gtksourceview/gtksourcehighlighter.c @@ +1,1 @@ +/* -*- mode: c; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8; coding: utf-8 -*- Sorry, I did the code cleanup, Most of the functions are just verbatim copies from the functions in the engine. I will add more comments bellow. @@ +387,3 @@ + * it affects only text tags. + */ +static void this is changed from the engine version and needs review. Now I connect to the highlight-update signal to now when the engine has updated some syntax and rehighlight the corresponding region. ::: tests/test-widget.c @@ +72,2 @@ gpointer user_data); static void margin_toggled_cb (GtkAction *action, ok, working on it.
Created attachment 164851 [details] [review] Updated patch after review. Here is the updated patch, more clean, and correcting the issues found in the review.
Review of attachment 164773 [details] [review]: ::: gtksourceview/gtksourcecontextengine-private.h @@ +62,3 @@ + * Eventually, we may add a GInterface (implemented by Analyzers) + * so handlers can add custom data to the annotation. */ + Annotation *annot; I really really do not like this: for instance it means that if I assign segment->annot.foo = bar it will have side effects on all the segments with that context. I'd prefer to keep it simple without any subtle tricks, the code is complicated enough as is. Let's maybe discuss this on irc ::: gtksourceview/gtksourcecontextengine.c @@ +328,3 @@ + + /* This is NULL if and only if it's a dummy segment which denotes + * inserted or deleted text. */ Well, rather than delete it I would have preferred to put it back at its place. Or is the comment not valid anymore? If that's the case, why? @@ -695,3 +578,2 @@ * higher than highlighting tags created before */ gtk_text_tag_set_priority (new_tag, ce->priv->n_tags); - set_tag_style (ce, new_tag, style_id); Mmm... does this mean that is now done lazily? Beside maybe I am reading the diff wrong but it looks like that function is is called on a tag that is not for syntax hl. Also I would have naively expected that the whole business of creating the highlighting tags was moved to the highlighter @@ -1265,5 +977,2 @@ return; - if (modify_refresh_region) - gtk_text_region_add (ce->priv->refresh_region, start, end); - well, I can see that myself, but it does not answer my question. It is not replaced by calling an highlighter method here and in the highlighter I do not see any equivalent code that adds a region to be refreshed
Created attachment 165204 [details] [review] streamlined patch According to IRC conversations, I updated the patch so it's more streamlined.
Review of attachment 165204 [details] [review]: Hi, this is definitely nearer to the inclusion, but there are still some things that worry me. In particular the one about tag styles beeing applied by looping the hash. (I just woke up so I may have missed many things ;) ::: gtksourceview/gtksourcecontextengine-private.h @@ +29,3 @@ + GtkTextBuffer *buffer; + const GtkTextIter *start, *end; +}; This looks like an util struct that should be in the .c file @@ +78,3 @@ + gint end_at; + SubPattern *next; +}; blank line ::: gtksourceview/gtksourcecontextengine.c @@ +559,2 @@ * higher than highlighting tags created before */ gtk_text_tag_set_priority (new_tag, ce->priv->n_tags); here we remove the set_tag_style when creating a tag, but I do not see what takes its place @@ +826,3 @@ end_offset = MIN (end_offset, segment->end_at); + context_classes = get_context_classes (ce,segment->context); space after comma @@ +2329,3 @@ G_CALLBACK (buffer_notify_highlight_syntax_cb), ce); + _gtk_source_highlighter_attach_buffer (ce->priv->highlighter, Not related to this line, but I see this is the only place where this function is called. I was expecting to see attach_buffer (NULL) in some places to "free" the highlighter @@ +5595,2 @@ line_info_destroy (&line); Careful here: with this line removed how do things work? Near the end of this function refreh_range is called and takes care of highlighting the region, but if lines are not added to the region what is rehighlighted? ::: gtksourceview/gtksourcehighlighter.c @@ +71,3 @@ + + /* All tags indexed by style name: values are GSList's of tags, ref()'ed. */ + GHashTable *tags; memory management scares me a bit here. These are pointers to stuff internal to the engine. As discussed I think they should be just here long term and removed from the engine, but for now let's at least ref/unref the hash etc @@ +296,3 @@ + gtk_text_iter_get_offset (start), + gtk_text_iter_get_offset (&real_end)); + g_hash_table_foreach (highlighter->priv->tags, (GHFunc) set_tag_style_hash_cb, highlighter); I do not understand what is this loop and why it is here. At a first look it seems something very inefficient to do each time we highlight a region @@ +405,3 @@ + { + gtk_text_region_add (highlighter->priv->refresh_region, &start, &end); + // before I called refresh_range, which emits highlight_updated for the whole buffer. I see the comment but I do not understand how the problem is solved also no // comments
Review of attachment 165204 [details] [review]: Yeah, I just wanted to attach the patch so we have a simpler common base to discuss the issues the patch still has, all of which are pointed out in your review. More detailed comments bellow. ::: gtksourceview/gtksourcecontextengine-private.h @@ +29,3 @@ + GtkTextBuffer *buffer; + const GtkTextIter *start, *end; +}; It is here because it is needed as a UserData pointer for callbacks in the engine and the highlighter. Maybe I should have internal copies in both .c files? ::: gtksourceview/gtksourcecontextengine.c @@ +558,3 @@ /* It must have priority lower than user tags but still * higher than highlighting tags created before */ gtk_text_tag_set_priority (new_tag, ce->priv->n_tags); set_tag_style belongs to the Highlighter. I believe that setting the style here is redundant, since the highlight methods will ALWAYS unapply the styles of the region to be highlighted and Then reapply the styles, and this is done in the Highlighter.(I don't like the way the highlighter does the highlighting, but I kept the implementation without changes from the engine, so improvements can be discussed later) @@ +5595,2 @@ line_info_destroy (&line); refresh-range emits the highlight-updated signal. the highlighter listens for that signal and do the highlighting in that region (in the callback)... this is one of the issues that need more discussion. ::: gtksourceview/gtksourcehighlighter.c @@ +71,3 @@ + + /* All tags indexed by style name: values are GSList's of tags, ref()'ed. */ + GHashTable *tags; I agree, I will add refs/unrefs. @@ +296,3 @@ + gtk_text_iter_get_offset (start), + gtk_text_iter_get_offset (&real_end)); + g_hash_table_foreach (highlighter->priv->tags, (GHFunc) set_tag_style_hash_cb, highlighter); mmm. you are right. This loops replaces the set_tag_style removed in the engine, so this is wrong because it is not done on tag creation but everytime I highlight. Note that with annotations and all the tag creation stuff in the highlighter this will go away, but I will look into a way of solving this for this patch. @@ +405,3 @@ + { + gtk_text_region_add (highlighter->priv->refresh_region, &start, &end); + // before I called refresh_range, which emits highlight_updated for the whole buffer. this is not solved, I should emit a signal here, but then I would like to add a new signal (maybe internal to the engine, or on the buffer) syntax-updated, so the highlighter hooks up to this signal instead, this way I could just emit the signal here the comment // is just the way I use to recall me that I need to work on that before this is merged.
Review of attachment 165204 [details] [review]: ::: gtksourceview/gtksourcecontextengine-private.h @@ +29,3 @@ + GtkTextBuffer *buffer; + const GtkTextIter *start, *end; +}; yes, better have these in the c files ::: gtksourceview/gtksourcecontextengine.c @@ +558,3 @@ /* It must have priority lower than user tags but still * higher than highlighting tags created before */ gtk_text_tag_set_priority (new_tag, ce->priv->n_tags); if it is really not needed let's merge a preliminary patch that removes it, so that we can track regressions more easily @@ +5595,2 @@ line_info_destroy (&line); yes the signal handler does the actual highlighting, but the region must be populated by the engine before emitting the signal. I think we should have a method like _highlighter_queue_reqion (or a better name if you can think of one :) ::: gtksourceview/gtksourcehighlighter.c @@ +405,3 @@ + { + gtk_text_region_add (highlighter->priv->refresh_region, &start, &end); + // before I called refresh_range, which emits highlight_updated for the whole buffer. As discussed on irc I think the engine should call an explicit method on the highlighter to trigger the refresh and then the highlighter should take care of emitting the signal on the buffer
Review of attachment 165204 [details] [review]: All comments addressed in a new patch ::: gtksourceview/gtksourcecontextengine-private.h @@ +29,3 @@ + GtkTextBuffer *buffer; + const GtkTextIter *start, *end; +}; moved! @@ +79,3 @@ + SubPattern *next; +}; +/* Context methods */ done! ::: gtksourceview/gtksourcecontextengine.c @@ +558,3 @@ /* It must have priority lower than user tags but still * higher than highlighting tags created before */ gtk_text_tag_set_priority (new_tag, ce->priv->n_tags); In fact, the approach of this patch is wrong. I moved create_tag to the highlighter and added the set_tag_style line back @@ +826,3 @@ end_offset = MIN (end_offset, segment->end_at); + context_classes = get_context_classes (ce,segment->context); done! @@ +2329,3 @@ G_CALLBACK (buffer_notify_highlight_syntax_cb), ce); + _gtk_source_highlighter_attach_buffer (ce->priv->highlighter, mmm, now it's called alwo with null @@ +5595,2 @@ line_info_destroy (&line); I readded the line ::: gtksourceview/gtksourcehighlighter.c @@ +71,3 @@ + + /* All tags indexed by style name: values are GSList's of tags, ref()'ed. */ + GHashTable *tags; moved to the highlighter @@ +296,3 @@ + gtk_text_iter_get_offset (start), + gtk_text_iter_get_offset (&real_end)); + g_hash_table_foreach (highlighter->priv->tags, (GHFunc) set_tag_style_hash_cb, highlighter); the foreach is now gone.
Created attachment 171114 [details] [review] Latest patch New patch! Please review.
Review of attachment 171114 [details] [review]: ::: gtksourceview/Makefile.am @@ +86,3 @@ gtksourceview-i18n.c \ gtksourceview-utils.c \ + gtksourcehighlighter.c \ alphabetic order ::: gtksourceview/gtksourcecontextengine-private.h @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8; coding: utf-8 -*- We probably already discussed this and I do not recall, but why do we need a separate header, contextengine.h is already private @@ +80,3 @@ +GtkTextTag * _gtk_source_context_engine_get_context_tag (GtkSourceContextEngine *ce, + Context *context); +Segment * _gtk_source_context_engine_get_tree (GtkSourceContextEngine *ce); not used in the highlighter as far as I can see, why is it here? @@ +82,3 @@ +Segment * _gtk_source_context_engine_get_tree (GtkSourceContextEngine *ce); + +GHashTable * _gtk_source_context_engine_get_style_tags (GtkSourceContextEngine *ce); ditto @@ +83,3 @@ + +GHashTable * _gtk_source_context_engine_get_style_tags (GtkSourceContextEngine *ce); +GtkTextBuffer * _gtk_source_context_engine_get_buffer (GtkSourceContextEngine *ce); ditto ::: gtksourceview/gtksourcecontextengine.c @@ +794,3 @@ + const GtkTextIter *start, *end; +}; + it is probably used below, but from the diff this looks unused, just double check it is not a leftover @@ -5942,3 +5484,2 @@ line_info_destroy (&line); - gtk_text_region_add (ce->priv->refresh_region, &line_start, &line_end); from the context I do not understand why this is removed and not substituted with something with the same side effects ::: gtksourceview/gtksourcecontextengine.h @@ +84,3 @@ +GtkSourceContextEngine * + _gtk_source_context_engine_new (GtkSourceContextData *data); If we are reindenting this, let's do it right and align the "(" properly ::: gtksourceview/gtksourcehighlighter.c @@ +57,3 @@ +#endif + +G_DEFINE_TYPE (GtkSourceHighlighter, _gtk_source_highlighter, G_TYPE_OBJECT) why the underscore here? I do not think it is needed or useful @@ +128,3 @@ + + if (gtk_text_iter_equal (start, end)) + return; I know it was already like that in the old code, but now that I see it I'd prefer this function written like: if (!iter_equal) { BufAndIters data; data.... foreach() } @@ +232,3 @@ + else + { /* FIXME: We should cache the start_at and end_at so we only apply the tag + where is needed, instead of erasing all the highlighting */ I am not sure I understand this fixme comment you added @@ +425,3 @@ +} + +/** name in the doc comment is wrong @@ +441,3 @@ +_gtk_source_highlighter_ensure_highlight (GtkSourceHighlighter *highlighter, + const GtkTextIter *start, + const GtkTextIter *end) The alignment of args is wrong after the function rename (it is also true for other functions in the file) @@ +460,3 @@ + while (!gtk_text_region_iterator_is_end (®_iter)) + { + GtkTextIter s, e; always leave a blank line after var declaration @@ +501,3 @@ + g_hash_table_foreach (highlighter->priv->tags, (GHFunc) set_tag_style_hash_cb, highlighter); + + // before I called refresh_range, which emits highlight_updated for the whole buffer. Can you explain a bit better the change here? The old call to refresh_range had different side effects (workaround for \r\n, emitting a signal, etc) I also do not particularly like that it walks the hash table manually instead of calling a common util aslo, no c++ comments please @@ +551,3 @@ + + + Just one blank line @@ +566,3 @@ + Segment *root_segment) +{ + g_return_if_fail (GTK_IS_SOURCE_HIGHLIGHTER (highlighter)); check buffer is a buffer or NULL @@ +672,3 @@ + + if (highlighter->priv->style_scheme != NULL) + g_object_unref (highlighter->priv->style_scheme); I usually prefer to unref in dispose ::: gtksourceview/gtksourcehighlighter.h @@ +4,3 @@ + * Copyright (C) 2003 - Gustavo Giráldez + * Copyright (C) 2005 - Marco Barisione, Emanuele Aina + * your copyright is missing ::: gtksourceview/gtksourceview.c @@ +60,1 @@ please remove this change
Review of attachment 171114 [details] [review]: All comments except one addressed. After you answer my questions I will update my patch with all comments addressed. ::: gtksourceview/Makefile.am @@ +86,3 @@ gtksourceview-i18n.c \ gtksourceview-utils.c \ + gtksourcehighlighter.c \ done ::: gtksourceview/gtksourcecontextengine-private.h @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8; coding: utf-8 -*- well. the contextengine.h is included in gtksourceview. So the idea of the private header is to have info only available to helpers (highlighter, fold engine, etc), but appart from that, there is not other reason. @@ +80,3 @@ +GtkTextTag * _gtk_source_context_engine_get_context_tag (GtkSourceContextEngine *ce, + Context *context); +Segment * _gtk_source_context_engine_get_tree (GtkSourceContextEngine *ce); was a left-over of previous attempts. Erased @@ +82,3 @@ +Segment * _gtk_source_context_engine_get_tree (GtkSourceContextEngine *ce); + +GHashTable * _gtk_source_context_engine_get_style_tags (GtkSourceContextEngine *ce); also erased @@ +83,3 @@ + +GHashTable * _gtk_source_context_engine_get_style_tags (GtkSourceContextEngine *ce); +GtkTextBuffer * _gtk_source_context_engine_get_buffer (GtkSourceContextEngine *ce); also erased ::: gtksourceview/gtksourcecontextengine.c @@ +794,3 @@ + const GtkTextIter *start, *end; +}; + it is indeed used. @@ +902,3 @@ + _gtk_source_highlighter_invalidate_region (ce->priv->highlighter, + start, &real_end); + this call replaces the text_region_add below. @@ -5942,3 +5484,2 @@ line_info_destroy (&line); - gtk_text_region_add (ce->priv->refresh_region, &line_start, &line_end); Now, refresh_range takes care of that. The text_region add was called each time for each line being analyzed, now is called only once. This is indeed equivalent since the while traverse the hole interval begin analyzed. ::: gtksourceview/gtksourcecontextengine.h @@ +84,3 @@ +GtkSourceContextEngine * + _gtk_source_context_engine_new (GtkSourceContextData *data); done ::: gtksourceview/gtksourcehighlighter.c @@ +57,3 @@ +#endif + +G_DEFINE_TYPE (GtkSourceHighlighter, _gtk_source_highlighter, G_TYPE_OBJECT) It is just newbie things... What should I change besides that If I remove the _? @@ +128,3 @@ + + if (gtk_text_iter_equal (start, end)) + return; done @@ +232,3 @@ + else + { /* FIXME: We should cache the start_at and end_at so we only apply the tag + where is needed, instead of erasing all the highlighting */ no worries, deleted @@ +425,3 @@ +} + +/** corrected @@ +441,3 @@ +_gtk_source_highlighter_ensure_highlight (GtkSourceHighlighter *highlighter, + const GtkTextIter *start, + const GtkTextIter *end) corrected for all functions @@ +460,3 @@ + while (!gtk_text_region_iterator_is_end (®_iter)) + { + GtkTextIter s, e; ups @@ +551,3 @@ + + + done! @@ +566,3 @@ + Segment *root_segment) +{ + g_return_if_fail (GTK_IS_SOURCE_HIGHLIGHTER (highlighter)); Don't understand what should I do if the check fails @@ +672,3 @@ + + if (highlighter->priv->style_scheme != NULL) + g_object_unref (highlighter->priv->style_scheme); so should I add a dispose method and do just that? What about calling attach_buffer with NULL when disposing or finalizing? ::: gtksourceview/gtksourcehighlighter.h @@ +4,3 @@ + * Copyright (C) 2003 - Gustavo Giráldez + * Copyright (C) 2005 - Marco Barisione, Emanuele Aina + * added. ::: gtksourceview/gtksourceview.c @@ +60,1 @@ done
Review of attachment 171114 [details] [review]: ::: gtksourceview/gtksourcecontextengine-private.h @@ +1,1 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8; coding: utf-8 -*- I'd say to just put them in context.h with appropriate comments ::: gtksourceview/gtksourcecontextengine.c @@ -5942,3 +5484,2 @@ line_info_destroy (&line); - gtk_text_region_add (ce->priv->refresh_region, &line_start, &line_end); But that is not equivalent: in the loop that adds line by line, you can break out of the loop in different cases, e.g. when the timer elapses. In that case in the old code just the analyzed lines were added to the region, while in your code all the interval is always added ::: gtksourceview/gtksourcehighlighter.c @@ +57,3 @@ +#endif + +G_DEFINE_TYPE (GtkSourceHighlighter, _gtk_source_highlighter, G_TYPE_OBJECT) well, look at how the macro it is expanded. I do not think you need to change much @@ +566,3 @@ + Segment *root_segment) +{ + g_return_if_fail (GTK_IS_SOURCE_HIGHLIGHTER (highlighter)); I just meant sanity check the args: g_return_if_fail (GTK_IS_TEXT_BUFFER (buffer) || buffer == NULL) @@ +672,3 @@ + + if (highlighter->priv->style_scheme != NULL) + g_object_unref (highlighter->priv->style_scheme); yes, my general rule of thumb is that all "unref" must be done in the dispose method, all "free" in finalize. Also setting the buffer to null sounds good
I pushed an updated version of the patch on the highlighter-split branch of git.gnome.org. The patch is based on the "splitv4" branch of github by jaliste. I also pushed an additional branch that tries to further decouple things. TODO: - check that the comments of the previous reviews are addressed, since I have not checked what happened between the last patch here and the last patch on github - I do not like very much to share so many data structures between engine and highlighter... ideas? Should we start splitting off proper objects with getters and setters? - Once we sqash and commit this work, all involved people should be credited (I think gregier worked on it at some point)
Paolo, thanks for pushing this. I will look up and try to rebase all my other in-progress patches and think more about sharing the objects. My idea was to hide almost all the structure of the context tree and just add the ability for the highlighter to queue some operations to be performed by the analyzer when reanalyzing the tree, in order to recompute some of the info needed by the highlighter, more or less what is in the first patches in this bug.
This seems obsolete at this point. If there is anything you'd still like to see land upstream with this, please create an issue/merge-request on gitlab.gnome.org/GNOME/gtksourceview and I'd be happy to help and review.