*** Bug 486127 has been marked as a duplicate of this bug. ***

Sorry, the duplicate was really not meant here...

Just a note to others that I am working on it. I already have some code that builds but still horribly fails on test cases.

Created attachment 99331 [details] [review]
Patch to add a new marker API

This is a first try to add a marker API based on GtkTextMark. Not all properties of the mark are implemented (priority, tooltip, etc) but the basic stuff in test-widget works. Please tell me what you think about it - maybe I also got something wrong with the GSequence as I am not so familiar with it

Created attachment 99361 [details] [review]
GtkSourceMark API 0.1

This updated patch is the result of an IRC discussion with Paolo:

- Fixed indentation in some places (might still not be 100% correct)
- Call parent class methods were appropriate (see comment in mark_deleted)
- rename property "marker-type" to "category"
- gtk_source_mark_new() now returns a GtkSourceMark*
- Mark _gtk_source_mark_changed() as private

Other points that might be of interest:
- Because the iterator of an already deleted marker is now longer available the whole visible margin is updated when a marker is changed. This could be a perfomance issue though it seems rather fast

- gtk_text_buffer_class->mark_deleted() is NULL and thus not called from gtk_source_buffer_mark_deleted()

- Paolo proposed to move gtk_source_buffer_marker_next() to gtk_source_mark_next(). I can do it but the problem is that the markers are saved in a GSequence buffer->priv->markers and I would have to give the marker access to it. Comments?

- priorities and tooltips are still implemented, docs are missing ;-)

Created attachment 99402 [details] [review]
GtkSourceMark API 0.2

Changes:

- priority is now construct only as it makes things easier and I don't think more flexibility will be need
- Simplied the drawing code even more - it is now even less efficient
- Some other fixes...

In general I would need some help with the drawing/invalidating code in gtksourceview. I guess it's all not that difficult but I hope there are people out there with more knowledge of the low-level GTK+ stuff.
Priorities are also not working yet. I hope they would work if I sorted the markers in a appropriate order but I seem to misunderstand the drawing code.

I have also hacked a bit on Anjuta to suppor 2.0 and to have a better test-case. Things work out nicely apart from the priorities and that sometimes the markers are not drawn until a redraw of the whole widget happens.

Created attachment 99435 [details] [review]
GtkSourceMark API 0.2.1

This patch fixes a stupid bug that caused GtkTextBuffer to behave inconsistent because gtk_text_buffer_mark_set_real() was only called for GtkSourceMarks.

BTW, if you need a test-case checkout the gtksourceview2 branch of anjuta (needs this patch to be applied to gtksourceview to build)

Created attachment 99443 [details] [review]
GtkSourceMark API 0.3

After yet another discussed with pbor I was able to fix most things mentioned before. The patch seems to work rather smooth with the gtksourceview2 branch of anjuta and all basic things work (including priorities).

The last thing to implement (if it should be supported) would be the tooltips property.

Created attachment 99530 [details] [review]
GtkSourceMark API 0.9

This is no the final version that I would love to see in GNOME 2.22

- It has full doc coverage of the new API
- Tooltips API removed for now as that can be done later and is not a big priority for me atm

I would love to see this in trunk soon to be able to merge the gtksourceview2 branch of anjuta.

Thanks!

If I may, few comments. First of all, make patch with some
'svn diff --diff-cmd diff -x -up'

The function names look inconsistent: the object is called "GtkSourceMark" and functions are still something_marker (the GtkSourceBuffer methods).

