GNOME Bugzilla – Bug 134610
Code folding
Last modified: 2016-12-07 11:56:18 UTC
Enhancment wish - Code folding with strategies
Created attachment 51171 [details] [review] code folding implementation This is a first implementation of code folding. Put "test.c" in the tests directory. The test-widget.c program adds folds for the test.c file. Please comment on the patch. Things left to do: - Fix markers again (by drawing them in the line number gutter) - Show tooltips for collapsed folds when hovering over the collapsed arrow - Draw vertical line over an expanded arrow to highlight the fold - Experiment with adding a GtkLabel widget to indicate a fold ([..]) using gtk_text_view_add_child_in_window
Created attachment 51172 [details] test.c
Just to be clear, this will only work if you have gtk+-2.8.0 or higher.
Comment on attachment 51171 [details] [review] code folding implementation Since the patch is large I will review it in multiple steps so that you can have my first comments without waiting too much time. >+ /* Create invisibility tag for folding lines. */ >+ gtk_text_buffer_create_tag (GTK_TEXT_BUFFER (source_buffer), >+ "invisibleline", "invisible", TRUE, NULL); >+ Please, use macros when appropriated (in this case for the tag name) For example: #define INVISIBLE_LINE "GtkSourceBuffer:InvisibleLine" Lemme start with the new API. >+struct _GtkSourceFold >+{ >+ GtkTextMark *start_line; >+ GtkTextMark *end_line; >+ GSList *children; >+ gboolean folded; >+ gboolean prelighted; >+ gboolean animated; >+ GtkExpanderStyle expander_style; >+}; >+ IMO, GtkSourceFold should be an opaque object with some accessor functions. >+/* fold methods. */ >+const GtkSourceFold *gtk_source_buffer_add_fold (GtkSourceBuffer *buffer, >+ const GtkTextIter *begin, >+ const GtkTextIter *end); Why do you return a const object? >+void gtk_source_buffer_remove_fold (GtkSourceBuffer *buffer, >+ const GtkSourceFold *fold); Why is fold const? >+ >+gboolean gtk_source_buffer_get_folded (GtkSourceBuffer *buffer, >+ const GtkTextIter *iter); This function seems wrong to me. >+void gtk_source_buffer_set_folded (GtkSourceBuffer *buffer, >+ GtkSourceFold *fold, >+ gboolean folded); >+ I think we should have something like gtk_source_fold_[get|set]_folded (GtkSourceFold *fold, ...); gtk_source_fold_get_bounds (GtkSourceFold *fold, GtkTextIter *start, (or "GtkTextMark *" or "int *") GtkTextIter *end); we also need a gtk_source_fold_get_children function. >+GSList *gtk_source_buffer_get_folds_in_region (GtkSourceBuffer *buffer, >+ const GtkTextIter *begin, >+ const GtkTextIter *end); I don't like this function very much. I'd prefer to have a function that returns the root folds. All the other folds can be obtained recursively. If you need to use this functions in the view, you can move the implementation there. >+ >+GtkSourceFold *gtk_source_buffer_get_fold_at_line (GtkSourceBuffer *buffer, >+ gint line); >+GtkSourceFold *gtk_source_buffer_get_fold_at_iter (GtkSourceBuffer *buffer, >+ const GtkTextIter *iter); Why do we need these functions? And why do we need both? Do they return the leaf fold? >+const GSList *gtk_source_buffer_get_all_folds (GtkSourceBuffer *buffer); We don't need it. We can use get_folds_in_region with being and end set to NULL.
Created attachment 51192 [details] IRC log with comments about the new API and more
Created attachment 51321 [details] [review] patch with the API comments Attached patch incorporates API changes from the review + irc discussion. Patch also fixes a bug where if you hide the fold gutter, the folds weren't being expanded.
Note: now that there is a gtksourcebuffer-private.h, can we move _gtk_source_buffer_highlight_region to it without breaking API/ABI?
On 8/25/05, Paolo Maggi <paolo.maggi@polito.it> wrote: > paolo jeroen: What I was thinking was creating gtksourcefold.[ch] and > gtksourcefold-private.h file. > paolo so to have functions like: > paolo gboolean gtk_source_fold_get_folded (GtkSourceFold *fold); > paolo void gtk_source_fold_set_folded (GtkSourceFold *fold, gboolean > folded); > paolo you don't need the buffer argument OK, so i've thought about this some more and looked at the current code: the current code really needs the GtkSourceBuffer. While get_folded simply returns the fold->folded field, set_folded does all sorts of things on the buffer. It is not smart to move these methods to gtksourcefold.[ch] and remove the GtkSourceBuffer argument from their parameters. It adds more code and doesn't improve the readability. You're not doing an operation on the GtkSourceFold itself, you're doing it on the GtkSourceBuffer. GtkSourceFold is really only a location to keep some information about the fold around. Splitting up the methods so that the convenient methods (get_folded & get_bounds) are in a separate file while the rest is in gtksourcebuffer doesn't make sense. > Other comments on the implementation. > > +struct _GtkSourceFold > +{ > + GtkTextMark *start_line; > + GtkTextMark *end_line; > + GSList *children; > + gboolean folded; > + gboolean prelighted; > + gboolean animated; > + GtkExpanderStyle expander_style; > +}; > > Please, add some comments explaining the meaning of "prelighted" and > "animated". Done. > +static gboolean > +insert_child_fold (GtkSourceBuffer *buffer, > + GtkSourceFold *child, > + GtkSourceFold *parent) > +{ > + GtkTextIter begin, end, pbegin, pend, iter; > + GSList *folds; > + GtkSourceFold *child_fold; > > Move the variable declarations in the block the variables are used. Done. > + > + /* check if the child fold is within the parent fold. */ > + if (gtk_text_iter_compare (&pbegin, &begin) == -1 && > + gtk_text_iter_compare (&pend, &end) == 1) > > You should check lines here instead of iters. No, you're opening a real can of worms if you're checking line numbers. I really need to look at how other editors (like Eclipse) deal with multiple folds on a single line. I'll come back on this when i've done that. > > + } > + else if (gtk_text_iter_in_range (&pbegin, &begin, &end) || > + gtk_text_iter_in_range (&pend, &begin, &end)) > + { > + g_warning ("Cannot add child fold: fold overlapses with an existing > fold."); > > It may be the warning should be a bit more explicative. Probably it > would be nice to print the bounds of the folds. Not, and should probably be an g_error (?). IIRC GdlDock returns criticals if you're trying to do something which isn't legal. > + } > + > + return FALSE; > > Shouldn't we print a warning all times FALSE is returned? When an illegal fold operation is detected (intersecting folds), a message is already printed in insert_child_fold (see your last comment). What i will change is to return NULL in gtk_source_buffer_add_fold when the fold couldn't be inserted. > paolo it has two subfold [2,3] and [5,6] > paolo you want to add a new subfold [1,4] note that it should report and > error > paolo while it seems to me it adds the fold without problems I'll add some logic so that it reparents [2,3] to [1,4] and insert [1,4] as a child to [0,10]. That's the proper solution. > +GtkSourceFold * > +gtk_source_buffer_add_fold (GtkSourceBuffer *buffer, > + const GtkTextIter *begin, > + const GtkTextIter *end) > +{ > > [snip] > > + > + /* try adding the child to all folds in the region. */ > + if (insert_child_fold (buffer, fold, parent)) > + break; > + > > If insert_child_fold always return FALSE you are going to check for all > the folds. > Since the folds are sorted, the algo could stop before testing for all > the folds. Typo here. s/parent/child_node/ fixes this. Done. I'll make a new patch either tomorrow or on saturday.
Note that GtkSourceFold already has a reference to the buffer (in the text marks) so you don't need to have a "buffer" argument in the set_folded function. In any case you should check the fold is valid (i.e. it is associated to the buffer you are using).
Created attachment 51544 [details] [review] new patch Patch adds gtksourcefold.[ch] and gtksourcefold-private.h files.
Created attachment 51550 [details] [review] latest patch Patch fixes gtk_source_view_get_lines (unnecessary complex). Also implements proper reparenting. Added test-fold.c program specifically for testing this.
Comment on attachment 51550 [details] [review] latest patch The API looks good now. I have only two comments: >+ /* Needed for sanity check since GtkSourceFold is a boxed type. */ >+ GtkSourceBuffer *buffer; Do we really need it? Why cannot we get it from one of the two text marks? Probably we need to add a gtk_source_fold_get_buffer function. I will review the implementation later.
I have tested the last patch a bit I have seen two re-drawing problems: 1. Launch ./test-fold, click with the mouse on line 8; the last folds are not redrawn 2. Mouse the mouse on an arrow and using the mouse wheel scroll down. From a cosmetic point of view I think having a bit of space between the arrow and the line will look better (like you showed me on http://www.tweakers.net/ext/i.dsp/1124804880.png ). Probably you should draw the line also when: - you move the mouse in the gutter (and you are inside a fold) - the cursor is inside a fold So you don't need to move the mouse over the arrow to see the bounds of a fold and you always know the bounds of the current fold.
Here some comment on gtksourcefold.[ch] and gtksourcefold-private.h. gtksourceview/gtksourcefold-private.h. -------------------------------------- To save a bit of memory you can move the gboolean fields at the end of the struct and declare it gint with :1, e.g. + gint folded : 1; Are you sure we really need the "buffer" field? gtksourceview/gtksourcefold.h ----------------------------- We probably need a gtk_source_fold_get_buffer function. It seems there is a little formatting problem with the declaration of the gtk_source_fold_get_bounds gtksourceview/gtksourcefold.c ----------------------------- Please, read the HACKING file of gtksourceview for some info on coding style. +void +gtk_source_fold_free (GtkSourceFold *fold) +{ + if (!fold) + return; + + /* FIXME: add a check that we're not freeing a fold that is in the buffer. */ It would be cool to add such test but only if it does not introduce big performance problems. Note that this function is only going to be used by signals and language bindings so I don't think it is worth introducing performance problems since I think we are quite sure the "canonical" users will use it only on fold that are not in the buffer... but as always it may be I'm too optimistic :) +GtkSourceFold * +gtk_source_fold_copy (const GtkSourceFold *fold) +{ I don't like how you have implemented this function. It has both performance and "maintainability" problems. Please, simply use "*copy = *fold" and then overwrite the children field and call g_object_ref for the two text marks. + +void +gtk_source_fold_set_folded (GtkSourceFold *fold, + gboolean folded) +{ + GtkTextBuffer *buffer; + GtkTextIter begin, end, insert; Move the declaration of "insert" in the proper block. + + if (!fold->animated) + fold->expander_style = GTK_EXPANDER_COLLAPSED; + [ snip ] + if (!fold->animated) + fold->expander_style = GTK_EXPANDER_EXPANDED; + } +} It is not clear to me why you are setting the expander_style here... isn't it managed by the view? Please, add documentation for the public function (but I guess you already know it)
Index: gtksourceview/gtksourcebuffer.c =================================================================== GtkSourceUndoManager *undo_manager; + + GSList *folds; }; For the reasons I will mention later, I think we should use a GList here. + +GtkSourceFold * +gtk_source_buffer_add_fold (GtkSourceBuffer *buffer, + const GtkTextIter *begin, + const GtkTextIter *end) +{ + GSList *folds; + GtkSourceFold *fold, *parent; + GtkTextIter iter; + + g_return_val_if_fail (GTK_IS_SOURCE_BUFFER (buffer), NULL); + g_return_val_if_fail (begin != NULL, NULL); + g_return_val_if_fail (end != NULL, NULL); + + g_message ("add fold @ %d, %d", gtk_text_iter_get_line (begin), gtk_text_iter_get_line (end)); + + fold = g_new0 (GtkSourceFold, 1); + fold->buffer = buffer; + fold->parent = NULL; + fold->children = NULL; + fold->folded = FALSE; + fold->prelighted = FALSE; + fold->animated = FALSE; + fold->expander_style = GTK_EXPANDER_EXPANDED; Maybe we could create a private _gtk_source_fold_new () function (I think it will impove maintainability) + if (gtk_text_iter_compare (&iter, end) == 1) + { + g_message ("adding fold before root %d", gtk_text_iter_get_line (&iter)); + buffer->priv->folds = g_slist_insert_before (buffer->priv->folds, + folds, fold); This is not a very efficient way to insert an element in a slist. I see two possible solutions: 1. Use a GList instead of a GSList 2. Use remove_link and concat (you need to store a pointer to the previous element) + break; + } + + /* try adding the child to all folds in the region. */ + if (insert_child_fold (buffer, fold, parent)) + break; + + folds = g_slist_next (folds); + } + + /* add the fold at the end of the list. */ + if (folds == NULL) { + g_message ("adding fold at end of root"); + buffer->priv->folds = g_slist_append (buffer->priv->folds, fold); Note you have just traversed the entire list, using g_slist_append you are going to traverse it again. I think we could store a pointer to the last item of the list and use it when appending the item. + } + + return fold; +} + +static void +foreach_fold_region (gpointer data, gpointer user_data) +{ + gtk_source_buffer_remove_fold (GTK_SOURCE_BUFFER (user_data), data); +} + +void +gtk_source_buffer_remove_fold (GtkSourceBuffer *buffer, + GtkSourceFold *fold) +{ + g_return_if_fail (GTK_IS_SOURCE_BUFFER (buffer)); + g_return_if_fail (fold != NULL); + g_return_if_fail (g_slist_index (buffer->priv->folds, fold) != -1); + + if (fold->folded) { + /* FIXME */ You probably have to use an internal remove_fold function since you only want to remove the invisible tag for the most external fold. + } + + gtk_text_buffer_delete_mark (GTK_TEXT_BUFFER (buffer), fold->start_line); + gtk_text_buffer_delete_mark (GTK_TEXT_BUFFER (buffer), fold->end_line); + + buffer->priv->folds = g_slist_remove (buffer->priv->folds, fold); + In this function you traverse the list buffer->priv->folds two times (using g_slist_index and g_slist_remove) -> we really need a GList. I suggest to use a something like: l = g_list_find (buffer->priv->folds, fold); g_return_if_fail (l != NULL); buffer->priv->folds = g_list_delete_link (buffer->priv->folds, l); In this way (but only if we use a GList) we traverse the list only once. I think it is worth optimizing [add|remove]_fold functions since they will be called very frequently while editing. + + g_slist_foreach (buffer->priv->folds, foreach_fold_region, buffer); It seems wrong to me. You probably want to run foreach on fold->children. +static void +get_folds_in_region (GtkTextBuffer *buffer, + const GtkTextIter *begin, + const GtkTextIter *end, + GtkSourceFold *fold, + GSList **list) +{ [snip] + /* the entire fold lies in the region, so add the fold + children. */ + else if (gtk_text_iter_compare (&fbegin, begin) >= 0 && + gtk_text_iter_compare (&fend, end) <= 0) + { + *list = g_slist_append (*list, fold); + children = fold->children; + while (children != NULL) { + get_folds_in_region (buffer, begin, end, children->data, list); + children = g_slist_next (children); + } + } + /* start iter is in the region, so add the fold first. */ + else if (gtk_text_iter_compare (&fbegin, begin) >= 0 && + gtk_text_iter_compare (&fbegin, end) <= 0) + { + *list = g_slist_append (*list, fold); It seems wrong to me. Suppose you have region [5, 9] and fold [7, 10]. You don't want to add the fold in the region but eventually some of its children. + children = fold->children; + while (children != NULL) { + get_folds_in_region (buffer, begin, end, children->data, list); + children = g_slist_next (children); + } + } Note that you have two branches with the same body. It seems to me you have missed the case only the end of the fold is inside the region (for example when the fold is [5, 9] and the region is [6, 12]). +} +GSList * +_gtk_source_buffer_get_folds_in_region (GtkSourceBuffer *buffer, + const GtkTextIter *begin, + const GtkTextIter *end) +{ [snip] + while (folds != NULL) + { + fold = folds->data; + + get_folds_in_region (GTK_TEXT_BUFFER (buffer), begin, end, fold, &result); + Since the folds are sorted you don't need to traverse the entire list. + folds = g_slist_next (folds); + } + + return result; +} + +static GtkSourceFold * +find_fold_at_line (GtkTextBuffer *buffer, + GSList *folds, + gint line) +{ + GtkSourceFold *fold; + GtkTextIter iter; + gint start_line, end_line; + + while (folds != NULL) + { + fold = folds->data; + + gtk_text_buffer_get_iter_at_mark (buffer, &iter, fold->start_line); + start_line = gtk_text_iter_get_line (&iter); + gtk_text_buffer_get_iter_at_mark (buffer, &iter, fold->end_line); + end_line = gtk_text_iter_get_line (&iter); + + if (line >= start_line && line <= end_line) { + GtkSourceFold *child; + + if (!fold->children) + return fold; + + child = find_fold_at_line (buffer, fold->children, line); + if (child) + return child; + else + return fold; + } + If line < start_line you can return NULL without traversing the entire list. Right? + +static GtkSourceFold * +find_fold_at_iter (GtkTextBuffer *buffer, + GSList *folds, + const GtkTextIter *iter) +{ + GtkSourceFold *fold; + GtkTextIter start_iter, end_iter; + + while (folds != NULL) + { Since the folds are sorted you don't need to traverse the entire list. +static gboolean +insert_child_fold (GtkSourceBuffer *buffer, + GtkSourceFold *child, + GtkSourceFold *parent) Should I really review this function? Sigh :( Not today, please! I have only read it very fast... what I can say for the moment is that most of the comments I have written about the gtk_source_buffer_add_fold function hold here too.
I've implemented the changes as a result of your review. The only thing i've not changes is the behavior of the get_folds_in_region: the method is designed to return the folds which *start* in the specified region. So a fold [7,10] in region [5,9] isn't returned. Any children of that fold that do start in [5,9] is returned. The reason behind this is that you only want to know which folds start in the region so we can draw the expander arrows. The GtkSourceBuffer field in the GtkSourceFold struct is still there for ease-of-use: frequently, we need the GtkSourceBuffer for fold operations. It's easier to just put the buffer in the struct than calling gtk_text_mark_get_buffer every time.
Ehh, example typo. Fold [5,10] in region [7,9].
In order to avoid multiple interaction on the review of insert_child_fold (the only function on the buffer side I still have to review) I'm going to write on bugzilla the test cases I'm going to use to review it so that you can be already sure it passes all of them Tests cases for add fold. Test case 1. ------------ * Initial state: You have a document with 15 lines, there is a single root fold [5, 10] * Actions: Add a fold [6, 8] * Final state: [6, 8] is a child of [5, 9] Test case 2. ------------ * Initial state: You have a document with 15 lines, there is a single root fold [5, 10] * Actions: Add two folds [6, 7] and [8,9] * Final state: [6, 7] and [8,9] are children of [5, 10] Test case 3. ------------ * Initial state: You have a document with 15 lines, there is a single root fold [5, 10] * Actions: Add a fold [4, 6] * Final state: [4, 6] is not inserted Test case 4. ------------ * Initial state: You have a document with 15 lines, there is a single root fold [5, 10] * Actions: Add a fold [5, 6] * Final state: [5, 6] is not inserted (I'm not sure) Test case 5. ------------ * Initial state: You have a document with 15 lines, there is a single root fold [5, 10] * Actions: Add a fold [7, 11] * Final state: [7, 11] is not inserted Test case 6. ------------ * Initial state: You have a document with 15 lines, there is a single root fold [5, 10] * Actions: Add a fold [7, 10] * Final state: [7, 10] is not inserted (I'm not sure) Test case 7. ------------ * Initial state: You have a document with 15 lines, there is a single root fold [5, 10] * Actions: Add a fold [3, 12] * Final state: [3, 12] is a root fold and [5, 9] is reparent and it is not a child of [3, 12]. Test case 8. ------------ * Repeat tests 1-6 using the final state of test 7 as initial state. Test case 9. ------------ * Repeat tests 1-8 using the following initial state: You have a document with 15 lines, there are two root folds [0, 2], [5, 10] Test case 10. ------------- * Repeat tests 1-8 using the following initial state: You have a document with 15 lines, there are two root folds [5, 10], [13, 14] Test case 11. ------------- * Repeat tests 1-8 using the following initial state: You have a document with 15 lines, there are 3 root folds [0, 2], [5, 10], [13, 14]
Created attachment 52642 [details] [review] new patch Patch includes a new test-fold program incorporating all the testcases specified in an earlier comment. Patch also readds working markers, though the visual implementation is still up for discussion.
See http://www.xs4all.nl/~jeroen/screenshots/Screenshot-GtkSourceView-Markers.png for an intial implementation of the markers + line numbers + code folding. Comments?
Sorry for the very late reply. Here some comment on gtksourcefold.[ch] and gtksourcefold-private.h. gtksourceview/gtksourcefold-private.h -------------------------------------- As I already said I'd prefer to remove the "buffer" field if it does not create noticeable performance problems. We would save 4 (or 8) bytes for each fold. Since in your patch you directly access fold->buffer only 3-4 times I think it is worth removing it from the struct. Mixing document and view fields in the same struct we will broke a bit the multiple view support... but anyway it will not work as I'd like due to the big limitation of gtktextbuffer/view. gtksourceview/gtksourcefold.h ----------------------------- It seems there is a little formatting problem with the declaration of gtk_source_fold_get_buffer gtksourceview/gtksourcefold.c ----------------------------- +void +gtk_source_fold_free (GtkSourceFold *fold) +{ [snip] + if (fold->children) + { + g_list_free (fold->children); + fold->children = NULL; + } You don't need to set fold->children to NULL. +static void +reapply_invisibleline_tag (GtkTextBuffer *buffer, + GList *folds) +{ + GtkSourceFold *fold; + GtkTextIter begin, end; Please, move the declarations in their proper block. Please, when you have applied the requested changes, attach a patch with only these three files and commit it to CVS (in an ad-hoc branch that we will merge to CVS HEAD as soon as the patch will be completely committed). We need to start committing some parts of the patch so that we reduce its size and make it more easily manageable. I'm going to review other parts of the patch now.
Here some comments on gtksourcebuffer.h gtksourceview/gtksourcebuffer.h ------------------------------- We probably lack an API to remove all the folds in a region. It may be useful when a user interactively removes a region of text. Am I wrong? We could eventually modify gtk_source_buffer_get_root_folds to get a region as input. What do you think?
/me adds his own little comment I think it would be useful to have an api to expand_all in a fold, in other words tp recursively unfold a fold and all his children. I am not 100% sure if this should be a public api or if it should for now only be used internally in SourceView bound to a keyboard shortcut...
Here's a new patch with the changes from #21. The apparent formatting problem in gtksourcefold.h is purely a cvs diff thing. If you apply the patch, the formatting is fine.
Created attachment 53580 [details] [review] gtksourcefold*.[ch] patch Patch with changes from #21.
gtksourceview/gtksourcebuffer.c ------------------------------- It would be cool if you could write a couple of lines explaining how this function is supposed to work possibly with some examples. +static gboolean +insert_child_fold (GtkSourceBuffer *buffer, + GtkSourceFold *child, + GtkSourceFold *parent, + GError **error) +{ + GtkTextIter begin, end, pbegin, pend, iter; + + if (*error) + return FALSE; It is not clear to me why you need this check. Please, add a comment on the use of the error parameter. /* fold already has children; try inserting it into one. */ + if (parent->children != NULL) + { + GList *folds = parent->children; + + while (folds != NULL) + { + GtkSourceFold *child_fold = folds->data; The "if" statement is redundant, You already have the "while" just below. + folds = g_list_next (folds); + } + } + + /* fold is inside parent, but not a child of parent. */ + DEBUG (g_message ("adding fold to parent fold @ %d", + gtk_text_iter_get_line (&pbegin))); + parent->children = g_list_append (parent->children, child); + child->parent = parent; + return TRUE; If you remove the "if" statement as I suggested you can more efficiently rewrite the code to avoid traversing the list twice. + /* We need to determine which siblings the child overlapses. + * Those siblings need to be reparented to the child. If the + * child intersects a sibling, then this operation is invalid. + * + * | <- parent fold + * | | <- child fold (to be inserted) + * | | [ <- first sibling (reparent) + * | | + * | | [ <- second sibling (reparent) + * | | + * | + * | [ <- third sibling (don't reparent) + * | + */ This comment is a bit misguiding since a few lines above you wrote: + /* check if the child fold overlaps the parent fold. */ + siblings = g_list_next (siblings); + while (siblings != NULL) Please, add a comment explaining that parent is the first sibling to reparent, otherwise the code is bit hard to read, i.e. it is not so clear why you move to the next sibling before the while. + DEBUG (g_message ("reparenting @ %d", gtk_text_iter_get_line (&pend))); + + reparent = g_list_append (reparent, sibling); + siblings = g_list_next (siblings); + } Use g_list_prepend and then reverse the list when you have done with it. + /* if sibling->parent is NULL, then it's a root fold. */ + if (sibling->parent == NULL) + { + siblings = g_list_find (buffer->priv->folds, sibling); + buffer->priv->folds = g_list_insert_before (buffer->priv->folds, + siblings, child); + buffer->priv->folds = g_list_remove (buffer->priv->folds, sibling); You traverse the list twice here (in g_list_find and g_list_remove). You can probably use g_list_delete_link. You can also write an ad-hoc list_replace_item function. + { + siblings = g_list_find (sibling->parent->children, sibling); + sibling->parent->children = g_list_insert_before (sibling->parent->children, + siblings, child); + sibling->parent->children = g_list_remove (sibling->parent->children, sibling); Same as above. + + if (error != NULL) + { + g_critical (error->message); + g_error_free (error); + return NULL; + } You leak "fold" here. + + /* add the fold at the end of the list. */ + if (folds == NULL) + { + DEBUG (g_message ("adding fold at end of root")); + buffer->priv->folds = g_list_append (buffer->priv->folds, fold); + } Note you have just traversed the entire list, using g_list_append you are going to traverse it again. I think we could store a pointer to the last item of the list and use it when appending the item. + /* the entire fold lies in the region, so add the fold + children. */ + else if (gtk_text_iter_compare (&fbegin, begin) >= 0 && + gtk_text_iter_compare (&fend, end) <= 0) + { + *list = g_list_append (*list, fold); + children = fold->children; + while (!fold->folded && children != NULL) { + get_folds_in_region (buffer, begin, end, children->data, list); + children = g_list_next (children); + } + } + /* start iter is in the region, so add the fold first. */ + else if (gtk_text_iter_compare (&fbegin, begin) >= 0 && + gtk_text_iter_compare (&fbegin, end) <= 0) + { + *list = g_list_append (*list, fold); + children = fold->children; + while (!fold->folded && children != NULL) { + get_folds_in_region (buffer, begin, end, children->data, list); + children = g_list_next (children); + } + } +} You have the same code for both branches, please collapse them in a single branch. Please, add a comment explaining which folds are returned by the get_folds_in_region and what it is used for (I was again asking to you the same questions I asked in my previous comments :) You normally check for 1 or -1 when comparing iterators, have you tested the various functions with iterators having the same offset? I have still to review the view part. But I think we can concentrate on fixing and committing the document part first.
Created attachment 54080 [details] [review] patch Attached patch incorporates Paolo's latest comments. Also adds the gtk_source_buffer_remove_folds_in_region method.
+ /* fold is inside parent, but not inside a child of parent. */ + DEBUG (g_message ("adding fold to parent fold @ %d", + gtk_text_iter_get_line (&pbegin))); + parent->children = g_list_append (last_fold, child); + child->parent = parent; + return TRUE; Are you sure about this call to g_list_append? I think you should not assign the result to parent->children. Or better, you should initialize last_fold to NULL (not to folds) and assign g_list_appen result to parent->children only if last_fold != NULL. Right? + + reparent = NULL; + reparent = g_list_append (reparent, parent); + Please write: reparent = g_list_append (NULL, parent); + /* if sibling->parent is NULL, then it's a root fold. */ + if (sibling->parent == NULL) + { + siblings = g_list_find (buffer->priv->folds, sibling); + buffer->priv->folds = g_list_insert_before (buffer->priv->folds, + siblings, child); + buffer->priv->folds = g_list_delete_link (buffer->priv->folds, siblings); + child->children = g_list_append (child->children, sibling); + child->parent = NULL; + sibling->parent = child; + } + else + { + siblings = g_list_find (sibling->parent->children, sibling); + sibling->parent->children = g_list_insert_before (sibling->parent->children, + siblings, child); + sibling->parent->children = g_list_delete_link (sibling->parent->children, siblings); + child->children = g_list_append (child->children, sibling); + child->parent = sibling->parent; + sibling->parent = child; + } What about using g_return_if_fail (siblings != NULL); siblings->data = child; instead of g_list_insert_before and g_list_delete_link? We are always sure siblings != NULL, right? + /* reparent all the following siblings as well. */ + l = g_list_next (reparent); + while (l != NULL) + { + sibling = l->data; + + if (sibling->parent != NULL) + sibling->parent->children = g_list_remove (sibling->parent->children, + sibling); + else + buffer->priv->folds = g_list_remove (buffer->priv->folds, sibling); + + child->children = g_list_append (child->children, sibling); + sibling->parent = child; + + l = g_list_next (l); + } This piece of code is way too expensive: - we know all the siblings we are removing are adjacent, instead of calling g_list_remove in a loop, get the first sibling to remove and then simply remove the consecutive siblings. Lemme try to write it in pseudo-C: GList *last = NULL; GList *first = NULL; /* reparent all the following siblings as well. */ l = g_list_next (reparent); while (l != NULL) { sibling = l->data; if (first == NULL) { if (sibling->parent != NULL) first = g_list_find (sibling->parent->children, sibling); else first = g_list_find (buffer->priv->folds, sibling); g_return_if_fail (first != NULL); last = first; } sibling->parent = child; last = g_list_next (last); l = g_list_next (l); } if (first->prev) first->prev->next = last; if (last) { last->prev->next = NULL; last->prev = first->prev; } first->prev = NULL; child->children = g_list_concat (child->children, first); + /* add the fold at the end of the list. */ + if (folds == NULL) + { + DEBUG (g_message ("adding fold at end of root")); + buffer->priv->folds = g_list_append (last_fold, fold); + } Are you sure about this call to g_list_append? See above. You normally check for 1 or -1 when comparing iterators, have you tested the various functions with iterators having the same offset? I have still to review the view part. But I think we can concentrate on fixing and committing the document part first.
Here some comments on gtksourceview.h gtksourceview/gtksourceview.h ----------------------------- I don't think we need to expose the show_fold_regions property as a public API. If we have folds then they must be shown and so, from a pratical point of view, show_fold_regions <-> gtk_source_buffer_get_root_folds (doc) != NULL. So I think we can remove the "show_fold_regions" property, the get/set function and the "show_fold_regions" field from struct _GtkSourceViewPrivate
There is a major issue with the patch: we lack signals emitted when a fold is added/removed, thus when folds are created dinamically (in response to a user action, e.g. typing, instead of hardcoding them) the expander is not drawed immediately. Another problem is that clicking on the line numbers coulmn causes the line number to be hidden. Another minor issue we discussed on irc and that I am writing down so that we do not forget is the use of g_list_length in the following hunk, since it walks the whole list when it's not needed + /* start with the first fold in the region */ + if (g_list_length (folds) > 0) + { + fold = folds->data; + + gtk_text_buffer_get_iter_at_mark (text_view->buffer, + &start_fold, + fold->start_line); + + }
Fold expansion when adding text at the fold boundaries seems odd. Try putting the cursor at the end of the line of a fold boundary and press <enter>. For the bottom boundary this expands the fold to include the new line, which it shouldn't (the bottom boundary should be preserved). For the top boundary it will move the top boundary to the new line, which it shouldn't (the new line should be simply included in the fold, the original top boundary should be preserved). Another thing is that the fold disappears (the expander disappears) when a fold which has an empty line as its top boundary disappears when folding. Also the next fold now acts strange because the expander is now hidden. Clicking in the text widget will render the expander visible again, but when moving the mouse over the expander it will disappear again. Mind the expansion still works though. The expander visibility is not limited to the fold right after the fold which disappeared (which caused all this strange behavior). I can't find consistency, but the base line is that one or more expanders render invisible, which they shouldn't.
Created attachment 54390 [details] [review] signals patch Attached patch implements the fold_added & fold_removed signals.
Created attachment 54391 [details] [review] new signals patch
jeroen didn't agree on irc to the removal of show_fold_regions. He makes a good point that the gutter should not go away/reappear when code folding is enabled and the number of folds drops to 0. Here are however my issues with that property: 1) (silly) I don't like the name much :) 2) The broken doc/view split of TextView makes the property a bit harmful: since all the logic is in the buffer and since all the buffer is not aware of the view a program could end up with the some folded folds in the buffer and with the gutter not showed in the view
Created attachment 54528 [details] screenshot of new GtkSourceFoldLabel Attached screenshot shows the new GtkSourceFoldLabel. This is a basic GtkLabel inherited class with a border drawn around it. It is not selectable in the textview. Still needs a bit of work, but the screenshot shows the basic idea. The label would only be shown when the fold is collapsed (similar to Eclipse). Comments? I've finished the "show_folds" code in gtksourcebuffer.c btw, but i'm behind a proxy atm. Will upload the new patch soonish.
Created attachment 54529 [details] better color for the border (better visible)
cool stuff on the ".." label :) Here are some comments on the gtksourceview.c code (far from a full review also because your current code has surely changed from the last patch attached here, but at least they do not get lost on irc) - I always have doubts when dealing with animations (the expander animation in this case). Shouldn't the animation be cancelled if the widget is destroyed while the animation is in progress? What if the text/folds change during the animation (if that can even happen, I am not sure it can happen) - in expose() you have + gtk_widget_style_get (widget, + "expander-size", &view->priv->expander_size, + NULL); + //view->priv->expander_size += EXPANDER_EXTRA_PADDING; I don't think refetching the style of the expander on every expose is right: what about doing it in widget_class->style_set()?
Created attachment 54797 [details] [review] new patch Attached patch is an updated version so the review process can continue. It has a really basic implementation for the new GtkSourceFoldLabel. Still has unfinished stuff left to do in gtksourceview.c.
Created attachment 54801 [details] [review] new patch with comments from pbor renamed the get|set_folds to get|set_folds_enabled. Fixed a minor code style issue.
Created attachment 55104 [details] [review] patch This patch includes a working implementation (minus 1 drawing bug) of the new GtkSourceFoldLabel.
<muntyan_> pbor: here's a bug btw, gtksourceview code folding uses tag with name "invisible" <muntyan_> it's not a good name for a tag used in a library <pbor> good point
*** Bug 362391 has been marked as a duplicate of this bug. ***
ping!.
So, what's the current status of this one?
New highlight engine is almost done (this time for real, beta releases are out). Once that is done, we can start revisiting Jeroen's work. As usual help is very much appreciated.
I may have a more recent patch on my laptop somewhere (will check this weekend). I'm not really involved with GNOME anymore (running OS-X and involved with Ruby on Rails now), but i'm willing to help out.
Ping?
Yeah, what's the status of this and does this patch fit somehow with the new engine?
Folding worked and should work now after some fixing-up (due to some internal api changes or something). But nobody has ever implemented the folding logic, i.e. what pieces of text to fold.
FWIW, in MonoDevelop we've now moved to a managed source editor, and one of the reasons for this was to get folding and various other features without waiting for unmanaged libraries to be released and hit the distros (for example, we continue to support users with GTK 2.8). We maintain optional GtkSourceView2 support... Our new editor doen't implement folding logic either; it has an API to mark fold regions. Our various parsers (which are implemented by extensions) simply return a set of fold regions along with their other parse information, and the editor binding consumes them from the parser service. I'm sure it's possible to handle folding in the editor widget, but external parser can do a much better job. For example, our HTML/ASP.NET parser can implicitly close <p> tags when they're followed by a block-level HTML element. They can also extract information for better summaries of the folded regions.
Ping, ping.
I resurrected the code, now it lives in code-folding-2 branch. Where did vertical lines which denote fold bounds go? To me single expander without indication where the region ends doesn't make sense.
which code did you resurrect? the one that just added the folds to the buffer and the drawing in the view or the one trying to integrate with the syntax engine? I seem to recall that the line in the mergin was definitely there, but its been a long time ago... bug #349060 has a patch to add the line (no idea how good)
I took code from folding+newhl branch, without any syntax highlighting stuff (which has never been implemented IIRC), only buffer and view code. The line on the margin is present on the first screenshot, but not in comment #36, so it's probably intentional. Or a bug?
I'd say it is a bug, since I definitely want the line :-)
Yevgen: could you please summarize the API additions? If I have seen correctly they are: - new functions in GeditDocument - GtkSourceFold - GtkSourceFoldLabel (private) There are no api changes in the view. Right? Which is the impact of the patch on the existing code? I remember I reviewed the code in document. Have you reviewed the other changes? Could you please post a screenshot? How do you plan to proceed?
*** Bug 500892 has been marked as a duplicate of this bug. ***
*** Bug 568007 has been marked as a duplicate of this bug. ***
Has this effort died?
I hope not because that would be really cool to have!
*** Bug 594233 has been marked as a duplicate of this bug. ***
Created attachment 145204 [details] A russian plugin to make code folding into gEdit. This plugin for gEdit does code folding.
Created attachment 145205 [details] File for the Russian code folding plugin This file goes with the other one about the code folding plugin, already existing and written in python.
I am used to use a code folding plugin written in Python by a Russian guy. It works but can be much better. What this plugin actually does : ° When you want to fold something (a function, a class, a switch...), you have to put the cursor on the first line of the "something" and do Alt + Z. So the first line loses all colours, and get gray foreground / green background. ° The plugin is based on the file indentation. For example if i have this in Java language : +public void changerCote(int cote){ + largeur=cote; + longueur=cote; +} I then have instead : +public Rectangle(int c_long, int c_larg){ +} If i have a comment starting on the line, like : +public void changerCote(int cote){ + largeur=cote; +// This is a comment + longueur=cote; +} I then have +public void changerCote(int cote){ +// This is a comment + longueur=cote; +} And i then can use the comment to fold the rest of the function. ° The line number of the lines folded are in the same level than the non-folded line. For example : +21 public void changerCote(int cote){ +22 largeur=cote; +23 longueur=cote; +24 } Will become this : +21 public void changerCote(int cote){ +2ø } Where 2, 3 and 4 are viewed instead of ø. I hope this will help for the code folding patch development.
Hey, I managed to merge large parts of code_folding_2 branch into master. Code folding works (mostly) if you click in the correct part of the gutter, but I have problems with the code to render the code-folding parts into the gutter, since in code_folding_2 branch, there was not gtk_source_gutter class. So for the moment, the code to paint the code folding is implemented in paint_marks_background method in gtksourceview, which is of course bad, since there is flicker and the gutter just render over it, but I don't know where it makes most sense to add it. My main question is about gtk_source_gutter class. It seems that I should add a new Renderer to do the render part, but I don't know if code_folding drawing should be done in a CellRenderer... Any hints on the best path to add the rendering part would be appreciated... I will keep a version of the code in http://github.com/jaliste/gtksourceview, in the branch code_folding_new.
It should quite easy to use the source gutter API. You indeed will need to pack a gtkcellrenderer in the gutter and then render the the toggle/expand at the corresponding lines. I think the easiest thing is to use GtkCellRendererPixbuf and the pixbuf-expander-closed, pixbuf-expander-open and is-expanded properties to show the expander/collapser icons.
Hey, I completely agree that to draw the expander, a cellrenderer using the gutter API would work. But my problem is how to draw the fold line in a cell renderer... I could pack the line in it, but it seems odd to me to use a cell renderer to draw the line. But maybe we don't want to draw a line and want to do something like done in Kate, which changes the colors of the gutter, in this case, I don't know... Maybe we should agree first about how to show the folds. For the moment, I am rendering in the gtksourceview expose event, just as the old code did...Some screenshots coming... BTW, the code still has a lot of (visible) bugs...
Created attachment 149239 [details] Screenshot using the patch Here is a screenshot of current functionality, a fold line is shown when you hover the expander.
I don't particulary like the way it is rendered, I think it could be improved. Not sure about the kate way, I like it but maybe it is kind of overkill showing so many colors.
In the end, the cell renderer is just a convenient way to pack columns where stuff is rendered in the gutter, and to make use of the existing functionality (like rendering text, icons, etc). That said, you can just render additional stuff yourself in the cell renderer functions. So, drawing a line is really not a problem. The render function will be called for each line that needs to be drawn, so it should be easy to decide whether or not to draw the indicator, and if or not to draw a line (or something else), you just need to do it on a per line basis. With regard to what to draw, I'm not sure. I kind of like the monodevelop style (single lines and +/- expand collapse).
Thanks for the clarifications... I am getting the picture... However, doing it the way you suggest, the data_func function of the renderer would be O(#number_of_root_folds) (that's the time you need to get the fold at the current line if you only have the view and the line as data). This means that in the expose we will end up with O(number_of_root_folds*number_of_lines_drawn). In the current code, which lives directly in the expose (but can live in the expose method of the gutter) takes O(number_of_root_folds) to find all the folds in the region to be drawn, then these are converted to a hashtable. So when you render each cell, directly, you only use O(number_of_root_folds) operations. For this reason, it does not seem convenient to me to use a Renderer to draw the folds. If I would be able to pass some extra info to the data_func of the renderer (like not only the current line, but the fold in the current_line also) then I could compute the holds in the expose and use the renderer as well. Anyway, for the moment, I will try to make things work better and cleaner (computing the folds in the expose of the gutter and then try to pass this to the renderer, e.g. by setting the properties of the renderer in a custom function that gets called explicitly in the expose event) and will update the bug when I have a clearer code.
Some update, Finally, I was able to do the render using the gutter api and a cell Renderer. It mainly works, but there is still a lot to do... especially since there is not support in highlighting afaik. Did anybody work in implementing support in the highlighting engine to parse the folds with the parse engine? There are still bugs and hacks in my branch... most notably: 1. Under some conditions after scrolling the fold gutter don't get updated, hover in the gutter redraws the gutter. 2. Some folds are wrongly collapsed, which makes the Expander to disappear. 3. Right now, I am using a text cell renderer, so I need to write a custom cell renderer that does the correct drawing. 4. The folds_renderer_data_function needs context information I compute in the expose_event of the gutter. I need a proper way to pass this info to that function. 5. Mouse clicks in the gutter always trigger the fold.
Created attachment 149496 [details] New screenshot showing the folding. Screenshot of my current branch
The last screenshot looks really nice. I think this design could fit fairly well.
(In reply to comment #72) > Some update, > > Finally, I was able to do the render using the gutter api and a cell Renderer. > It mainly works, but there is still a lot to do... especially since there is > not support in highlighting afaik. > > Did anybody work in implementing support in the highlighting engine to parse > the folds with the parse engine? Not yet. We still don't know if the right way is using the highlighting engine or just implement something else per language to manage this. Why don't you join us in #gedit on Gimpnet? > > There are still bugs and hacks in my branch... most notably: > > 1. Under some conditions after scrolling the fold gutter don't get updated, > hover in the gutter redraws the gutter. > 2. Some folds are wrongly collapsed, which makes the Expander to disappear. > 3. Right now, I am using a text cell renderer, so I need to write a custom cell > renderer that does the correct drawing. > 4. The folds_renderer_data_function needs context information I compute in the > expose_event of the gutter. I need a proper way to pass this info to that > function. > 5. Mouse clicks in the gutter always trigger the fold.
Regardless of where it's implemented, I would recommend a parsing interface which easily allows the end-user to extend/add/define parsers (e.g. with python/shell scripts). The way vim works in this regard is nice, allowing one to combine user-defined-marker based, and expression based fold parsers. Do a ":help fold-methods" in vim for some idea of what kinds of things are possible. At least to have an interface which is extensible enough to permit these different possibilities would be nice.
Created attachment 149557 [details] Last screenshot! Here is the last screenshot of my branch. I merged the patch of bug 149201, and some modifications to the c.lang file (basically adding support for brackets) and it works! Please note that in lines 9 and 10, there is no box showing the fold, but there are two folds there! (so it's a bug in the view code) However, as pbor commented on irc. The main problem with using highlighting engine to fold is that if context is too long, the highlight engine will detect it by steps and you get several folds... The big part still missing is the fold_manager... right now I am just adding folds whenever the sourcecontext matches... which is just bad...
Very very nice your last screenshot.
Is this work in your branch on github? Because I could not find it. I would be interested in cleaning this up and trying to work out the last bugs and see to merge it on master.
(In reply to comment #79) > Is this work in your branch on github? Because I could not find it. I would be > interested in cleaning this up and trying to work out the last bugs and see to > merge it on master. I updated my branch in github. Now it contains basic integration between folding and highlight engine, using an old version of your patch to bug #472660 (So it's far from ready to be merge). So there are three parts in the code: 1. buffer part (my branch contains mostly original code from Jeroen and was reviewed by P. Maggi long before in this bug) 2. View part, which I wrote. For the moment there some parts are hacky but can be cleaned up quickly( I hope)... I would like to make some changes to the gutter infrastructure and/or discuss better solutions to clean these hacky parts... 3. Highlighting + folding part. this is very very hacky right now and we need to discuss which approach is better.
Ok thanks. We just merged that context classes patch into master (the API has been renamed slightly). With regard to the points: 1. I haven't had a look at that, but I'll see to review that once again. 2. I will have a look at that as well. I'm fairly familiar with the gutter API (as I wrote it :)), so maybe I can shed some light on the matter. 3. Yes, I think we got the context classes thing pretty much implemented as we'd like. The classes are now applied right after the analysis is done. I'm not so sure about checking explicitly the 'fold' class in the highlighting engine and calling back into the source buffer, maybe we can find a way to make use (or add) signals to the engine to re-evaluate regions with folds externally (we'd like the engine to just manage the classes, but not imposing any meaning on them so that if you don't have to reimplement all of that when you write a new engine).
Hey, I apreciate the pre-review of my view code to help me iron out the hacks... I have rebased my work to master. Current branch lives in code_folding_new2 in github. about the gutter hack, please look at the expose_event handler in gtksourcegutter.c. - I still don't solve how to hide/show the fold gutter. - I would like to have a function like the data func in the gutter, but that is called once for each expose event. I need to call gtk_source_folds_in_region with the expose region once for the expose and save the folds in some context struct for the expose event. The hack right now is that this info is in the view struct directly... (I could use get_fold_at_line but this would be slower since I would have to traverse the list of folds for each line, by computing the folds in region, I only need to traverse the list of folds in the current region, which is better). More generally about the view part, my fold cell renderer implementation is very basic. We might want to prelight things when the user pointer is over a fold expander... We also want to use colors according to the style being used... Right now it just render black lines with cairo. About the folding+ engine integration... I am not sure that classes are good to implement folds... I dont understand wether is possible to nest classes...? I would just implement a syntax like <context name="blabla" fold="true"> in the engine. Then when you read contexts with fold, the position of the start and on the end of the context should be marked, and this info passed to the "fold manager"... The main problem with the above is that you need to nested contexts... this is easy to do in C for brackets, but I don't know how to do in html without a serious modification of the lang file. Other idea I had is to use the highlighting engine to define "hidden brackets" in some way, and then use a version of the code that matches brackets to indentify fold regions outside of the highlighting engine.
Created attachment 156159 [details] [review] fix showing and hiding the fold gutter
Created attachment 177288 [details] [review] First patch : add gtksourcefold struct I will start putting patches here so we can get the proper review. This is the first patch. It only adds a new struct to hold Folds.
Review of attachment 177288 [details] [review]: Some stylish comments inline. ::: gtksourceview/gtksourcefold.c @@ +33,3 @@ + GtkSourceFold *fold; + + fold = g_new0 (GtkSourceFold, 1); let's use g_slice_new, See also that the 0 version here is useless as you are initializing anyway all attributes. @@ +41,3 @@ + fold->start_mark = g_object_ref (gtk_text_buffer_create_mark (fold->buffer, + NULL, begin, FALSE)); + /* FIXME : Should the end mark have right gravity */ Makes sense doesn't it? My understanding of gravities is by try and fail :) @@ +60,3 @@ + return; + + if (!gtk_text_mark_get_deleted (fold->start_mark)) let's use {} with all conditions even if they are of just one statement @@ +89,3 @@ + g_return_val_if_fail (fold != NULL, NULL); + + copy = g_new (GtkSourceFold, 1); slice here too.
Hi! Does someone have news on this?
Patch needs-work as per comment 85 - volunteers welcome.
Created attachment 211799 [details] [review] fixsome bug throw comment 87 hi i want to work on this bug for start i make patch update and fix what said on comment 85 please say me what to do i'm new in gtksourceview but i have some experience in gtk and gobject (In reply to comment #87)
Created attachment 211801 [details] [review] CREATE STUCT FOLD SORRY:I SENT WRONG PATCH IN LAST COOMENT fix some bug throw comment 87 hi i want to work on this bug for start i make patch update and fix what said on comment 85 please say me what to do i'm new in gtksourceview but i have some experience in gtk and gobject (In reply to comment #87)
Created attachment 212043 [details] [review] folding struct so why no one make reaction? is there any body work on GtkSourceView also some other bug fixed. now i work on GtkTextTag's invisible property to create real folding i need help on it so is there any body know about GtkSourceView structure and know what i must work on?
i am sorry for the delay, I beleive that the patches in this bug are not the most up-to-date patches we had with Garret. Unfortunately, we have not have time for this in a long time... we were trying to solve other bugs... Let us try to discuss in IRC next week so I can try to find which are the latest patches so you can start from there.
(In reply to comment #91) > i am sorry for the delay, I beleive that the patches in this bug are not the > most up-to-date patches we had with Garret. Unfortunately, we have not have > time for this in a long time... we were trying to solve other bugs... Let us > try to discuss in IRC next week so I can try to find which are the latest > patches so you can start from there. as mentioned i update the bug to latest commit and now it can apply on the master branch also now i work on import gtksourcefoldlabel from your github to my patch still update drawing model of your code to gtk 3 drawing model i need info about gtksourcegutterrendererfold and little got confused about what code in your github branch i need to work on thanks for your reply
Created attachment 212380 [details] [review] Add GtkFoldStruct and GtkFoldLabel hi I add GtkSourceFoldLabel to patch some changelog is: 1.use draw function instead expose 2.use cairo instead gdk 3.make compatible with gtksourceview 4.remove _ from begin of function name 5.make compatible with gtksourceview 3.4.1(latest commit on 20 April 2012)
i can't upgrade gtksourcefoldcellrenderer because too many things change such 1.GtkSourceView use GtkSourceGutterRenderer instead GtkCellRenderer 2.some virtual function changed and some argument of them are removed 3._cell_renderer_get_size is deprecated in GtkSourceGutterRenderer and etc can anybody help me throw fix these issue
hi once i upgrade gtksourcefoldcellrenderer i got in to a great deal of problem because too many things are changed such as 1.GtkSourceView use GtkSourceGutterRenderer instead GtkCellRenderer 2.some virtual function changed and some argument of them are removed 3._cell_renderer_get_size is deprecated in GtkSourceGutterRenderer and etc can anybody help me throw fix these issue
*** Bug 708317 has been marked as a duplicate of this bug. ***
Hi there, I wanted to ask if someone is still working on code folding or is interested to work on it. I would like to help with it. Actually I want to implement a feature in Meld to fold equal regions ( see https://bugzilla.gnome.org/show_bug.cgi?id=664624 ) and it seems that the cleanest way to do this is to use some code folding API in gtksourceview. ;-) So if someone is working on this or interested in it, please let me know. Also this thread is already quite long and I am not sure, where to start actually, so if someone could help me here, it would be great. A little information about myself: I already worked with GTK in C, C++ and Python, although not too much. So I know the basics, but I don't have any deep knowledge. Regards David
The most recent branch is this one, but it doesn't do much: https://git.gnome.org/browse/gtksourceview/log/?h=wip/gutter-renderer-folds The branch starts at commit "Create empty GtkSourceGutterRendererFolds class", but the GtkSourceMarksSequence utility class (merged on master) is probably useful for the code folding too. Anyway, I think it is better to first implement code folding in an application, and then, if it works (and when the GtkTextView bugs for invisible text are fixed), we can look at adding an API to GtkSourceView. In the meantime, if the MarksSequence is useful, it can be copied to the application (it is not yet a public class). Good luck!
Actually, it was a long time since I thought about code folding. When writing the MarksSequence utility, I thought it would be useful for the code folding too, but I've lost my notes. Anyway, I've written an API proposal on this wiki page: https://wiki.gnome.org/Projects/GtkSourceView/CodeFolding Code folding should be stored as a tree, not a simple list of marks.
Created attachment 333077 [details] screenshot 2016-08-10 nore Hi Sebastian, thanks for your answer. In the last days I put some time in reading source code and documentation in order to understand gtksourceview a little more. I also had a look at your API proposal. Although I don't feel capable of commenting it yet, it gives me a rough understanding of where we are heading. What I understood by now is that test-folding should show me an example of your drawings. I also added some debug-messages in gutter_renderer_folds_draw(), draw_sign(), etc to see if they are really actually called, and yes they are. But all I see is an empty gutter. I attached a screenshot to make it clear. Maybe you can tell me, why it does not work. Yours, David
I don't know, the branch doesn't compile anymore for me due to a GCC error at another place in the code (an added flag that I don't know where it is added), so the branch needs to be rebased on top of master, but there are most probably some conflicts. IIRC you need to have lines of text in test-folding (enough to display all the drawings in the gutter).
But as I said, implement code folding first in an application (in C), then when it works, we can look at adding an API to GtkSourceView.
(In reply to Sébastien Wilmet from comment #101) > ... > IIRC you need to have lines of text in test-folding (enough to display all > the drawings in the gutter). I tried this, but it did not have any effect. In fact, all the functions in gtksourcegutterrendererfolds.c seem to be called correctly. And with cairo_surface_write_to_png() I can even draw everything correctly into a file. I really don't understand, what's happening. (In reply to Sébastien Wilmet from comment #102) > But as I said, implement code folding first in an application (in C), then > when it works, we can look at adding an API to GtkSourceView. Do you mean a real application or a little test application for this purpose, which only consists of a GtkSourceView with code folding?
I mean for a real-world purpose, to see if the API is convenient to use and has enough features. Maybe a good place to develop this API is in Gtef, or in an application (but written in C so that it can be moved easily to GtkSourceView). https://github.com/swilmet/gtef
Created attachment 333195 [details] [review] GtefGuterRendererFolds (In reply to Sébastien Wilmet from comment #104) > I mean for a real-world purpose, to see if the API is convenient to use and > has enough features. Maybe a good place to develop this API is in Gtef, or > in an application (but written in C so that it can be moved easily to > GtkSourceView). > > https://github.com/swilmet/gtef OK, so let's implement code folding into Gtef. :-) I started today by copying your gutter renderer to Gtef and rename everything, so I hope it is fitting now. I attached a patch. In the next days I want to be working on this some more. By the way, now at Gtef your test-folding example is working for me. :-) I am not sure about the workflow. For now I created a local git branch and work on it there. Yours, David
Created attachment 333200 [details] [review] GtefGutterRendererFolds Patch had a small mistake. Here is the new version
Created attachment 333202 [details] [review] GtefFoldRegion I found some time and motivation to write a rough GtefFoldRegion class. I hope you can have a look at it. I do not have a lot of experience with GObject, so maybe I did not do everything the right way. ;-)
David, It will be nice and more convenient if you share your code on github.
Yes, Gtef is currently on GitHub, so create a fork there and then do a pull request if you think it's ready. In Gtef, it's not a problem if the API is not directly perfect, it can be improved over time. I've filed the following issue, as I think it's a good first step: https://github.com/swilmet/gtef/issues/1 We can continue the discussion on GitHub, you can open new issues for discussing the other aspects of code folding.
I just moved my branch to github: https://github.com/NoreSoft/gtef/tree/code-folding
I close this bug because there are a lot of comments and previous attempts, so it is difficult to follow. I have opened a new bug. For historical purposes, someone can read this bug too. *** This bug has been marked as a duplicate of bug 775751 ***