After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 134610 - Code folding
Code folding
Status: RESOLVED DUPLICATE of bug 775751
Product: gtksourceview
Classification: Platform
Component: General
git master
Other Linux
: Normal enhancement
: ---
Assigned To: GTK Sourceview maintainers
GTK Sourceview maintainers
: 362391 500892 568007 594233 708317 (view as bug list)
Depends on: 618560
Blocks: 644703
 
 
Reported: 2004-02-17 10:30 UTC by David Makovský (Yakeen)
Modified: 2016-12-07 11:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
code folding implementation (40.64 KB, patch)
2005-08-23 08:56 UTC, Jeroen Zwartepoorte
needs-work Details | Review
test.c (1.24 KB, text/x-csrc)
2005-08-23 08:56 UTC, Jeroen Zwartepoorte
  Details
IRC log with comments about the new API and more (8.85 KB, text/plain)
2005-08-23 14:08 UTC, Paolo Maggi
  Details
patch with the API comments (45.48 KB, patch)
2005-08-25 08:38 UTC, Jeroen Zwartepoorte
none Details | Review
new patch (53.68 KB, patch)
2005-08-30 06:54 UTC, Jeroen Zwartepoorte
none Details | Review
latest patch (60.04 KB, patch)
2005-08-30 10:27 UTC, Jeroen Zwartepoorte
none Details | Review
new patch (80.12 KB, patch)
2005-09-25 20:36 UTC, Jeroen Zwartepoorte
none Details | Review
gtksourcefold*.[ch] patch (12.04 KB, patch)
2005-10-17 13:13 UTC, Jeroen Zwartepoorte
none Details | Review
patch (70.19 KB, patch)
2005-10-30 14:08 UTC, Jeroen Zwartepoorte
needs-work Details | Review
signals patch (3.04 KB, patch)
2005-11-06 16:31 UTC, Jeroen Zwartepoorte
none Details | Review
new signals patch (4.71 KB, patch)
2005-11-06 17:12 UTC, Jeroen Zwartepoorte
none Details | Review
screenshot of new GtkSourceFoldLabel (40.86 KB, image/png)
2005-11-09 11:02 UTC, Jeroen Zwartepoorte
  Details
better color for the border (better visible) (40.93 KB, image/png)
2005-11-09 11:08 UTC, Jeroen Zwartepoorte
  Details
new patch (56.71 KB, patch)
2005-11-15 19:58 UTC, Jeroen Zwartepoorte
none Details | Review
new patch with comments from pbor (56.78 KB, patch)
2005-11-15 20:53 UTC, Jeroen Zwartepoorte
none Details | Review
patch (62.41 KB, patch)
2005-11-22 20:02 UTC, Jeroen Zwartepoorte
none Details | Review
A russian plugin to make code folding into gEdit. (4.65 KB, text/x-python)
2009-10-10 09:01 UTC, generalzarmakuizz
  Details
File for the Russian code folding plugin (796 bytes, text/plain)
2009-10-10 09:03 UTC, generalzarmakuizz
  Details
Screenshot using the patch (32.36 KB, image/png)
2009-12-07 12:07 UTC, José Aliste
  Details
New screenshot showing the folding. (35.03 KB, image/png)
2009-12-10 01:29 UTC, José Aliste
  Details
Last screenshot! (24.58 KB, image/png)
2009-12-11 00:55 UTC, José Aliste
  Details
fix showing and hiding the fold gutter (1.33 KB, patch)
2010-03-15 02:03 UTC, Garrett Regier
none Details | Review
First patch : add gtksourcefold struct (12.69 KB, patch)
2010-12-31 02:48 UTC, José Aliste
needs-work Details | Review
fixsome bug throw comment 87 (13.35 KB, patch)
2012-04-11 01:37 UTC, Bijan Binaee
none Details | Review
CREATE STUCT FOLD (14.66 KB, patch)
2012-04-11 02:10 UTC, Bijan Binaee
none Details | Review
folding struct (12.78 KB, patch)
2012-04-14 11:51 UTC, Bijan Binaee
none Details | Review
Add GtkFoldStruct and GtkFoldLabel (22.16 KB, patch)
2012-04-19 19:22 UTC, Bijan Binaee
none Details | Review
screenshot 2016-08-10 nore (34.26 KB, image/png)
2016-08-10 21:30 UTC, David Rabel
  Details