How about background color for lines with markers (no idea if it's been there before)?

It would be nice if test-widget did more than saying "create marker"/"delete marker" (anjuta is good, but is impossible to build, I tried).

And bunch of random things:

1) gtk_source_mark_get_category and gtk_source_mark_get_priority should be the real getters, not the get_property method; and get_property should call those.
2) gtksourcebuffer.c: last parameter to g_signal_new() for buffer_signals[MARKER_UPDATED], should it be GTK_TYPE_SOURCE_MARK instead of GTK_TYPE_TEXT_MARK?
3) g_object_unref doesn't need to be cast to GDestroyNotify;
4) gtksourceview.c: //#undef ENABLE_DEBUG - either #define it or uncomment (no // comments).
5) gtksourceview.c,marker_updated_cb: C99-style variable declaration, not at the top of the block; and the indentation is gtk-style, not gtksourceview-style.
6) gtksourceview.c,sort_markers_by_priority(): could it be just 
return gtk_source_mark_get_priority(mark1) - gtk_source_mark_get_priority(mark2);
or 
return priority1 - priority2;
7) Somewhere in gtksourceview.c: //g_assert (current_marker == NULL); - either remove it or uncomment it.
8) gtksourceengine.h was deleted from Makefile.am.
9) test-widget.c,remove_all_markers: will crash since it doesn't delete the link from the list.
10) test-widget.c,button_press_cb: frees return value of gtk_source_mark_get_category().
11) gtksourceview.c,draw_line_markers: it doesn't like (complains with g_warning about "unknown marker") markers without assigned pixbuf. It's not right, is it?
12) same function: the loop would be nicer 
while (current_marker)
{
...
}
it'd be easier to read, since do..while immediately triggers the question "why post-condition?".
13) gtksourcebuffer.c,compare_marker(): it returns 1 if either mark is deleted. If this case is ever possible, then it is undefined behavior, comparison function must represent a real linear order. Perhaps deleted marks should be deleted from the sequence when they are deleted from the buffer, or the comparison function should pretend deleted marks are on line -1. Then, is it legal to have a comparison function which returns 0 for different objects? GSequence docs don't seem to say it's okay. For example, it would be natural for g_sequence_search() to return *any* position which compares equal to searched data, and if this position wasn't the first one then gtk_source_buffer_real_mark_deleted() and gtk_source_buffer_get_next_marker() will break.
14) gtk_source_buffer_get_markers_in_region(): it calls gtk_text_iter_get_line() inside MAX and MIN macros.
15) same function: it does linear search from the beginning of the buffer (no idea if it will matter in practice, depends on whether there ever will be more than hundred marks).
16) gtksourceview.c, sort_markers_by_priority(): again, is it legal to return 0 for different list elements?
17) gtk_source_view_get_marker_pixbuf(): ref's return value, it shouldn't.

And some general style stuff: if() should be

if (something)
{
   ...
}
else
{
   ...
}

but

if (something)
        do_something();

- no brackets for one-line branch (unless there is a big else branch).

and pointers in boolean context should be compared to NULL:

"if (ptr == NULL)" or "if (ptr != NULL)", not "if (ptr)" or "g_assert (foo_ptr && !bar_ptr)" (I don't like this one myself, but what can you do).

Function definitions:

void
function (gpointer foobar,
          int      blah)
{
}

not

void function (gpointer foobar, int blah)

Sorry, ignore my brainfart about sorting. Only the comment about deleted marks stands. And that matters only in one case, in mark-deleted handler. So, gtk_source_buffer_real_mark_deleted is called after the mark is deleted from the buffer, so the comparison function will return value different from what it returned when the mark was inserted. How is g_sequence_search going to work in that case? Won't it break?

I am too tired and I have too much of an head-splitting headache to provide sensible review comments right now, but since Yevgen added many useful comments (thanks!) I'll add some more minor style nitpicks so that you will not have to go through the patch one more time.

1) use "type *var", not "type* var"
2) gtk_source_mark_finalize is empty, you may as well not override it
3) when adding gtk-doc comments do not leave an empty line between the comment and the code
4) GTK_TEXT_BUFFER_CLASS(gtk_source_buffer_parent_class)->delete_range (... : leave a space after the CLASS macro
5)
+static void
+gtk_source_buffer_real_mark_set		(GtkTextBuffer		 *buffer,
+					 const GtkTextIter	 *location,
+					 GtkTextMark		 *mark)
spacing is screwed up here
6) in the same function, why is the parent handled called before the code? is it needed? usually it's a bit dangerous make assumptions on the side effects of the parent handler
7) in  marker_updated_cb you seem to be using 2-spaces indentation


'nuf nitpicking :-)



