Created attachment 35472 [details]
XML helper plugin

This still needs "plugins/xmlhelp/Makefile" added to the AC_OUTPUT section in
configure.in in the dist root.

not a review by any standard (I just scrolled through the code...), but

- you use a lot of gedit_document_* functions, you should use gtk_text_view_*
functions instead (GeditDocument inherits from GtkTextView, those functions are
just leftovers from a lang time ago and some of them have been removed in
2.9.X). Note that this may require some rework of the code to operate using
GtkTextIter instead of offsets.

- Not a big deal, but it would be cool if you could use the same coding style of
the rest of gedit

- Not looked closely, but I see you include isspace etc... I seem to recall that
glib has equivalent functions which are utf8 safe.

- I spotted a if (not NULL) g_free, g_free alreday checks for NULL, so no need
fror the if


Btw, in the future please attach the file directly not gzipped, it makes take a
look at them easier.


On a more general note I was thinking that it would be cool to have a
"xml-bundle" plugin (or collection of plugins) which add more xml specific
tools, for instance a validator.

- I will use the gedit internal API when you guys document what it is. Come to
think of it, the plugin API isn't documented either, and for that matter is
insufficient to write REALLY good plugins.

This XML helper is very crude because it depends on the user hitting a keyboard
shortcut or using the menu option. It really NEEDS to run live, and e.g. fill in
the last tag when you type "</". But there is no plugin hook that would let me
do stuff like that!

- I like my coding style fine, and it's not that different from other plugins.
I think only space before ()...

- isspace() is not used in the code

- checking for NULL is just good habit... just because g_free() is careful,
doesn't mean I don't need to be.

If you're thinking of proper XML support, I would consider that totally
unrelated to this plugin... this is really just like the name says, a simple XML
"helper" -- something to help you out when you need to type in a lot of XML. It
can do a lot more, and would if the plugin API permitted "live" actions, but I'd
still keep it separate from "real" XML stuff like well-formedness, validation,
doing clever things with DTDs etc.

You are right the gedit internal API is not documented and probably it is not
stable enough to write 3rd part plugins, but pbor only suggested you to use
GtkTextBuffer API insted of the GeditDocument one since we are going to clean
GeditDocument removing some of the functions that are already defined in the
parent class GtkTextBuffer.

You don't need plugin hooks to make your plugin run live. You only need to
connect to the GtkTextBuffer signal and see when the user type "</". The
automatic spell checker in the spell plugin may show you how to perform this
operation.

I like my coding style too, but when possible we prefer to adopt the official
GNOME coding style (see
http://developer.gnome.org/doc/guides/programming-guidelines/code-style.html)

Checking for NULL is a good habit, but why do you want to check twice?
From g_free documentation:
"Frees the memory pointed to by mem. If mem is NULL it simply returns."

Thanks,
Paolo M.

I guess I need to clarify some points:

- The XML bundle plugin I was talking about above was just a blue sky idea and
has nothing to do with the inclusion of your plugin. I was just stating that it
would be cool to collect a series of xml related functionalities :)

- about the "gedit internals api" I am exactly saying that you should NOT use
it: the plugin is passed a GeditDocument, which *is* a GtkTextBuffer, so you are
free to use all the gtk_text_buffer api as if you were writing your own editor.
That's pretty much as powerful as you can get, with regard to content manipulation.
If you need to use gtk_text_view apis you can easily obtain the view as well.
So to reiterate: try to not use gedit_document_*... in fact if you try to
compile against CVS head you'll find that some of the functions are gone.
The plugin api should only be needed to modify the gedit UI (and at the moment
it can only add menu items)

- about coding style: yes, I'm sure you like yours just fine, everybody does.
And I also know that it's really similar to the rest of the code (just the space
 before "(" and braces on new lines).
The fact is that if the plugin is to be included in gedit we would really prefer
to keep the coding style consistent across the codebase, since when we change
something in the plugin interface we have to fix all the plugins included.

- isspace is mentioned near an #include... that's why I pointed it out.

- g_free has been introduced *exactly* to automatically check for NULL, so if
(!NULL) before g_free is noise.

(just for the record, I submitted the comment above at the same time as Paolo M.
did, thus the redundancy :)

Created attachment 35652 [details]
corrected xmlhelper

This replaces xmlhelp.c in the previous attachment.

This version uses no keyboard shortcuts or menu items; it automatically turns
itself on when an XML file is being edited, and does magic stuff as you type.