GtefGuterRendererFolds (15.05 KB, patch)
2016-08-12 20:36 UTC, David Rabel
none Details | Review
GtefGutterRendererFolds (15.02 KB, patch)
2016-08-13 00:10 UTC, David Rabel
none Details | Review
GtefFoldRegion (19.52 KB, patch)
2016-08-13 02:48 UTC, David Rabel
none Details | Review

Description David Makovský (Yakeen) 2004-02-17 10:30:37 UTC
Enhancment wish - Code folding with strategies
Comment 1 Jeroen Zwartepoorte 2005-08-23 08:56:27 UTC
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
Comment 2 Jeroen Zwartepoorte 2005-08-23 08:56:55 UTC
Created attachment 51172 [details]
test.c
Comment 3 Jeroen Zwartepoorte 2005-08-23 08:59:06 UTC
Just to be clear, this will only work if you have gtk+-2.8.0 or higher.
Comment 4 Paolo Maggi 2005-08-23 13:08:59 UTC
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.
Comment 5 Paolo Maggi 2005-08-23 14:08:51 UTC
Created attachment 51192 [details]
IRC log with comments about the new API and more
Comment 6 Jeroen Zwartepoorte 2005-08-25 08:38:06 UTC
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.
Comment 7 Jeroen Zwartepoorte 2005-08-25 09:03:20 UTC
Note: now that there is a gtksourcebuffer-private.h, can we move
_gtk_source_buffer_highlight_region to it without breaking API/ABI?
Comment 8 Jeroen Zwartepoorte 2005-08-25 21:15:37 UTC
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.
Comment 9 Paolo Maggi 2005-08-26 08:25:35 UTC
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).
Comment 10 Jeroen Zwartepoorte 2005-08-30 06:54:35 UTC
Created attachment 51544 [details] [review]
new patch

Patch adds gtksourcefold.[ch] and gtksourcefold-private.h files.
Comment 11 Jeroen Zwartepoorte 2005-08-30 10:27:39 UTC
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 12 Paolo Maggi 2005-08-30 13:23:35 UTC
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.
Comment 13 Paolo Maggi 2005-08-30 13:54:29 UTC
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.
Comment 14 Paolo Maggi 2005-08-30 15:16:20 UTC
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)
Comment 15 Paolo Maggi 2005-08-30 17:16:33 UTC
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.
Comment 16 Jeroen Zwartepoorte 2005-08-31 13:19:16 UTC
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.
Comment 17 Jeroen Zwartepoorte 2005-08-31 13:20:57 UTC
Ehh, example typo. Fold [5,10] in region [7,9].
Comment 18 Paolo Maggi 2005-08-31 13:29:12 UTC
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]
Comment 19 Jeroen Zwartepoorte 2005-09-25 20:36:30 UTC
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.
Comment 20 Jeroen Zwartepoorte 2005-09-25 20:38:37 UTC
See
http://www.xs4all.nl/~jeroen/screenshots/Screenshot-GtkSourceView-Markers.png
for an intial implementation of the markers + line numbers + code folding. Comments?
Comment 21 Paolo Maggi 2005-10-17 10:39:41 UTC
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.
Comment 22 Paolo Maggi 2005-10-17 10:55:48 UTC
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?
Comment 23 Paolo Borelli 2005-10-17 11:29:50 UTC
/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...
Comment 24 Jeroen Zwartepoorte 2005-10-17 13:12:23 UTC
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.
Comment 25 Jeroen Zwartepoorte 2005-10-17 13:13:14 UTC
Created attachment 53580 [details] [review]
gtksourcefold*.[ch] patch

Patch with changes from #21.
Comment 26 Paolo Maggi 2005-10-17 13:35:05 UTC
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.
Comment 27 Jeroen Zwartepoorte 2005-10-30 14:08:57 UTC
Created attachment 54080 [details] [review]
patch