a couple of more serious issues that come to my mind, but take them with the benefit of doubt because as I said I am not really in the in the proper mood to think about this throughly:

 - does "priority" really make sense as a property of the mark? doesn't it only make sense when the mark is added inside a buffer? should we use something more similar to text tag priority? should the caller simply use the catagory to it's own priority sorting on the caller side? are there use cases where marks in the same category have different priorities or different categories have same priority?

 - GSList *gtk_source_buffer_get_markers_in_region: I know the method was already there, but I am not really fond of this function... is there any use case where this is needed in anjuta etc? I was thinking of making it private and just call it from the view. I'd rather expose some iterate on the marks in a region if needed: e.g. iter_forward_to_mark(category). With that kind of call, we could also get rid of buffer_get_first_mark etc, since getting the start iter and moving to the next mark would be simple enough.

 - next(), prev() (and if we keep them also get_in_region, get_first and get_last) should really take a category arg. If null they just operate on any mark

 - view should probably have a mark-activated signal triggered when the marker is clicked

 - we should probably have a way to (optionally) set callbacks on a marker category to get the pixbuf and tooltip to use so that you can have more advanced uses (e.g. think of numbered bookmarks where in the same category you have "1", "2", etc drawn in the sidebar. we can take inspiration from the gtktooltip api. Obviously the simpler version to just set a pixbuf for the whole category should stay as well.

> The function names look inconsistent: the object is called "GtkSourceMark" and
> functions are still something_marker (the GtkSourceBuffer methods).
>

Fixed
 
> How about background color for lines with markers (no idea if it's been there
> before)? 

Hmm, that wasn't here before and I think it is of little use. You can use other GtkTextView APIs if you need it.

> 1) gtk_source_mark_get_category and gtk_source_mark_get_priority should be the
> real getters, not the get_property method; and get_property should call those.

Fixed

> 2) gtksourcebuffer.c: last parameter to g_signal_new() for
> buffer_signals[MARKER_UPDATED], should it be GTK_TYPE_SOURCE_MARK instead of
> GTK_TYPE_TEXT_MARK?

No idea, would you prefer SOURCE_MARK?

> 3) g_object_unref doesn't need to be cast to GDestroyNotify;

Fixed

> 4) gtksourceview.c: //#undef ENABLE_DEBUG - either #define it or uncomment (no
> // comments).

Fixed

> 5) gtksourceview.c,marker_updated_cb: C99-style variable declaration, not at
> the top of the block; and the indentation is gtk-style, not
> gtksourceview-style.

Fixed

> 6) gtksourceview.c,sort_markers_by_priority(): could it be just 
> return gtk_source_mark_get_priority(mark1) -
> gtk_source_mark_get_priority(mark2);
> or 
> return priority1 - priority2;

Yeah, fixed! I knew that this must have been simpler...

> 7) Somewhere in gtksourceview.c: //g_assert (current_marker == NULL); - either
> remove it or uncomment it.

Removed, I think that does no longer fit because we sort the list somewhere in between.

> 8) gtksourceengine.h was deleted from Makefile.am.

Fixed

> 9) test-widget.c,remove_all_markers: will crash since it doesn't delete the
> link from the list.

This is completely different now because of the new API but has another problem. When you iterate over the marks using gtk_source_buffer_next_mark() and delete the mark you are currently using, your iterator becomes invalid. Maybe we should have delete_marks (buffer, category) in gtk_source_buffer

> 10) test-widget.c,button_press_cb: frees return value of
> gtk_source_mark_get_category().

Fixed

> 11) gtksourceview.c,draw_line_markers: it doesn't like (complains with
> g_warning about "unknown marker") markers without assigned pixbuf. It's not
> right, is it?

It's the same behaviour as before but we can of course change it if you like to.

> 12) same function: the loop would be nicer 
> while (current_marker)
> {
> ...
> }
> it'd be easier to read, since do..while immediately triggers the question "why
> post-condition?".

I did not write that code but changed anyway.

> 13) gtksourcebuffer.c,compare_marker(): it returns 1 if either mark is deleted.

This is now fixed as it is impossible to call compare_marker() with deleted markers now.

> 14) gtk_source_buffer_get_markers_in_region(): it calls
> gtk_text_iter_get_line() inside MAX and MIN macros.

Fixed, using gtk_text_iter_order() now.

> 15) same function: it does linear search from the beginning of the buffer (no
> idea if it will matter in practice, depends on whether there ever will be more
> than hundred marks).

Does not really matter in practice but feel free to suggest a better algorithm for these things. I tried to be as straightforward as possible but I think there are more elegant ways to use a GSequence.

> 16) gtksourceview.c, sort_markers_by_priority(): again, is it legal to return 0
> for different list elements?

Yes, so I changed it to also maintain the line ordering as there are problems with the drawing method otherwise. Markers with same priority will be drawn on top of each other in an undefined way. (you can't know which one will be on top)

> 17) gtk_source_view_get_marker_pixbuf(): ref's return value, it shouldn't.

I did not change this one in any way.

Style:

Tried to fix most of the style stuff, maybe some mistakes remain. Once you decide to accept the patch I will of course clean the rest up.

> 6) in the same function, why is the parent handled called before the code? is
> it needed? usually it's a bit dangerous make assumptions on the side effects of
> the parent handler