Also, all the gedit_document calls are gone, and it's all
GtkTextBuffer/iter/mark etc stuff.

Typing </ auto-closes the last open tag, and typing <> lets you cycle through
opening a new tag.

Comment on attachment 35652 [details]
corrected xmlhelper

Hi, 
  I have given a view to your code (but I have not tested it).
It seems good and, if it will prove to be robust enough when I will test it, I
think it could be included in the upcoming gedit 2.10.
I have only a few comments:
* there are still some style issues (minor problem we could fix just before
committing)
* reading the code I had the impression that it has some problem with
"undo/redo". May you please verify it?
* please, run it with valgrind and/or memprof to check for memleaks or other
problems (we can fix them also later)
* please, define a simbolic constant for the max size of the stack (256) (minor
issue)

The only showstop I see is the undo/redo problem we will have to test
carefully.

pbor: other comments?

ops... another commets.
I think it should work also when the language is HTML.

I have tested it and it seems to have some problems, i.e. I can make it crashes.

How to reproduce it:
1. Run gedit
2. Enable View->Highlight Mode->Markup->XML
3. Write <aaa></
4. It closes </aaa>
5. Write <bbb></
6. It closes </bbb>
7. Write <b
8. It crashes

I'm using CVS HEAD for gedit, gtksourceview and gtk+.

Here the stacktrace:

Backtrace was generated from '/opt/gnome/gnome210/INSTALL/bin/gedit'

Using host libthread_db library "/lib/tls/libthread_db.so.1".
[Thread debugging using libthread_db enabled]
[New Thread -151150464 (LWP 29491)]
0x0077b7a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2

+ Trace 54265

Yo

Another crash with similar steps but different stack trace:

Backtrace was generated from '/opt/gnome/gnome210/INSTALL/bin/gedit'

Using host libthread_db library "/lib/tls/libthread_db.so.1".
[Thread debugging using libthread_db enabled]
[New Thread -151150464 (LWP 29505)]
0x0077b7a2 in _dl_sysinfo_int80 () from /lib/ld-linux.so.2

+ Trace 54266

<CUT>

Comment on attachment 35652 [details]
corrected xmlhelper

Hi, 
  I have other comments on your code.
Lemme concentrate on the close tag part for the moment.