Attached patch incorporates Paolo's latest comments. Also adds the
gtk_source_buffer_remove_folds_in_region method.
Comment 28 Paolo Maggi 2005-11-05 16:21:05 UTC
+		/* 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.
Comment 29 Paolo Maggi 2005-11-05 17:15:05 UTC
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
Comment 30 Paolo Borelli 2005-11-05 18:00:59 UTC
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);
+		
+	}
Comment 31 jessevdk@gmail.com 2005-11-06 13:06:43 UTC
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.
Comment 32 Jeroen Zwartepoorte 2005-11-06 16:31:03 UTC
Created attachment 54390 [details] [review]
signals patch

Attached patch implements the fold_added & fold_removed signals.
Comment 33 Jeroen Zwartepoorte 2005-11-06 17:12:43 UTC
Created attachment 54391 [details] [review]
new signals patch
Comment 34 Paolo Borelli 2005-11-06 18:41:27 UTC
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
Comment 35 Jeroen Zwartepoorte 2005-11-09 11:02:57 UTC
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.
Comment 36 Jeroen Zwartepoorte 2005-11-09 11:08:45 UTC
Created attachment 54529 [details]
better color for the border (better visible)
Comment 37 Paolo Borelli 2005-11-12 12:14:17 UTC
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()?
Comment 38 Jeroen Zwartepoorte 2005-11-15 19:58:58 UTC
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.
Comment 39 Jeroen Zwartepoorte 2005-11-15 20:53:12 UTC
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.
Comment 40 Jeroen Zwartepoorte 2005-11-22 20:02:07 UTC
Created attachment 55104 [details] [review]
patch

This patch includes a working implementation (minus 1 drawing bug) of the new
GtkSourceFoldLabel.
Comment 41 Paolo Borelli 2005-12-12 09:59:30 UTC
<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
Comment 42 Paolo Borelli 2006-10-28 14:51:20 UTC
*** Bug 362391 has been marked as a duplicate of this bug. ***
Comment 43 Diego Escalante Urrelo (not reading bugmail) 2007-04-04 04:12:51 UTC
ping!.
Comment 44 Sven Herzberg 2007-06-18 20:53:40 UTC
So, what's the current status of this one?
Comment 45 Paolo Borelli 2007-06-18 22:36:18 UTC
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.
Comment 46 Jeroen Zwartepoorte 2007-06-19 07:24:44 UTC
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.
Comment 47 Diego Escalante Urrelo (not reading bugmail) 2008-02-15 09:23:48 UTC
Ping?
Comment 48 Johannes Schmid 2008-03-02 00:02:03 UTC
Yeah, what's the status of this and does this patch fit somehow with the new engine?
Comment 49 Yevgen Muntyan 2008-03-02 02:26:46 UTC
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.
Comment 50 Mikayla Hutchinson 2008-05-06 21:01:56 UTC
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.
Comment 51 Diego Escalante Urrelo (not reading bugmail) 2008-07-29 06:37:23 UTC
Ping, ping.
Comment 52 Yevgen Muntyan 2008-08-03 23:06:20 UTC
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.
Comment 53 Paolo Borelli 2008-08-04 06:58:18 UTC
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)
Comment 54 Yevgen Muntyan 2008-08-04 07:08:50 UTC
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?
Comment 55 Paolo Borelli 2008-08-04 07:19:09 UTC
I'd say it is a bug, since I definitely want the line :-)
Comment 56 Paolo Maggi 2008-08-04 07:56:01 UTC
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?
Comment 57 Paolo Borelli 2009-01-03 11:43:56 UTC
*** Bug 500892 has been marked as a duplicate of this bug. ***
Comment 58 Ignacio Casal Quinteiro (nacho) 2009-01-16 21:22:34 UTC
*** Bug 568007 has been marked as a duplicate of this bug. ***
Comment 59 Oli 2009-08-12 08:45:52 UTC
Has this effort died?
Comment 60 Johannes Schmid 2009-08-12 09:07:08 UTC
I hope not because that would be really cool to have!
Comment 61 Ignacio Casal Quinteiro (nacho) 2009-09-05 16:21:28 UTC
*** Bug 594233 has been marked as a duplicate of this bug. ***
Comment 62 generalzarmakuizz 2009-10-10 09:01:27 UTC
Created attachment 145204 [details]
A russian plugin to make code folding into gEdit.

This plugin for gEdit does code folding.
Comment 63 generalzarmakuizz 2009-10-10 09:03:02 UTC
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.
Comment 64 generalzarmakuizz 2009-10-10 09:17:54 UTC
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.
Comment 65 José Aliste 2009-12-06 21:08:30 UTC
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.
Comment 66 jessevdk@gmail.com 2009-12-07 11:27:06 UTC
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.
Comment 67 José Aliste 2009-12-07 11:57:51 UTC
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...
Comment 68 José Aliste 2009-12-07 12:07:16 UTC
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.
Comment 69 Ignacio Casal Quinteiro (nacho) 2009-12-07 12:20:28 UTC
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.
Comment 70 jessevdk@gmail.com 2009-12-07 13:19:35 UTC
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).
Comment 71 José Aliste 2009-12-08 01:25:23 UTC
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.
Comment 72 José Aliste 2009-12-09 00:19:10 UTC
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.
Comment 73 José Aliste 2009-12-10 01:29:15 UTC
Created attachment 149496 [details]
New screenshot showing the folding. 

Screenshot of my current branch
Comment 74 Johannes Schmid 2009-12-10 09:05:04 UTC
The last screenshot looks really nice. I think this design could fit fairly well.
Comment 75 Ignacio Casal Quinteiro (nacho) 2009-12-10 10:52:04 UTC
(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.
Comment 76 Mark Edgington 2009-12-10 16:43:00 UTC
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.
Comment 77 José Aliste 2009-12-11 00:55:34 UTC
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...
Comment 78 Ignacio Casal Quinteiro (nacho) 2009-12-11 01:23:44 UTC
Very very nice your last screenshot.
Comment 79 jessevdk@gmail.com 2010-01-03 13:17:29 UTC
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.
Comment 80 José Aliste 2010-01-04 10:30:34 UTC
(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.
Comment 81 jessevdk@gmail.com 2010-01-04 12:19:26 UTC
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).
Comment 82 José Aliste 2010-01-04 15:39:30 UTC
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.
Comment 83 Garrett Regier 2010-03-15 02:03:57 UTC
Created attachment 156159 [details] [review]
fix showing and hiding the fold gutter
Comment 84 José Aliste 2010-12-31 02:48:15 UTC
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.
Comment 85 Ignacio Casal Quinteiro (nacho) 2010-12-31 10:42:23 UTC
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.
Comment 86 Yann SOUBEYRAND 2011-11-05 17:07:28 UTC
Hi!

Does someone have news on this?
Comment 87 André Klapper 2011-11-05 18:38:35 UTC
Patch needs-work as per comment 85 - volunteers welcome.
Comment 88 Bijan Binaee 2012-04-11 01:37:52 UTC
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)
Comment 89 Bijan Binaee 2012-04-11 02:10:07 UTC
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)
Comment 90 Bijan Binaee 2012-04-14 11:51:34 UTC
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?
Comment 91 José Aliste 2012-04-14 12:17:43 UTC
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.
Comment 92 Bijan Binaee 2012-04-15 14:59:52 UTC
(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
Comment 93 Bijan Binaee 2012-04-19 19:22:19 UTC
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)
Comment 94 Bijan Binaee 2012-04-23 02:12:53 UTC
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
Comment 95 Bijan Binaee 2012-04-23 02:15:41 UTC
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
Comment 96 Igor Gnatenko 2013-09-18 17:55:15 UTC
*** Bug 708317 has been marked as a duplicate of this bug. ***
Comment 97 David Rabel 2016-08-06 19:26:41 UTC
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
Comment 98 Sébastien Wilmet 2016-08-06 19:51:21 UTC
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!
Comment 99 Sébastien Wilmet 2016-08-07 13:18:28 UTC
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.
Comment 100 David Rabel 2016-08-10 21:30:42 UTC
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
Comment 101 Sébastien Wilmet 2016-08-11 05:43:09 UTC
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).
Comment 102 Sébastien Wilmet 2016-08-11 05:47:46 UTC
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.
Comment 103 David Rabel 2016-08-11 18:05:18 UTC
(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?
Comment 104 Sébastien Wilmet 2016-08-11 18:27:06 UTC
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
Comment 105 David Rabel 2016-08-12 20:36:02 UTC
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
Comment 106 David Rabel 2016-08-13 00:10:54 UTC
Created attachment 333200 [details] [review]
GtefGutterRendererFolds

Patch had a small mistake. Here is the new version
Comment 107 David Rabel 2016-08-13 02:48:25 UTC
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. ;-)
Comment 108 Bijan Binaee 2016-08-13 03:59:51 UTC
David, It will be nice and more convenient if you share your code on github.
Comment 109 Sébastien Wilmet 2016-08-13 07:34:02 UTC
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.
Comment 110 David Rabel 2016-08-13 12:14:36 UTC
I just moved my branch to github: https://github.com/NoreSoft/gtef/tree/code-folding
Comment 111 Sébastien Wilmet 2016-12-07 11:54:25 UTC
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 ***