Well, it does not assume anything on the parent code - it does just call it here to make it work for GtkTextMarks (which are ignored for the following code). I can also use a if...else construct if you think it's better.

> a couple of more serious issues that come to my mind, but take them with the
> benefit of doubt because as I said I am not really in the in the proper mood to
> think about this throughly:
> 
>  - does "priority" really make sense as a property of the mark? doesn't it only
> make sense when the mark is added inside a buffer? should we use something more
> similar to text tag priority? should the caller simply use the catagory to it's
> own priority sorting on the caller side? are there use cases where marks in the
> same category have different priorities or different categories have same
> priority?
> 

Don't know but I wanted to have the flexibility to have it as a marker property. It is also simpler to implement.

>  - GSList *gtk_source_buffer_get_markers_in_region: I know the method was
> already there, but I am not really fond of this function... is there any use
> case where this is needed in anjuta etc? I was thinking of making it private
> and just call it from the view. I'd rather expose some iterate on the marks in
> a region if needed: e.g. iter_forward_to_mark(category). With that kind of
> call, we could also get rid of buffer_get_first_mark etc, since getting the
> start iter and moving to the next mark would be simple enough.
> 
>  - next(), prev() (and if we keep them also get_in_region, get_first and
> get_last) should really take a category arg. If null they just operate on any
> mark

It's now private and I added all API methods that I needed to use with anjuta. Feel free to propose better names than I have in the moment for these methods, please!

> 
>  - view should probably have a mark-activated signal triggered when the marker
> is clicked

The problem is to determine the position of the mark on screen. And I am really bad with those geometry things. => Future API addition (same code would work for tooltips) ;-)

