Hi all.
I attach three patches: 1. Keybindings to insert new line after/before current 2. Snippet keybindings support 3. Automatically insert additional indentation in XML/HTML files if previous line ends with an opening tag
They must be unrelated and can be applied independently of each other.
Some of the patches have drawbacks. Before "polishing" them, I want to know whether they are useful and may go to trunk.
The drawback of the first patch is that I doubt it's so useful. I know Vi[m] provides shortcuts for these actions ('o' and 'O' in normal mode), but their necessity is questionable for me. I implemented this to ease XML editing with tag autocompletion turned on. Suppose you need to write to consequent <li>'s. Without this patch you need to press End+Enter after writing the first <li> to place the cursor where you need to start the second. With my patch, you only need to press one shortcut you can assign in Preferences > Keybindings :)
The drawback of the third patch is that it's not completed. If user likes to leave HTML tags like <br> "unclosed", she would be disturbed by automatic indentation caused by my patch, so a check box in Preferences is desirable. I'll code it as soon as we decide this patch can go to trunk.
Speaking about the second patch, here is an example of assigning a keybinding (I currently use it in my user snippets.conf):
# special group to define keybindings # keybindings' format resembles the one used in Preferences > # Keybindings tab [Keybindings] block_cursor=<Alt>bracketleft
Keybindings may be assigned either in user or global snippets.conf (I'm not sure if the latter is useful). Another decision I made is to allow keybindings for snippets in [Special] section (like "block_cursor" used above). While "block_cursor" snippet is not very useful when you have to type its name, it is useful when you just have to press a shortcut.
And, as usual for me, all of these patches lack user documentation…
Best regards, Eugene.
On Sat, 10 Jul 2010 21:48:35 +0400 Eugene Arshinov earshinov@gmail.com wrote:
I attach three patches:
Thanks; as we're preparing a bugfix release I haven't looked at them, but anyway:
- Snippet keybindings support
Not sure about this - shouldn't the keybinding be typing the snippet name and pressing tab ;-) I use short names to minimise typing - b for a brace pair, c for a multiline comment.
If we do want snippet keybindings perhaps it would be better to integrate with the existing keybindings rather than storing them separately - then users could configure them in the normal way.
Regards, Nick
Hi Nick
On Mon, 16 Aug 2010 13:41:00 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Sat, 10 Jul 2010 21:48:35 +0400 Eugene Arshinov earshinov@gmail.com wrote:
I attach three patches:
Thanks; as we're preparing a bugfix release I haven't looked at them, but anyway:
No problem, it can wait.
- Snippet keybindings support
Not sure about this - shouldn't the keybinding be typing the snippet name and pressing tab ;-) I use short names to minimise typing - b for a brace pair, c for a multiline comment.
I tried, but even b<tab> seems too long for me to type in order to get a brace pair. I must note, an easier and quicker way of inserting a brace pair is the main reason of why I wrote this patch, and I agree that there may be not so many uses of snippet keybindings.
If we do want snippet keybindings perhaps it would be better to integrate with the existing keybindings rather than storing them separately - then users could configure them in the normal way.
It's worth thinking.
Best regards, Eugene.
On 17 August 2010 05:34, Eugene Arshinov earshinov@gmail.com wrote:
Hi Nick
On Mon, 16 Aug 2010 13:41:00 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Sat, 10 Jul 2010 21:48:35 +0400 Eugene Arshinov earshinov@gmail.com wrote:
I attach three patches:
Thanks; as we're preparing a bugfix release I haven't looked at them, but anyway:
No problem, it can wait.
- Snippet keybindings support
Not sure about this - shouldn't the keybinding be typing the snippet name and pressing tab ;-) I use short names to minimise typing - b for a brace pair, c for a multiline comment.
I tried, but even b<tab> seems too long for me to type in order to get a brace pair. I must note, an easier and quicker way of inserting a brace pair is the main reason of why I wrote this patch, and I agree that there may be not so many uses of snippet keybindings.
If we do want snippet keybindings perhaps it would be better to integrate with the existing keybindings rather than storing them separately - then users could configure them in the normal way.
It's worth thinking.
Hi Eugene,
This could be a bit of work since the number of snippets is variable and the keybindings GUI is fixed. I had a quick look a while ago with the view of allowing the extra build commands to have keybindings but never came up with a simple way of handling variable numbers of commands.
If you decide to add a general way of adding variable numbers of entries in the keybindings GUI it could then be used elsewhere.
Cheers Lex
Best regards, Eugene. _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On Tue, 17 Aug 2010 10:08:08 +1000 Lex Trotman elextr@gmail.com wrote:
If we do want snippet keybindings perhaps it would be better to integrate with the existing keybindings rather than storing them separately - then users could configure them in the normal way.
It's worth thinking.
This could be a bit of work since the number of snippets is variable and the keybindings GUI is fixed. I had a quick look a while ago with the view of allowing the extra build commands to have keybindings but never came up with a simple way of handling variable numbers of commands.
If you decide to add a general way of adding variable numbers of entries in the keybindings GUI it could then be used elsewhere.
Plugins can change the number of keybindings in their plugin's key group*, so maybe this could be done for other things in the core. It would probably only take some small changes. 'Format->Send selection to' keybindings could be variable size too.
* http://www.geany.org/manual/reference/pluginutils_8h.html#e8eeecc54d81ce0545...
Regards, Nick
On 18 August 2010 00:35, Nick Treleaven nick.treleaven@btinternet.com wrote:
On Tue, 17 Aug 2010 10:08:08 +1000 Lex Trotman elextr@gmail.com wrote:
If we do want snippet keybindings perhaps it would be better to integrate with the existing keybindings rather than storing them separately - then users could configure them in the normal way.
It's worth thinking.
This could be a bit of work since the number of snippets is variable and the keybindings GUI is fixed. I had a quick look a while ago with the view of allowing the extra build commands to have keybindings but never came up with a simple way of handling variable numbers of commands.
If you decide to add a general way of adding variable numbers of entries in the keybindings GUI it could then be used elsewhere.
Plugins can change the number of keybindings in their plugin's key group*, so maybe this could be done for other things in the core. It would probably only take some small changes. 'Format->Send selection to' keybindings could be variable size too.
http://www.geany.org/manual/reference/pluginutils_8h.html#e8eeecc54d81ce0545...
Looks like a good solution, I never thought of looking in the plugin interface.
Cheers Lex
Regards, Nick _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On Wed, 18 Aug 2010 09:56:45 +1000% Lex Trotman elextr@gmail.com wrote:
On 18 August 2010 00:35, Nick Treleaven nick.treleaven@btinternet.com wrote:
On Tue, 17 Aug 2010 10:08:08 +1000 Lex Trotman elextr@gmail.com wrote:
If we do want snippet keybindings perhaps it would be better to integrate with the existing keybindings rather than storing them separately - then users could configure them in the normal way.
It's worth thinking.
This could be a bit of work since the number of snippets is variable and the keybindings GUI is fixed. I had a quick look a while ago with the view of allowing the extra build commands to have keybindings but never came up with a simple way of handling variable numbers of commands.
If you decide to add a general way of adding variable numbers of entries in the keybindings GUI it could then be used elsewhere.
Plugins can change the number of keybindings in their plugin's key group*, so maybe this could be done for other things in the core. It would probably only take some small changes. 'Format->Send selection to' keybindings could be variable size too.
http://www.geany.org/manual/reference/pluginutils_8h.html#e8eeecc54d81ce0545...
Looks like a good solution, I never thought of looking in the plugin interface.
Here is the new patch. It seems to be not so simple and clean as the previous one, but now keybindings are edited in Preferences dialog as (I believe) most users prefer.
Unfortunately, keybindings_set_item was not enough for me to implement this. For example, I had to extend GeanyKeyGroup struct so that I have a way to tweak loading from / saving to keybindings.conf. Anyway, any suggestions about how to improve the patch are welcome.
Currently there are two little problems
- All underscores from keybinding names are removed (I wonder why), so names of some "special" snippets are displayed incorrectly (**)
- Editing of keybindings in the Preferences dialog isn't "caught" completely by Geany. For example, if I remove a keybinding using click - BackSpace - Enter and press OK, Geany thinks I edited nothing and does not update keybindings.conf. Though, as expected, the removed keybinding no longer functions.
** and users are allowed to assign keybindings for "special" snippets
I finished this patch today, so I hadn't much time for testing. As for now, everything except the two issues above seems to work fine.
Best regards, Eugene.
On Sun, 29 Aug 2010 21:35:35 +0400% Eugene Arshinov earshinov@gmail.com wrote:
On Wed, 18 Aug 2010 09:56:45 +1000% Lex Trotman elextr@gmail.com wrote:
On 18 August 2010 00:35, Nick Treleaven nick.treleaven@btinternet.com wrote:
On Tue, 17 Aug 2010 10:08:08 +1000 Lex Trotman elextr@gmail.com wrote:
If we do want snippet keybindings perhaps it would be better to integrate with the existing keybindings rather than storing them separately - then users could configure them in the normal way.
It's worth thinking.
This could be a bit of work since the number of snippets is variable and the keybindings GUI is fixed. I had a quick look a while ago with the view of allowing the extra build commands to have keybindings but never came up with a simple way of handling variable numbers of commands.
If you decide to add a general way of adding variable numbers of entries in the keybindings GUI it could then be used elsewhere.
Plugins can change the number of keybindings in their plugin's key group*, so maybe this could be done for other things in the core. It would probably only take some small changes. 'Format->Send selection to' keybindings could be variable size too.
http://www.geany.org/manual/reference/pluginutils_8h.html#e8eeecc54d81ce0545...
Looks like a good solution, I never thought of looking in the plugin interface.
Here is the new patch. It seems to be not so simple and clean as the previous one, but now keybindings are edited in Preferences dialog as (I believe) most users prefer.
Unfortunately, keybindings_set_item was not enough for me to implement this. For example, I had to extend GeanyKeyGroup struct so that I have a way to tweak loading from / saving to keybindings.conf. Anyway, any suggestions about how to improve the patch are welcome.
Currently there are two little problems
All underscores from keybinding names are removed (I wonder why), so names of some "special" snippets are displayed incorrectly (**)
Editing of keybindings in the Preferences dialog isn't "caught" completely by Geany. For example, if I remove a keybinding using click - BackSpace - Enter and press OK, Geany thinks I edited
nothing and does not update keybindings.conf. Though, as expected, the removed keybinding no longer functions.
** and users are allowed to assign keybindings for "special" snippets
I finished this patch today, so I hadn't much time for testing. As for now, everything except the two issues above seems to work fine.
Best regards, Eugene.
Sorry, that patch is a little outdated. Here is the last version.
On Sun, 29 Aug 2010 21:59:32 +0400 Eugene Arshinov earshinov@gmail.com wrote:
> If we do want snippet keybindings perhaps it would be better > to integrate with the existing keybindings rather than > storing them separately - then users could configure them in > the normal way.
Here is the new patch. It seems to be not so simple and clean as the previous one, but now keybindings are edited in Preferences dialog as (I believe) most users prefer.
Unfortunately, keybindings_set_item was not enough for me to implement this. For example, I had to extend GeanyKeyGroup struct so that I have a way to tweak loading from / saving to keybindings.conf. Anyway, any suggestions about how to improve the patch are welcome.
OK, I didn't explain before what I had in mind but I think this patch is too complex.
What I think we could accept is setting up the snippet keybinding group size and items (like a plugin) when snippets.conf is read, using any defaults found in the file. I don't think we should write to snippets.conf. If the user wants to set the keybinding in snippets.conf, then they must remember that it won't be kept in sync with keybindings.conf. This should make the code simpler.
Currently there are two little problems
- All underscores from keybinding names are removed (I wonder why), so names of some "special" snippets are displayed incorrectly (**)
** and users are allowed to assign keybindings for "special" snippets
This is so a menu item string with underscores for mnemonics can be reused without adding a translation string but without displaying the underscore mnemonics for the keybindings dialog.
This could be fixed by adding a keygroup field 'translatable', on by default.
- Editing of keybindings in the Preferences dialog isn't "caught" completely by Geany. For example, if I remove a keybinding using click - BackSpace - Enter and press OK, Geany thinks I edited
nothing and does not update keybindings.conf. Though, as expected, the removed keybinding no longer functions.
I can't reproduce this as described.
I finished this patch today, so I hadn't much time for testing. As for now, everything except the two issues above seems to work fine.
Best regards, Eugene.
Sorry, that patch is a little outdated. Here is the last version.
Regards, Nick
Hi
On Wed, 8 Sep 2010 16:14:07 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Sun, 29 Aug 2010 21:59:32 +0400 Eugene Arshinov earshinov@gmail.com wrote:
>> If we do want snippet keybindings perhaps it would be >> better to integrate with the existing keybindings rather >> than storing them separately - then users could configure >> them in the normal way.
Here is the new patch. It seems to be not so simple and clean as the previous one, but now keybindings are edited in Preferences dialog as (I believe) most users prefer.
Unfortunately, keybindings_set_item was not enough for me to implement this. For example, I had to extend GeanyKeyGroup struct so that I have a way to tweak loading from / saving to keybindings.conf. Anyway, any suggestions about how to improve the patch are welcome.
OK, I didn't explain before what I had in mind but I think this patch is too complex.
What I think we could accept is setting up the snippet keybinding group size and items (like a plugin) when snippets.conf is read, using any defaults found in the file. I don't think we should write to snippets.conf. If the user wants to set the keybinding in snippets.conf, then they must remember that it won't be kept in sync with keybindings.conf. This should make the code simpler.
No, I didn't mean to read/save keybindings in snippets.conf. It is very strange that my patch does it, maybe I forgot to remove something from the code :) I'll recheck the patch, probably tomorrow.
The patch is complex because the list of snippets is somewhat used in two places: snippets.conf and keybindings.conf (now containing snippet keybindings). When one of each files is [re]loaded, the list of keybindings in Preferences should be updated. Maybe I used too complex logic for handling that…
Currently there are two little problems
- All underscores from keybinding names are removed (I wonder
why), so names of some "special" snippets are displayed incorrectly (**)
** and users are allowed to assign keybindings for "special" snippets
This is so a menu item string with underscores for mnemonics can be reused without adding a translation string but without displaying the underscore mnemonics for the keybindings dialog.
This could be fixed by adding a keygroup field 'translatable', on by default.
OK, I will implement it after we decide what to do with the overcomplexity mentioned above.
- Editing of keybindings in the Preferences dialog isn't "caught" completely by Geany. For example, if I remove a keybinding
using click - BackSpace - Enter and press OK, Geany thinks I edited nothing and does not update keybindings.conf. Though, as expected, the removed keybinding no longer functions.
I can't reproduce this as described.
I'll try to reproduce it with trunk.
I finished this patch today, so I hadn't much time for testing. As for now, everything except the two issues above seems to work fine.
Best regards, Eugene.
Sorry, that patch is a little outdated. Here is the last version.
Regards, Nick _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On Wed, 8 Sep 2010 23:47:28 +0400% Eugene Arshinov earshinov@gmail.com wrote:
Hi
On Wed, 8 Sep 2010 16:14:07 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Sun, 29 Aug 2010 21:59:32 +0400 Eugene Arshinov earshinov@gmail.com wrote:
> >> If we do want snippet keybindings perhaps it would be > >> better to integrate with the existing keybindings rather > >> than storing them separately - then users could > >> configure them in the normal way.
Here is the new patch. It seems to be not so simple and clean as the previous one, but now keybindings are edited in Preferences dialog as (I believe) most users prefer.
Unfortunately, keybindings_set_item was not enough for me to implement this. For example, I had to extend GeanyKeyGroup struct so that I have a way to tweak loading from / saving to keybindings.conf. Anyway, any suggestions about how to improve the patch are welcome.
OK, I didn't explain before what I had in mind but I think this patch is too complex.
What I think we could accept is setting up the snippet keybinding group size and items (like a plugin) when snippets.conf is read, using any defaults found in the file. I don't think we should write to snippets.conf. If the user wants to set the keybinding in snippets.conf, then they must remember that it won't be kept in sync with keybindings.conf. This should make the code simpler.
No, I didn't mean to read/save keybindings in snippets.conf. It is very strange that my patch does it, maybe I forgot to remove something from the code :) I'll recheck the patch, probably tomorrow.
OK, it does not read keybindings from snippets.conf, only snippet names. Maybe function names are confusing: if you look at load_snippet_keybindings_from_snippets, you may think it really loads keybindings, but it doesn't :) Instead it reads just snippet names and accordingly updates list of snippet keybindings available in Preferences.
The patch is complex because the list of snippets is somewhat used in two places: snippets.conf and keybindings.conf (now containing snippet keybindings). When one of each files is [re]loaded, the list of keybindings in Preferences should be updated. Maybe I used too complex logic for handling that…
Currently there are two little problems
- All underscores from keybinding names are removed (I wonder
why), so names of some "special" snippets are displayed incorrectly (**)
** and users are allowed to assign keybindings for "special" snippets
This is so a menu item string with underscores for mnemonics can be reused without adding a translation string but without displaying the underscore mnemonics for the keybindings dialog.
This could be fixed by adding a keygroup field 'translatable', on by default.
OK, I will implement it after we decide what to do with the overcomplexity mentioned above.
- Editing of keybindings in the Preferences dialog isn't
"caught" completely by Geany. For example, if I remove a keybinding using click - BackSpace - Enter and press OK, Geany thinks I edited nothing and does not update keybindings.conf. Though, as expected, the removed keybinding no longer functions.
I can't reproduce this as described.
I'll try to reproduce it with trunk.
Works fine :) Probably my mistake.
I finished this patch today, so I hadn't much time for testing. As for now, everything except the two issues above seems to work fine.
Best regards, Eugene.
Sorry, that patch is a little outdated. Here is the last version.
I attach slightly updated version, with 2 code comments updated and 1 unneeded function parameter removed.
Best regards, Eugene.
On Thu, 9 Sep 2010 11:01:04 +0400 Eugene Arshinov earshinov@gmail.com wrote:
OK, I didn't explain before what I had in mind but I think this patch is too complex.
What I think we could accept is setting up the snippet keybinding group size and items (like a plugin) when snippets.conf is read, using any defaults found in the file. I don't think we should write to snippets.conf. If the user wants to set the keybinding in snippets.conf, then they must remember that it won't be kept in sync with keybindings.conf. This should make the code simpler.
No, I didn't mean to read/save keybindings in snippets.conf. It is very strange that my patch does it, maybe I forgot to remove something from the code :) I'll recheck the patch, probably tomorrow.
OK, it does not read keybindings from snippets.conf, only snippet names. Maybe function names are confusing: if you look at load_snippet_keybindings_from_snippets, you may think it really loads keybindings, but it doesn't :) Instead it reads just snippet names and accordingly updates list of snippet keybindings available in Preferences.
BTW I replied to your first email before I saw this, sorry.
IIUC, I think it would make the code much simpler if the user has to say which snippets they want keybindings for, not offering all snippets for keybindings.
Regards, Nick
On Thu, 9 Sep 2010 11:41:10 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Thu, 9 Sep 2010 11:01:04 +0400 Eugene Arshinov earshinov@gmail.com wrote:
OK, I didn't explain before what I had in mind but I think this patch is too complex.
What I think we could accept is setting up the snippet keybinding group size and items (like a plugin) when snippets.conf is read, using any defaults found in the file. I don't think we should write to snippets.conf. If the user wants to set the keybinding in snippets.conf, then they must remember that it won't be kept in sync with keybindings.conf. This should make the code simpler.
No, I didn't mean to read/save keybindings in snippets.conf. It is very strange that my patch does it, maybe I forgot to remove something from the code :) I'll recheck the patch, probably tomorrow.
OK, it does not read keybindings from snippets.conf, only snippet names. Maybe function names are confusing: if you look at load_snippet_keybindings_from_snippets, you may think it really loads keybindings, but it doesn't :) Instead it reads just snippet names and accordingly updates list of snippet keybindings available in Preferences.
BTW I replied to your first email before I saw this, sorry.
IIUC, I think it would make the code much simpler if the user has to say which snippets they want keybindings for, not offering all snippets for keybindings.
Maybe, but what will the interface look like?
On Fri, 10 Sep 2010 11:11:02 +0400 Eugene Arshinov earshinov@gmail.com wrote:
IIUC, I think it would make the code much simpler if the user has to say which snippets they want keybindings for, not offering all snippets for keybindings.
Maybe, but what will the interface look like?
I thought it would just list the snippets asked for.
Anyway, I may be wrong about simplifying things, as I said, I haven't really studied the patch.
Sorry that you've put quite a lot of work into this, but I see snippet keybindings as an extra thing which most people might just type the snippet name, so I think the implementation has to be quite simple to justify maintaining the change.
The original patch may be simpler, maybe it's best not to have to read snippet keybindings from keybindings.conf...
Regards, Nick
On Fri, 10 Sep 2010 12:38:57 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Fri, 10 Sep 2010 11:11:02 +0400 Eugene Arshinov earshinov@gmail.com wrote:
IIUC, I think it would make the code much simpler if the user has to say which snippets they want keybindings for, not offering all snippets for keybindings.
Maybe, but what will the interface look like?
I thought it would just list the snippets asked for.
Anyway, I may be wrong about simplifying things, as I said, I haven't really studied the patch.
Sorry that you've put quite a lot of work into this, but I see snippet keybindings as an extra thing which most people might just type the snippet name, so I think the implementation has to be quite simple to justify maintaining the change.
Speaking about maintaining the change, maybe it's worth extracting snippet keybindings to a separate plugin? I think it can be done. In Geany itself I'll need a new signal on loading of snippets.conf, a function to activate a snippet by name, and a new function to set a keygroup with load and save callbacks.
The original patch may be simpler, maybe it's best not to have to read snippet keybindings from keybindings.conf...
Yes, it's simpler. But, on the opposite side, there a user must edit keybindings manually (probably with Preferences > Keybindings opened in order to determine the text representation of the needed keybinding) and she can't get "keybinding is already used" notifications. Though, most users will rarely (if ever) need snippet keybindings.
And, storing snippet keybindings in keybindings.conf, like all other keybindings, seems more natural to me, that saving them to snippets.conf. Though, AFAIK, Gedit Snippets plugin saves keybindings together with snippets.
Hard to justify :)
Best regards, Eugene.
Hi again.
Maybe we can incorporate the first patch into Geany? "Power users" who need snippet keybindings will be able to specify keybindings manually in snippets.conf, just like hidden prefs in geany.conf. This will keep the implementation simple, and ordinary users mostly likely do not need the keybindings.
Best regards, Eugene.
On 28 September 2010 17:34, Eugene Arshinov earshinov@gmail.com wrote:
Hi again.
Maybe we can incorporate the first patch into Geany? "Power users" who need snippet keybindings will be able to specify keybindings manually in snippets.conf, just like hidden prefs in geany.conf. This will keep the implementation simple, and ordinary users mostly likely do not need the keybindings.
Makes sense if documented :-D
Cheers Lex
Best regards, Eugene. _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On Tue, 28 Sep 2010 17:57:27 +1000% Lex Trotman elextr@gmail.com wrote:
On 28 September 2010 17:34, Eugene Arshinov earshinov@gmail.com wrote:
Hi again.
Maybe we can incorporate the first patch into Geany? "Power users" who need snippet keybindings will be able to specify keybindings manually in snippets.conf, just like hidden prefs in geany.conf. This will keep the implementation simple, and ordinary users mostly likely do not need the keybindings.
Makes sense if documented :-D
Cheers Lex
Agree :)
On Tue, 28 Sep 2010 11:34:10 +0400 Eugene Arshinov earshinov@gmail.com wrote:
Maybe we can incorporate the first patch into Geany? "Power users" who need snippet keybindings will be able to specify keybindings manually in snippets.conf, just like hidden prefs in geany.conf. This will keep the implementation simple, and ordinary users mostly likely do not need the keybindings.
Now committed but changed so the keybindings are only read from the user keyfile. Thanks for the patches.
Regards, Nick
P.S. Also I finally commented on the other two patches submitted at the time, check the list.
On Fri, 1 Oct 2010 16:12:40 +0100 Nick Treleaven nick.treleaven@btinternet.com wrote:
Maybe we can incorporate the first patch into Geany? "Power users" who need snippet keybindings will be able to specify keybindings manually in snippets.conf, just like hidden prefs in geany.conf. This will keep the implementation simple, and ordinary users mostly likely do not need the keybindings.
Now committed but changed so the keybindings are only read from the user keyfile. Thanks for the patches.
The change was mainly to make editor_snippets_init() shorter for easier understanding. Now system keyfile snippet keybindings are supported too.
Regards, Nick
Hi Nick
On Tue, 5 Oct 2010 12:10:08 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Fri, 1 Oct 2010 16:12:40 +0100 Nick Treleaven nick.treleaven@btinternet.com wrote:
Maybe we can incorporate the first patch into Geany? "Power users" who need snippet keybindings will be able to specify keybindings manually in snippets.conf, just like hidden prefs in geany.conf. This will keep the implementation simple, and ordinary users mostly likely do not need the keybindings.
Now committed but changed so the keybindings are only read from the user keyfile. Thanks for the patches.
The change was mainly to make editor_snippets_init() shorter for easier understanding. Now system keyfile snippet keybindings are supported too.
Thanks for committing and improving the patch.
These is a little flaw in current editor.c:load_kb() function. It will probably segfault if snippets.conf does not contain Keybindings section. You should check if (keys != NULL) before doing foreach_strv(ptr, keys).
Best regards, Eugene.
On Fri, 8 Oct 2010 05:41:00 +0400 Eugene Arshinov earshinov@gmail.com wrote:
The change was mainly to make editor_snippets_init() shorter for easier understanding. Now system keyfile snippet keybindings are supported too.
Thanks for committing and improving the patch.
These is a little flaw in current editor.c:load_kb() function. It will probably segfault if snippets.conf does not contain Keybindings section. You should check if (keys != NULL) before doing foreach_strv(ptr, keys).
foreach_strv is null safe since that commit ;-)
Regards, Nick
On Fri, 8 Oct 2010 15:47:44 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Fri, 8 Oct 2010 05:41:00 +0400 Eugene Arshinov earshinov@gmail.com wrote:
The change was mainly to make editor_snippets_init() shorter for easier understanding. Now system keyfile snippet keybindings are supported too.
Thanks for committing and improving the patch.
These is a little flaw in current editor.c:load_kb() function. It will probably segfault if snippets.conf does not contain Keybindings section. You should check if (keys != NULL) before doing foreach_strv(ptr, keys).
foreach_strv is null safe since that commit ;-)
Well, then it's another thing I missed :-)
On Wed, 8 Sep 2010 23:47:28 +0400 Eugene Arshinov earshinov@gmail.com wrote:
Unfortunately, keybindings_set_item was not enough for me to implement this. For example, I had to extend GeanyKeyGroup struct so that I have a way to tweak loading from / saving to keybindings.conf. Anyway, any suggestions about how to improve the patch are welcome.
OK, I didn't explain before what I had in mind but I think this patch is too complex.
What I think we could accept is setting up the snippet keybinding group size and items (like a plugin) when snippets.conf is read, using any defaults found in the file. I don't think we should write to snippets.conf. If the user wants to set the keybinding in snippets.conf, then they must remember that it won't be kept in sync with keybindings.conf. This should make the code simpler.
No, I didn't mean to read/save keybindings in snippets.conf. It is very strange that my patch does it, maybe I forgot to remove something from the code :) I'll recheck the patch, probably tomorrow.
I didn't study the patch much, but you said 'I have a way to tweak loading from / saving to keybindings.conf'.
The patch is complex because the list of snippets is somewhat used in two places: snippets.conf and keybindings.conf (now containing snippet keybindings). When one of each files is [re]loaded, the list of keybindings in Preferences should be updated. Maybe I used too complex logic for handling that…
I may be wrong, but why does any keybinding code need to know about snippets? I thought when snippets.conf is read or re-read, we would just resize the keybinding group and initialize each keybinding based on the keys listed in a snippets.conf [keybindings] section. (Just like a plugin would).
I'm not sure why editor.c needs to cache the keybindings either. Maybe I'm missing something.
Regards, Nick
On Thu, 9 Sep 2010 11:35:18 +0100 Nick Treleaven nick.treleaven@btinternet.com wrote:
No, I didn't mean to read/save keybindings in snippets.conf. It is very strange that my patch does it, maybe I forgot to remove something from the code :) I'll recheck the patch, probably tomorrow.
I didn't study the patch much, but you said 'I have a way to tweak loading from / saving to keybindings.conf'.
Oops. You didn't say snippets.conf, my mistake ;-)
Regards, Nick
On Sat, 10 Jul 2010 21:48:35 +0400 Eugene Arshinov earshinov@gmail.com wrote:
Hi all.
I attach three patches:
- Keybindings to insert new line after/before current
...
- Automatically insert additional indentation in XML/HTML files if
previous line ends with an opening tag
I finally got round to replying to the other patches, sorry for the delay.
The drawback of the first patch is that I doubt it's so useful. I know Vi[m] provides shortcuts for these actions ('o' and 'O' in normal mode), but their necessity is questionable for me. I implemented this to ease XML editing with tag autocompletion turned on. Suppose you need to write to consequent <li>'s. Without this patch you need to press End+Enter after writing the first <li> to place the cursor where you need to start the second. With my patch, you only need to press one shortcut you can assign in Preferences > Keybindings :)
Maybe we could add these. BTW you could use utils_get_eol_char(). Also we already have sci_get_line_indent_position().
The drawback of the third patch is that it's not completed. If user likes to leave HTML tags like <br> "unclosed", she would be disturbed by automatic indentation caused by my patch, so a check box in Preferences is desirable. I'll code it as soon as we decide this patch can go to trunk.
For HTML perhaps we could have a filetype pref for this.
Regards, Nick
On Thu, 30 Sep 2010 13:23:58 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Sat, 10 Jul 2010 21:48:35 +0400 Eugene Arshinov earshinov@gmail.com wrote:
Hi all.
I attach three patches:
- Keybindings to insert new line after/before current
...
- Automatically insert additional indentation in XML/HTML files if
previous line ends with an opening tag
I finally got round to replying to the other patches, sorry for the delay.
The drawback of the first patch is that I doubt it's so useful. I know Vi[m] provides shortcuts for these actions ('o' and 'O' in normal mode), but their necessity is questionable for me. I implemented this to ease XML editing with tag autocompletion turned on. Suppose you need to write to consequent <li>'s. Without this patch you need to press End+Enter after writing the first <li> to place the cursor where you need to start the second. With my patch, you only need to press one shortcut you can assign in Preferences > Keybindings :)
Maybe we could add these. BTW you could use utils_get_eol_char(). Also we already have sci_get_line_indent_position().
I missed this function… Updated patch attached.
The drawback of the third patch is that it's not completed. If user likes to leave HTML tags like <br> "unclosed", she would be disturbed by automatic indentation caused by my patch, so a check box in Preferences is desirable. I'll code it as soon as we decide this patch can go to trunk.
For HTML perhaps we could have a filetype pref for this.
What should it look like? I can't name this pref "autoindent" because it would be confusing if for XML and HTML it only controls the indentation after XML/HTML tags, not after braces in PHP/JS chunks. If I make the pref more specific (e.g., "xml-autoindent"), it won't probably be quite proper to insert such a specific member to GeanyFiletype struct.
Maybe it's more appropriate to add a check button near "Preferences > Editor > Indentation > Auto-indent mode" list? AFAIK (never used it), "Match braces" mode works only for braces languages and thus is already somewhat filetype-specific.
For the present, I attach an updated patch which doesn't insert indentation after "short" HTML tags. I also modified existing tag autocompletion code so that it doesn't check for HTML tags if current lexer is XML, not HTML. I slightly modified utils_find_open_xml_tag() in order to reuse it in my autoindentation code. Particularly I removed `check_tag' parameter and strange condition
else if (! check_tag && *cur == '>') break;
I'm not sure why this condition was needed there.
Best regards, Eugene.
On Fri, 8 Oct 2010 06:03:33 +0400 Eugene Arshinov earshinov@gmail.com wrote:
The drawback of the first patch is that I doubt it's so useful. I know Vi[m] provides shortcuts for these actions ('o' and 'O' in normal mode), but their necessity is questionable for me. I implemented this to ease XML editing with tag autocompletion turned on. Suppose you need to write to consequent <li>'s. Without this patch you need to press End+Enter after writing the first <li> to place the cursor where you need to start the second. With my patch, you only need to press one shortcut you can assign in Preferences > Keybindings :)
Maybe we could add these. BTW you could use utils_get_eol_char(). Also we already have sci_get_line_indent_position().
I missed this function… Updated patch attached.
Thanks. Committed with changes - moved keybindings to the Insert group and changed implementation a bit.
Nick
On Fri, 15 Oct 2010 18:45:11 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Fri, 8 Oct 2010 06:03:33 +0400 Eugene Arshinov earshinov@gmail.com wrote:
The drawback of the first patch is that I doubt it's so useful. I know Vi[m] provides shortcuts for these actions ('o' and 'O' in normal mode), but their necessity is questionable for me. I implemented this to ease XML editing with tag autocompletion turned on. Suppose you need to write to consequent <li>'s. Without this patch you need to press End+Enter after writing the first <li> to place the cursor where you need to start the second. With my patch, you only need to press one shortcut you can assign in Preferences > Keybindings :)
Maybe we could add these. BTW you could use utils_get_eol_char(). Also we already have sci_get_line_indent_position().
I missed this function… Updated patch attached.
Thanks. Committed with changes - moved keybindings to the Insert group and changed implementation a bit.
Thanks for committing. Your implementation using sci_send_command() indeed looks better than mine.
On Fri, 8 Oct 2010 06:03:33 +0400 Eugene Arshinov earshinov@gmail.com wrote:
The drawback of the third patch is that it's not completed. If user likes to leave HTML tags like <br> "unclosed", she would be disturbed by automatic indentation caused by my patch, so a check box in Preferences is desirable. I'll code it as soon as we decide this patch can go to trunk.
For HTML perhaps we could have a filetype pref for this.
What should it look like? I can't name this pref "autoindent" because it would be confusing if for XML and HTML it only controls the indentation after XML/HTML tags, not after braces in PHP/JS chunks. If I make the pref more specific (e.g., "xml-autoindent"), it won't probably be quite proper to insert such a specific member to GeanyFiletype struct.
Maybe it's more appropriate to add a check button near "Preferences > Editor > Indentation > Auto-indent mode" list? AFAIK (never used it), "Match braces" mode works only for braces languages and thus is already somewhat filetype-specific.
For the present, I attach an updated patch which doesn't insert indentation after "short" HTML tags.
OK, looks like a good solution.
Committed patch, but I disabled autoindentation if tag autoclosing is enabled, because the two features don't really work well together.
E.g. typing <table> adds </table> after the cursor; what if the user wants to put the closing tag on a newline? They press enter and the closing tag is indented, which is not wanted.
I also modified existing tag autocompletion code so that it doesn't check for HTML tags if current lexer is XML, not HTML. I slightly modified utils_find_open_xml_tag() in order to reuse it in my autoindentation code. Particularly I removed `check_tag' parameter and strange condition
else if (! check_tag && *cur == '>') break;
I'm not sure why this condition was needed there.
Was it necessary to remove it? Just checking as we should leave it otherwise. (Currently the change is applied).
Nick
On Mon, 25 Oct 2010 18:01:25 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Fri, 8 Oct 2010 06:03:33 +0400 Eugene Arshinov earshinov@gmail.com wrote:
The drawback of the third patch is that it's not completed. If user likes to leave HTML tags like <br> "unclosed", she would be disturbed by automatic indentation caused by my patch, so a check box in Preferences is desirable. I'll code it as soon as we decide this patch can go to trunk.
For HTML perhaps we could have a filetype pref for this.
What should it look like? I can't name this pref "autoindent" because it would be confusing if for XML and HTML it only controls the indentation after XML/HTML tags, not after braces in PHP/JS chunks. If I make the pref more specific (e.g., "xml-autoindent"), it won't probably be quite proper to insert such a specific member to GeanyFiletype struct.
Maybe it's more appropriate to add a check button near "Preferences
Editor > Indentation > Auto-indent mode" list? AFAIK (never used it),
"Match braces" mode works only for braces languages and thus is already somewhat filetype-specific.
For the present, I attach an updated patch which doesn't insert indentation after "short" HTML tags.
OK, looks like a good solution.
Do I understand correctly that you think we don't need GUI preference? If you do, I agree :)
Committed patch, but I disabled autoindentation if tag autoclosing is enabled, because the two features don't really work well together.
E.g. typing <table> adds </table> after the cursor; what if the user wants to put the closing tag on a newline? They press enter and the closing tag is indented, which is not wanted.
Agree.
I also modified existing tag autocompletion code so that it doesn't check for HTML tags if current lexer is XML, not HTML. I slightly modified utils_find_open_xml_tag() in order to reuse it in my autoindentation code. Particularly I removed `check_tag' parameter and strange condition
else if (! check_tag && *cur == '>') break;
I'm not sure why this condition was needed there.
Was it necessary to remove it? Just checking as we should leave it otherwise. (Currently the change is applied).
No, technically it wasn't necessary. The removal just allows to prune check_tag argument and make the function and it's usage a bit simpler.
Maybe you're right, we should keep the condition, but I think we should change it to just "*cur == '>'" (so the check_tag argument still can be removed).
The condition may become true only for invalid HTML: it should contain two > > without < between them (follow the code of utils_find_open_xml_tag). The condition guarantees that if we have such an erroneous HTML line, no close tag will be automatically inserted. For example, it is helpful when you write "<script>if (a >" with XML tag autocompletion turned on.
In previous email I wrote this condition is strange as I can't understand why we account `check_tag' argument here (i.e., why not just check for *cur == '>'). Basically, in original editor.c:handle_xml check_tag was set to
0 if we autocomplete after > 1 if we autocomplete after </
What's the difference for the condition we discuss, if we always start backward search for an open tag _before_ those ">" and "</" chunks?
By the way, the code of utils_find_open_xml_tag is quite old (committed in 2005, rev. 4 "Initial import").
Best regards, Eugene.
On Tue, 26 Oct 2010 12:14:25 +0400 Eugene Arshinov earshinov@gmail.com wrote:
For the present, I attach an updated patch which doesn't insert indentation after "short" HTML tags.
OK, looks like a good solution.
Do I understand correctly that you think we don't need GUI preference?
I think we could add a per-filetype indent mode option so it would be easy to disable in case someone wanted to. The user could then set auto-indent to basic for HTML.
I also modified existing tag autocompletion code so that it doesn't check for HTML tags if current lexer is XML, not HTML. I slightly modified utils_find_open_xml_tag() in order to reuse it in my autoindentation code. Particularly I removed `check_tag' parameter and strange condition
else if (! check_tag && *cur == '>') break;
I'm not sure why this condition was needed there.
Was it necessary to remove it? Just checking as we should leave it otherwise. (Currently the change is applied).
No, technically it wasn't necessary. The removal just allows to prune check_tag argument and make the function and it's usage a bit simpler.
Yes, I forgot that ;-)
Maybe you're right, we should keep the condition, but I think we should change it to just "*cur == '>'" (so the check_tag argument still can be removed).
OK, done.
The condition may become true only for invalid HTML: it should contain two > > without < between them (follow the code of utils_find_open_xml_tag). The condition guarantees that if we have such an erroneous HTML line, no close tag will be automatically inserted. For example, it is helpful when you write "<script>if (a >" with XML tag autocompletion turned on.
In previous email I wrote this condition is strange as I can't understand why we account `check_tag' argument here (i.e., why not just check for *cur == '>'). Basically, in original editor.c:handle_xml check_tag was set to
0 if we autocomplete after > 1 if we autocomplete after </
What's the difference for the condition we discuss, if we always start backward search for an open tag _before_ those ">" and "</" chunks?
Not sure. Let me know if there are any problems.
Nick
On Wed, 27 Oct 2010 17:16:59 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Tue, 26 Oct 2010 12:14:25 +0400 Eugene Arshinov earshinov@gmail.com wrote:
For the present, I attach an updated patch which doesn't insert indentation after "short" HTML tags.
OK, looks like a good solution.
Do I understand correctly that you think we don't need GUI preference?
I think we could add a per-filetype indent mode option so it would be easy to disable in case someone wanted to. The user could then set auto-indent to basic for HTML.
I assume it's necessary to give users an opportunity to control automatic indentation of tags (HTML) and braces (JS/PHP) independently. I suggest making the new "indent_mode" option a combination of flags, each of which control what indent feature will be _disabled_. That this, the default for any filetype will be 0 (all features enabled). For filetypes which aren't HTML users will add 1 to disable autoindentation. For HTML they will add 1 to disable JS/PHP autoindentation and 2 to disable tag autoindentation. I "inverse" the semantics of flags, so that when an indent feature is added it will be turned on by default. Maybe it's better to name this option "disable_indent" and use strings instead of numbers. For example, "disable_indent=braces,tags" for HTML would disable autoindentation completely. Any thoughts/suggestions?
Best regards, Eugene.
On Thu, 28 Oct 2010 23:27:53 +0400 Eugene Arshinov earshinov@gmail.com wrote:
I think we could add a per-filetype indent mode option so it would be easy to disable in case someone wanted to. The user could then set auto-indent to basic for HTML.
I assume it's necessary to give users an opportunity to control automatic indentation of tags (HTML) and braces (JS/PHP) independently. I suggest making the new "indent_mode" option a
I wasn't thinking of brace autoindentation. In that case perhaps it's simpler to add a HTML-specific setting to disable tag autoindentation.
Nick
On Fri, 29 Oct 2010 15:31:37 +0100% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Thu, 28 Oct 2010 23:27:53 +0400 Eugene Arshinov earshinov@gmail.com wrote:
I think we could add a per-filetype indent mode option so it would be easy to disable in case someone wanted to. The user could then set auto-indent to basic for HTML.
I assume it's necessary to give users an opportunity to control automatic indentation of tags (HTML) and braces (JS/PHP) independently. I suggest making the new "indent_mode" option a
I wasn't thinking of brace autoindentation. In that case perhaps it's simpler to add a HTML-specific setting to disable tag autoindentation.
It makes sense, but since now I could not find an acceptable way to implement it. I've just discovered that GeanyFiletype's are stored in a pointer array, so instead of ordinary GeanyFiletype for HTML we may add to the array an instance of hypothetical HtmlFiletype { GeanyFiletype parent; gboolean enable_tag_autoindentation } and of course write some code to load enable_tag_autoindentation from filetype config. Is it OK?
Best regards, Eugene.
On Fri, 29 Oct 2010 21:56:53 +0400 Eugene Arshinov earshinov@gmail.com wrote:
I wasn't thinking of brace autoindentation. In that case perhaps it's simpler to add a HTML-specific setting to disable tag autoindentation.
It makes sense, but since now I could not find an acceptable way to implement it. I've just discovered that GeanyFiletype's are stored in a pointer array, so instead of ordinary GeanyFiletype for HTML we may add to the array an instance of hypothetical HtmlFiletype { GeanyFiletype parent; gboolean enable_tag_autoindentation } and of course write some code to load enable_tag_autoindentation from filetype config. Is it OK?
I think just add a html_tag_autoindentation field to GeanyFiletypePrivate and use that.
Nick
On Tue, 2 Nov 2010 15:13:12 +0000% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Fri, 29 Oct 2010 21:56:53 +0400 Eugene Arshinov earshinov@gmail.com wrote:
I wasn't thinking of brace autoindentation. In that case perhaps it's simpler to add a HTML-specific setting to disable tag autoindentation.
It makes sense, but since now I could not find an acceptable way to implement it. I've just discovered that GeanyFiletype's are stored in a pointer array, so instead of ordinary GeanyFiletype for HTML we may add to the array an instance of hypothetical HtmlFiletype { GeanyFiletype parent; gboolean enable_tag_autoindentation } and of course write some code to load enable_tag_autoindentation from filetype config. Is it OK?
I think just add a html_tag_autoindentation field to GeanyFiletypePrivate and use that.
Okay, the patch is attached. I named the field 'xml_indent_tags' and the filetype property 'indent_tags'. By default (like all fields in GeanyFiletypePrivate) it is zero, but in global filetypes.{xml,html} I set it to true.
Best regards, Eugene.
On Tue, 2 Nov 2010 23:28:43 +0300 Eugene Arshinov earshinov@gmail.com wrote:
I think just add a html_tag_autoindentation field to GeanyFiletypePrivate and use that.
Okay, the patch is attached.
Thanks, applied (with some changes).
I named the field 'xml_indent_tags' and the filetype property 'indent_tags'. By default (like all fields in GeanyFiletypePrivate) it is zero, but in global filetypes.{xml,html} I set it to true.
I changed the filetype property to xml_indent_tags also, I think it's clearer.
I changed the code slightly to check whether the document uses the HTML/XML lexer rather than those filetypes, and to always read the property. This should allow other filetypes that have XML syntax to work also if the property is set.
BTW, should we set xml_indent_tags=true for PHP, Docbook filetypes?
Nick
On Wed, 10 Nov 2010 17:51:18 +0000% Nick Treleaven nick.treleaven@btinternet.com wrote:
On Tue, 2 Nov 2010 23:28:43 +0300 Eugene Arshinov earshinov@gmail.com wrote:
I think just add a html_tag_autoindentation field to GeanyFiletypePrivate and use that.
Okay, the patch is attached.
Thanks, applied (with some changes).
I named the field 'xml_indent_tags' and the filetype property 'indent_tags'. By default (like all fields in GeanyFiletypePrivate) it is zero, but in global filetypes.{xml,html} I set it to true.
I changed the filetype property to xml_indent_tags also, I think it's clearer.
I changed the code slightly to check whether the document uses the HTML/XML lexer rather than those filetypes, and to always read the property. This should allow other filetypes that have XML syntax to work also if the property is set.
Agree. Thanks for update and commit.
BTW, should we set xml_indent_tags=true for PHP, Docbook filetypes?
I think, yes, we should. I attach a patch that have this changes plus some documentation in geany.txt (if you use this patch, please regenerate geany.html yourself as my rst2html causes most of this file to be changed).
Best regards, Eugene.
On Wed, 10 Nov 2010 22:44:43 +0300 Eugene Arshinov earshinov@gmail.com wrote:
I changed the code slightly to check whether the document uses the HTML/XML lexer rather than those filetypes, and to always read the property. This should allow other filetypes that have XML syntax to work also if the property is set.
Agree. Thanks for update and commit.
BTW, should we set xml_indent_tags=true for PHP, Docbook filetypes?
I think, yes, we should. I attach a patch that have this changes plus some documentation in geany.txt (if you use this patch, please regenerate geany.html yourself as my rst2html causes most of this file to be changed).
Thanks, applied.
Nick