>	/* automatically fill in end tag when </ is entered */
>	if (text[0] == '/')
>	{
>		prevpos = *position;
>		gtk_text_iter_backward_char (&prevpos);
>		buf = gtk_text_iter_get_visible_text (&prevpos, position);
>		if (g_utf8_get_char(buf) == '<')
>		{
>			gtk_text_iter_backward_char(&prevpos);
>			element = find_endable_tag(buffer, &prevpos);
>			if (element)
>			{
>				g_signal_handlers_block_by_func(buffer, buffer_insert_text_handler, NULL);
>				gtk_text_buffer_insert_at_cursor (buffer, "/", -1);
>				gtk_text_buffer_insert_at_cursor (buffer, element, -1);
>				gtk_text_buffer_insert_at_cursor (buffer, ">", -1);
>				g_signal_handlers_unblock_by_func(buffer, buffer_insert_text_handler, NULL);
>				g_free (element);
>				g_signal_stop_emission_by_name (buffer, "insert-text");
>			}
>		}

I think this part of code is a bit dangerous... I'd prefer a solution that does
not play too much with blocking/unblockin signals.
Instead of catching the "insert-text" signal of the document, I think you
should catch the "key_press_event" of the view and
when the "/" key is pressed, check if the character just before the "insert"
mark is '<'. In that case, instead of adding
"/" you will add as a single operation the closing tag. In this way you will
not have problems with the undo manager (that is working on the insert signal
of the document). In the case you have added the tag, then the handler of
"key_press_event" will have to return TRUE, otherwise FALSE.
For an example of how to use this technique give a look at "key_press_cb"
method in gtksourceview.c
(http://cvs.gnome.org/viewcvs/gtksourceview/gtksourceview/gtksourceview.c?rev=1
.30&view=markup).

Other comments to come...

Regards,
Paolo

Created attachment 35696 [details]
new xmlhelp.c

This version:

- works with undo/redo
- seems ok with valgrind (please check, I don't know valgrind very well)
- has a symbol for the stack size
- has the segfault fixed
- uses "key_press_event" instead of "insert-text"
- doesn't try to close processing instructions

Created attachment 35768 [details]
XML helper plugin

same version, but a complete list of the new files, plus a patch for
configure.in

<biot> I disagree about turning it on for HTML as well
<paolo> why?
<biot> it's really quite XML-specific, at least the auto-close thing
<biot> since with HTML, you don't *have* to close tags
<biot> hence, you can't assume that the tag you're closing is simply the last
opened one
<biot> some people might find it handy, but most would not... since most people
write sloppy HTML
<biot> so at most, something like a way to turn it on for HTML files, but not
automatic
<paolo> hmmm... I think there should be a way to temporaly escape the tag closing
<paolo> what does it happens in the following case:
<paolo> hmmm.. nothing wrong example
<biot> perhaps a menu item so you can enable the plugin, if your file is HTML
<biot> although that's not very intuitive
<paolo> no, I was thinking something like pressing the ESC key if you don't want
the plugin close the tag
<paolo> for example while you are editing a CDATA
<paolo> actually it should be automatically disabled while you are editing a CDATA
<biot> I don't get what you mean
<paolo> suppose you want to edit:
<paolo> <a>bbb</a><![CDATA[test: </b>]]
<paolo> you cannot if the XMLhelp plugin is active
<paolo> am I smoking crack?
<biot> you mean typing in the </b> wouldn't work?
<biot> (it does...)
<paolo> does it detect there is not open tag?
<biot> yes
<biot> if there's nothing there to fill in, it just doesn't do anything, you
keep typing
<paolo> and this: <a>bbb</a><![CDATA[test: <a></b>]]
<biot> wait
<biot> I'm wrong about the first one, it closed that <!CDATA
<biot> I have no idea what <!CDATA means, should probably find out 
<biot> :)
<paolo> CDATA
<paolo> 
<paolo> Everything inside a CDATA section is ignored by the parser.
<paolo> 
<paolo> If your text contains a lot of "<" or "&" characters - as program code
often does - the XML element can be defined as a CDATA section.
<paolo> 
<paolo> A CDATA section starts with "<![CDATA[" and ends with "]]>
<paolo> http://www.w3schools.com/xml/xml_cdata.asp
<biot> yeah, just looking at the spec now
<biot> figuring out if you're in a CDATA section is going to involve quite a bit
of parsing
<biot> not like the quick tricks I do now
<paolo> well, if syntax hl is enabled there will be a trick, i.e. seeing if you
are inside a syntax region tag called "CDATA"
<biot> ah, gtksourceview provides functionality for this?
<paolo> yep, it can help 
<paolo> but it is a bit hacky
[...]
<paolo> biot: I'm going to attach the IRC log to the bug report. Is it ok for you?
<biot> about the HTML thing?
<paolo> and the CDATA one
<biot> ok

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

Has any more work gone into the plugin? Or will something like it be written later?

Well, it works as advertised. It's up to Paolo to decide whether he wants it in
gedit or not.

I figure that (while I'm removing myself from the CC list) I may as well move 
this to the New state and add the PATCH keyword.   

The backtrace in comment #10 is the same as a gnome-terminal crash reported in
bug  151680

Hello Biot ;)

this is far from WONTIFIX even if we haven't had time to get back to it since we
are still working on the new plugin system etc. reopening.

If you don't have time/will to work on the plugin that's fair enough, but I
think the bug should stay open as it contains useful info for anyone who wants
to write this feature.

Then please remove my email from the list... I wasn't sure how to do that, since
I'd originally opened this bug.

It's obvious to me that no action will be taken on this, hence I've lost interest.

Bert, I'm sorry but I don't think it's possible to remove your mail address
since as you say you are the original reporter and bugzilla doesn't allow it.
Given the lack of activity on this bug in recent times, I hope you can put up
with some random mails... at worse you can use procmail.

I'm also sorry for the lack of action on the plugin and I understand your
frustration, but we have very limited manpower and we don't work on gedit for a
job. Our resources have been put elsewere
(http://live.gnome.org/Gedit_2fNewMdi). When that major work is done, we hope to
get more traction on plugins especially considering that writing them in python
will be supported.

Once again, sorry for having wasted some of your time, I at least hope that in
the meantime your plugins has been useful for you. (By the way nothing stops you
from releasing it as a separate package).


ciao,
    paolo

We are moving all the request for plugins that we don't plan to add to gedit
itself on http://live.gnome.org/Gedit/Plugins.

Closing as WONTFIX.

Now (hopefully) fully obsoleted by Python Gedit plugin available on http://live.gnome.org/Gedit/XMLHelperPlugin.