>  - we should probably have a way to (optionally) set callbacks on a marker
> category to get the pixbuf and tooltip to use so that you can have more
> advanced uses (e.g. think of numbered bookmarks where in the same category you
> have "1", "2", etc drawn in the sidebar. we can take inspiration from the
> gtktooltip api. Obviously the simpler version to just set a pixbuf for the
> whole category should stay as well.

Do you really want to have it that complicated? It might be nice but I see no chance to get this in the 2.22 timeframe from my side - sorry! 

Created attachment 99636 [details] [review]
GtkSourceMark API 0.9.1

This is the latest patch (I also updated the anjuta branch).

- new API methods
- _gtk_source_buffer_get_marks_in_region() private
- some changes in the algorithm
- fixed style (mostly)

The biggest problem is deleting markers one is iterating over...

Sorry for jumping  in the discussion so late.

I have a few comments on the API.

1. I don't like very much to have both gtk_source_buffer_get_next_mark_iter and gtk_source_buffer_get_next_mark. Is the second one really needed? When?

2. gtk_source_buffer_get_marks_at_line should take a category too.

3. Shouldn't the priority be bound to the category and not to the specific mark?

Please, see the GtkSourceMarker section in http://live.gnome.org/GtkSourceView/Gsv2APIReview

(In reply to comment #16)
> Sorry for jumping  in the discussion so late.
> 

No problem though I might have saved me some work.

> 1. I don't like very much to have both gtk_source_buffer_get_next_mark_iter and
> gtk_source_buffer_get_next_mark. Is the second one really needed? When?

It is the only way to iterate over the markers in the moment though we probably need something else (see my last comment). 

> 
> 2. gtk_source_buffer_get_marks_at_line should take a category too.

No problem.

> 
> 3. Shouldn't the priority be bound to the category and not to the specific
> mark?

As I explained before we have more flexibility if it's a mark property but I you all agree that you would want it as a category property I can change it.

> 
> Please, see the GtkSourceMarker section in
> http://live.gnome.org/GtkSourceView/Gsv2APIReview
> 

Some comments:
- a line property is not needed as there is gtk_text_buffer_get_iter_at_mark ()
- "colored marker lines" - I personally don't need that in anjuta
- tooltips/"clicked": I really want this too but might need some help implementing it (see last comment).
- The rest should be more or less implemented now

I will come up with another patch probbably fixing most of the issues you mentioned.

Created attachment 100171 [details] [review]
GtkSourceMark API 0.9.2

OK, next iteration:

- priority is now based on the category and set using gtk_source_view_set_marker_priority()

- next/prev now go to the next mark from iter, the old next/prev methods are gone

- New foreach() method to iterate over the marks (useful when searching a special mark in anjuta...)

Happy review!

Created attachment 100604 [details] [review]
GtkSourceMark API 0.9.3

I started looking at the patch, and while reading it I couldn0t refrain from fixing up minor stylistic issues I found, so I am attaching the patch here to avoid redoing it. This one is more or less jhs' patch. the only real differences are
 - added gtk_source_mark_next/prev
 - removed the foreach function

Maybe I am missing something but I do not see the use case for the foreach API: it leads to a very inefficient way to process marks (you always traverse the whole list). IMHO the best way to search for marks is get the mark at an iter and then use gtk_source_mark_next

Apart from the small changes in the above patch here are some more comments on the implementation that I noticed:

 - I really do not like how gtk_source_buffer_get_next/prev_mark are implemented: they are done by getting a list with all the marks and then throwing it away, which is very inefficient. We should instead take advantage of gsequence (1) or if the api doesn't allow it, change data struture

 - It's not clear to me why compare_marks works using line numbers... shouldn't we properly order marks on the same line? why not use gtk_text_iter_compare?

 - in gtk_source_buffer_real_mark_deleted we have to manually trasverse the sequence which is unfortunate (O(N) instead O(log(n)))... I tried simply using g_sequence_search to find the mark to delete, but it didn't work. Any ideas?

 - I see you moved the priority stuff in the view... is that ok? should it be in the buffer? what do you guys think?


More comments to follow






(1) It seems there is not an easy way to find a "position" in a gsequence without having the data itself... but maybe we can do some dirty trick like creating a dummy mark at the requested iter position, then search in the gsequence and then remove the mark... Or if adding a dummy mark is not allowed, we could just create once without adding it to the buffer and then associate some data with it with g_object_set_data and special case it in the compare_marks function. Mind you, these are just a couple of ugly hacks that come to my mind, not sure if they work...

Created attachment 101015 [details] [review]
proof of concept improvements

Ok, I took a stab at solving some of the issues I listed above not all of them. 

Here are the changes:

buffer:
 - compare_marks now uses iter position to do the comparison
 - added gtk_source_buffer_get_marks_at_iter
 - gtk_source_buffer_get_next_iter: reimplements using one of the hacks I sketched above... it's a bit hackish, but it seems to work and should be efficient. get_prev_mark would need the same treatment
 - gtk_source_buffer_get_iter_at_line: reimplemented with the above functions

view:
 - reimplemented the whole drawing, it's much easier to follow IMHO, we just go line by line and draw the needed marks


It must be carefully evaluted how things behave when there are more marks at the same iter

about the drawing, forgot to say that I removed the pixbuf composition: IMHO if there are > 1 marks on a line just the one with higher prio should be drawn

I disagree with that.  If the debugger is stopped at a breakpoint, I want to be able to see the 'current line' marker and a breakpoint marker at the same time.  If it's not able to do that, it becomes very confusing.

Do you have a screenshot or mockup? I can't imagine how that would look any good: note that with the old code the pixbufs were just cramped one over another, so it would just see one + some junk...

Also, what are the usecases beside that one? I think there are better ways than a marker to highlight the current line (e.g. we have the line number in bold and the line background in another color).

That said that change was pretty much ortogonal to the rest so putting it back if needed is not a big issue... there are more pressing things about this patch to solve first.

Also, as I suggested in comment 12, once the core feature is in I think we should add a more powerful api to set a callback to draw the marker. Setting a pixbuf would be just an utility method. Using that api all crazy compositing could be done.

Created attachment 101017 [details]
screenshot showing breakpoint/current-line markers

Well, here's a screenshot of what I was talking about.  But I suppose per your last comment, if there were a callback API to draw the marker, that would probably be sufficient.  But honestly, I have trouble thinking of a case where it would be beneficial to *not* show a lower-priority marker.  Do you have a use case in mind?  In my mind, if you set markers for a line, you did so to show some information, so by not showing the lower-priority markers, you lose valuable information...

I agree with Jonathon here that I could be useful to show two or more markers on the same line. This is the usual case when debugging and I think it works that way in most editors.

Created attachment 101559 [details] [review]
GtkSourceMark using GArray

So today I took a look again at this source mark business and decided that GSequence is really not working well for us (sorry Johannes it was my fault suggesting to use it).

I went back to using a simple GArray which after all is the simplest thing and should work fairly well given that insertion and deletions of marks are relatively rare and the array allows to binary search for the mark once we know the iter. Beside normally the number of marks should be << than the number of lines, so things should not be too bad.
Code is simpler and easier to understand which is a good thing.

This patch seems to work fairly well, at least with test widget but more extensive testing is needed especially since test widget only offers ui to insert bookmarks at the start of a line.

The "view" part of the patch still needs a bit of work, but should be easy:
 - I need to put back pixbuf compositing since it seems used
 - "marker" should be renamed to source mark in the public API (would be nice to clean up test widget too)
 - I'd like feedback about the category/priority api... is it correct to have it per view? should it be per buffer?

Created attachment 101725 [details]
gtksourcemark.c

c file missing in the patch...

Created attachment 101726 [details]
gtksourcemark.h

h file missing

Created attachment 101896 [details] [review]
updated a bit

Latest updates:
 - properly rename the api in the view (s/marker/mark)
 - use a hash category <-> struct MarkCategory in the view
 - s/Marker/Mark in the demo
 - put back pixbuf compositing

Created attachment 101905 [details] [review]
different buffer api

This instead is more or less the same patch, but with the api for the buffer that I suggest: the buffer.next_mark() method in the prev patch behaves in a way that differs from all the other gtktextview api, since it may return a mark at the given iter position.
What I propose here is more consistent with the gtk api IMHO:

gtk_source_buffer_forward_iter_to_mark
gtk_source_buffer_backward_iter_to_mark

to move the iter

gtk_source_buffer_get_marks_at_iter

to get the marks.

If you want all the marks in a section including the current iter position you simply first call get_marks at the current iter and then forward the mark.
If you want to move the cursor to the next mark position you simply call forward_to_mark.


Personally I think that this is now ready to be committed (the api is IMHO complete and if needed we can improve the implementation incrementally), so it would be a good time to give the patch some review and/or raise api concerns... sounds like a fun way to start the year, no? :-)

PS: if insetead we go back to the previous patch, we need to fix a small bug I found: a strcmp result is not compared with 0.

First thanks for that great work but I have some comments:

- you rename "show_line_numbers" to "show-line_numbers". Doesn't this change API or does GTK+ just convert those silently?

- the priorities are ordered the wrong way. That's because sort_marks_by_priority sorts ascending and what we want is descending (draw the markers with lowest priority first). Either we change gtksourceview.c:1132 from

return priority2 - priority1;

to

return priority1 - priority2;

or we do a g_slist_reverse afterwards. The latter is cleaner as it sorts in a usualy way but has bad performance.

Otherwise, please commit this - it works great! (And i could close about 6 bugs...)

I have a few comments about the API. I have not reviewed the implementation.

1 - I don't like gtk_source_buffer_get_marks_at_line, I'd prefer to have a more generic *gtk_source_buffer_get_marks_in_region

2 - I think we need to a way to mark a "mark" as "invisible" or (may be) "disabled" (note that this is different from the visible property of GtkTextMark). This could be used to temporarily hide a marker or a set of markers (for example disable "warnings" while debugging).

3 - In some cases it would be nice to be able to add a "description" to the marker. This description could be shown as a tooltip when the mouse move over the icon associated with the marker (see elipse). 

4 - How can we avoid user call "gtk_text_mark_set_visible" on a GtkSourceMark? Do we want to avoid this?

5 - We should define a set of stock marker categoris for which GtkSourceView provided stock icons that are automatically associated by default to the category (e.g. breakpoint, bookmark, error, warning, etc.) (bug 132784). So, if we create a  mark of cateroty "GTK_SOURCE_MARK_CATEGORY_ERRROR", then GtkSourceView automatically associate to it an "error" icon. This means we need to install some stock icons with GtkSourceView (with support for themes).

6. GtkSourceMark should not have a public constructor (like GtkTextMark)

7. I think we should add the following functions: gtk_source_view_[get|set]_mark_category_background_color. See bug #315055.

I think point 1, 4 and 6 should be solved before committing.

The other points are improvements that can be added later in a API/ABI compatible way.

> 1 - I don't like gtk_source_buffer_get_marks_at_line, I'd prefer to have a more
> generic *gtk_source_buffer_get_marks_in_region

This is there as private API. All marks have "left-gravity" and usually you want to know if there is a mark in the given line. I have no strong option here but I would prefer to keep get_marks_at_line().

> 
> 2 - I think we need to a way to mark a "mark" as "invisible" or (may be)
> "disabled" (note that this is different from the visible property of
> GtkTextMark). This could be used to temporarily hide a marker or a set of
> markers (for example disable "warnings" while debugging).

That could be really useful!

> 
> 3 - In some cases it would be nice to be able to add a "description" to the
> marker. This description could be shown as a tooltip when the mouse move over
> the icon associated with the marker (see elipse). 

We already though about it but AFAIK it should be a later API addition.
 
> 4 - How can we avoid user call "gtk_text_mark_set_visible" on a GtkSourceMark?
> Do we want to avoid this?

I think we cannot avoid it but we should document it properly. By default marks are not visible which is a bit confusing because of course the pixbuf is visible. Setting them to visible will add a vertical bar before the first character in the line which simply looks ugly.

> 5 - We should define a set of stock marker categoris for which GtkSourceView
> provided stock icons that are automatically associated by default to the
> category (e.g. breakpoint, bookmark, error, warning, etc.) (bug 132784). So, if
> we create a  mark of category "GTK_SOURCE_MARK_CATEGORY_ERRROR", then
> GtkSourceView automatically associate to it an "error" icon. This means we need
> to install some stock icons with GtkSourceView (with support for themes).
> 

Isn't this overkill? Think about a LaTEX editor for example, it may use marks for completely different purpose. Or a editor used for collaboration, there markings might really have different meanings as error, warning, breakpoint, etc.

We should leave GtkSourceView as general as possible here and don't imply any use-cases.

> 6. GtkSourceMark should not have a public constructor (like GtkTextMark)

Should be trivial to fix...
 
> 7. I think we should add the following functions:
> gtk_source_view_[get|set]_mark_category_background_color. See bug #315055.

What happens when we have more than one mark in the line? Mix the colors?

I have spoken with pbor about the patch.

For point 1, he convinced me about not exposing gtk_source_buffer_get_marks_in_region and having gtk_source_buffer_get_marks_at_line

For point 4, we can "ignore" this problem

For point 6, we have seen that in current gtk+ there is a public constructor for GtkTextMark too, so it is ok to have a public constructor for GtkSourceMark.


So I think the patch could be committed as soon as the documentation will be completed.

Created attachment 102149 [details] [review]
Final version of the patch including docs and ChangeLog

Finished the documentation and added an appropriate ChangeLog entry. Kindly asking if I may commit this.

Thanks!

Comment on attachment 102149 [details] [review]
Final version of the patch including docs and ChangeLog

>Index: /home/jhs/devel/gtksourceview/ChangeLog
>===================================================================
>--- /home/jhs/devel/gtksourceview/ChangeLog	(Revision 1823)
>+++ /home/jhs/devel/gtksourceview/ChangeLog	(Arbeitskopie)
>@@ -1,3 +1,115 @@
>+2008-01-04  Paolo Borelli  <pborelli@katamail.com>
>+            Johannes Schmid <jhs@gnome.org>

[snip]

>+
>+2008-01-04  Johannes Schmid,,,  <jhs@idefix>
>+
>+	reviewed by: <delete if not using a buddy>
>+

Why did you add two changelog entries?

> <para>
>-
>+GtkSourceView is the main object of the gtksourceview library. It provides
>+a text view which syntax highlighting, undo/redo and text marks. Use a 
>+#GtkSourceBuffer to display text with a GtkSourceView.
> </para>

As I said on IRC, I think we should put something more explicative here.
See GtkTextTag documentation for inspiration.
See also the documentation of gtk_source_buffer_create_marker in gsv 1.

>+	/**
>+	 * GtkSourceMark:category:
>+	 *
>+	 * The category of the #GtkSourceMark. Controls which pixmap is displayed
>+	 * when the mark is added to a #GtkSourceBuffer.
>+	 */

The category is used to classify marks. It can be used for associating pixbug but not only for this.

>+	g_object_class_install_property (object_class,
>+					 PROP_CATEGORY,
>+					 g_param_spec_string ("category",
>+							      "category",
>+							      "The marker category",
>+							      "",
>+							      G_PARAM_READABLE | G_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY));

Mark strings for translations.

Is the default category the empty string or NULL?

>+/**
>+ * gtk_source_mark_new:
>+ * @name: Name of the #GtkSourceMark
>+ * @category: Type of the #GtkSourceMark, controls which pixmap is displayed

Category is not only for pixmap.

>+ *
>+ * Creates a new #GtkSourceMark. You normally do not need it and can use 
>+ * gtk_source_buffer_create_marker() instead for convenience.

Add a few words about @name and @category.

>+/**
>+ * gtk_source_mark_next:
>+ * @mark: a #GtkSourceMark
>+ * @category: a string specifying the mark category or %NULL
>+ *
>+ * Returns the next #GtkSourceMark in the buffer or %NULL if the mark
>+ * was not added to a buffer.

What does it happen if there are no "next" mark?

>+
>+/**
>+ * gtk_source_mark_prev:
>+ * @mark: a #GtkSourceMark
>+ * @category: a string specifying the mark category or %NULL
>+ *
>+ * Returns the previous #GtkSourceMark in the buffer or %NULL if the mark
>+ * was not added to a buffer.

What does it happen if there are no "prev" mark?

>+	/**
>+	 * GtkSourceBuffer::source-mark-updated
>+	 * @buffer: the buffer that received the signal
>+	 *
>+	 * The ::source_mark_updated signal is emitted each time 
>+	 * a mark is added to or removed from the @buffer.
>+	 **/

Is it also emitted when a mark is moved?

> 
> /**
>- * gtk_source_buffer_create_marker:
>+ * gtk_source_buffer_create_mark:
>  * @buffer: a #GtkSourceBuffer.
>- * @name: the name of the marker, or %NULL.
>- * @type: a string defining the marker type, or %NULL.
>- * @where: location to place the marker.
>+ * @name: the name of the mark, or %NULL.
>+ * @category: a string defining the mark category.
>+ * @where: location to place the mark.
>  *
>- * Creates a marker in the @buffer of type @type.  A marker is
>- * semantically very similar to a #GtkTextMark, except it has a type
>- * which is used by the #GtkSourceView displaying the buffer to show a
>- * pixmap on the left margin, at the line the marker is in.  Because
>- * of this, a marker is generally associated to a line and not a
>- * character position.  Markers are also accessible through a position
>- * or range in the buffer.
>+ * Creates a source mark in the @buffer of category @category.  A source mark is
>+ * a #GtkTextMark, but it displays a pixmap in the margin.

This is not very precise.


For all the functions taking category (like forward_iter, backward_iter, get_marks_at_iter, etc.) we should
specify what happens when category is NULL.

>+ * gtk_source_buffer_get_marks_at_line:
>  * @buffer: a #GtkSourceBuffer.
>- * @iter: a #GtkTextIter to initialize.
>- * @marker: a #GtkSourceMarker of @buffer.
>+ * @line: a line number.
>+ * @category: category to search for or %NULL
[snip]
>+ * Returns the list of marks of the given category at @line.
>+ * If @category is NULL, all marks are included

It should be "If @category is NULL, all marks at @line are included".

> gboolean
>-gtk_source_view_get_show_line_markers (GtkSourceView *view)
>+gtk_source_view_get_show_line_marks (GtkSourceView *view)
> {
>-	g_return_val_if_fail (view != NULL, FALSE);
> 	g_return_val_if_fail (GTK_IS_SOURCE_VIEW (view), FALSE);
> 
>-	return view->priv->show_line_markers;
>+	return view->priv->show_line_marks;
> }

It should be "return (view->priv->show_line_marks != FALSE);" to normalize the return value.

>- * gtk_source_view_set_marker_pixbuf:
>+ * gtk_source_view_set_mark_category_pixbuf:
>  * @view: a #GtkSourceView.
>- * @marker_type: a marker type.
>- * @pixbuf: a #GdkPixbuf.
>+ * @category: a mark category.
>+ * @pixbuf: a #GdkPixbuf or #NULL.
>  *
>- * Associates a given @pixbuf with a given @marker_type.
>+ * Associates a given @pixbuf with a given @category.
>+ * If @pixbuf is #NULL, the pixbuf is unset.

It should be "Associates a given @pixbuf with a given marker @category."
> 
> /**
>- * gtk_source_view_get_marker_pixbuf:
>+ * gtk_source_view_get_mark_category_pixbuf:
>  * @view: a #GtkSourceView.
>- * @marker_type: a marker type.
>+ * @category: a mark category.
>  *
>- * Gets the pixbuf which is associated with the given @marker_type.
>+ * Gets the pixbuf which is associated with the given @category.
>  *

Some as above.

>+/**
>+ * gtk_source_view_set_mark_category_priority:
>+ * @view: a #GtkSourceView.
>+ * @category: a mark category.
>+ * @priority: the priority for the category
>+ *
>+ * Set the priority for the mark category. Mark of categories with
>+ * higher priorities will be drawn on top.

It should be "Set the @priority for the given mark @category. When there are multiple marks on the same line,
marks of categories with
higher priorities will be drawn on top."

Is it possible to unset a "priority"?

Created attachment 102165 [details] [review]
Fix mentioned issues

Thanks for the quick review!

I tried to fix all points you mentioned (and those we discussed on IRC) but feel free to do some nitpicking.

Thanks for all your comments and review and especially to Paolo Borelli for coding the backend. I fix the remaing issues in the documentation and commited the patch!