Hi.
When "Escape sequences" checkbox is checked in Find or Replace dialog, before escaping the find and replace strings we should save the original ones, so that they can be used in messages shown to user and in the history of those dialogs' entries. This is what the patch is about.
I described in the commit message (see the top of the patch) the changes made in the code. Just in case, here is a copy:
In the code, now we pass the original text together with the one that is actually searched for. New `original_text' field was added to GeanySearchData. A bug was fixed in document.c:show_replace_summary(): it did not escape the "No matches found for ..." string.
Hope that I did not touch plugin API with these changes. In header files I changed the following: - document_find_text()'s signature - document_replace_text()'s signature - search_find_usage()'s signature - struct GeanySearchData
-- Best regards, Eugene.
Le 15/05/2011 14:29, Eugene Arshinov a écrit :
Hi.
When "Escape sequences" checkbox is checked in Find or Replace dialog, before escaping the find and replace strings we should save the original ones, so that they can be used in messages shown to user and in the history of those dialogs' entries. This is what the patch is about.
I described in the commit message (see the top of the patch) the changes made in the code. Just in case, here is a copy:
In the code, now we pass the original text together with the one that is actually searched for. New `original_text' field was added to GeanySearchData. A bug was fixed in document.c:show_replace_summary(): it did not escape the "No matches found for ..." string.
Hope that I did not touch plugin API with these changes. In header files I changed the following:
- document_find_text()'s signature
- document_replace_text()'s signature
- search_find_usage()'s signature
- struct GeanySearchData
Seems none of the functions you changed were in the plugin API, so it's fine. For the structure, since you appended to it, it's also fine.
However, a few comments:
1) why did you used an extra string everywhere but in document.c:show_replace_summary()? OK it already do this, but it seems not consistent since you added an "original_text".
2) on search.c:on_replace_dialog_response(), prefer initialize original_find to NULL and free it in fail rather than freeing it every time. (though anyway this function is ugly as-is with the backward goto jump just for failure)
Otherwise looks OK to me.
Regards, Colomban
On Tue, 17 May 2011 15:38:40 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 15/05/2011 14:29, Eugene Arshinov a écrit :
Hi.
When "Escape sequences" checkbox is checked in Find or Replace dialog, before escaping the find and replace strings we should save the original ones, so that they can be used in messages shown to user and in the history of those dialogs' entries. This is what the patch is about.
I described in the commit message (see the top of the patch) the changes made in the code. Just in case, here is a copy:
In the code, now we pass the original text together with the one that is actually searched for. New `original_text' field was added to GeanySearchData. A bug was fixed in document.c:show_replace_summary(): it did not escape the "No matches found for ..." string.
Hope that I did not touch plugin API with these changes. In header files I changed the following:
- document_find_text()'s signature
- document_replace_text()'s signature
- search_find_usage()'s signature
- struct GeanySearchData
Seems none of the functions you changed were in the plugin API, so it's fine. For the structure, since you appended to it, it's also fine.
However, a few comments:
- why did you used an extra string everywhere but in
document.c:show_replace_summary()? OK it already do this, but it seems not consistent since you added an "original_text".
- on search.c:on_replace_dialog_response(), prefer initialize
original_find to NULL and free it in fail rather than freeing it every time. (though anyway this function is ugly as-is with the backward goto jump just for failure)
Otherwise looks OK to me.
Attached are the original patch updated for current trunk (r5798) and another one containing changes according to your advice. In the second patch I also applied the same "technique" to show proper replace text in messages.
-- Best regards, Eugene
On Sat, 21 May 2011 12:16:59 +0400 Eugene Arshinov earshinov@gmail.com wrote:
On Tue, 17 May 2011 15:38:40 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 15/05/2011 14:29, Eugene Arshinov a écrit :
Hi.
When "Escape sequences" checkbox is checked in Find or Replace dialog, before escaping the find and replace strings we should save the original ones, so that they can be used in messages shown to user and in the history of those dialogs' entries. This is what the patch is about.
I described in the commit message (see the top of the patch) the changes made in the code. Just in case, here is a copy:
In the code, now we pass the original text together with the one that is actually searched for. New `original_text' field was added to GeanySearchData. A bug was fixed in document.c:show_replace_summary(): it did not escape the "No matches found for ..." string.
Hope that I did not touch plugin API with these changes. In header files I changed the following:
- document_find_text()'s signature
- document_replace_text()'s signature
- search_find_usage()'s signature
- struct GeanySearchData
Seems none of the functions you changed were in the plugin API, so it's fine. For the structure, since you appended to it, it's also fine.
However, a few comments:
- why did you used an extra string everywhere but in
document.c:show_replace_summary()? OK it already do this, but it seems not consistent since you added an "original_text".
- on search.c:on_replace_dialog_response(), prefer initialize
original_find to NULL and free it in fail rather than freeing it every time. (though anyway this function is ugly as-is with the backward goto jump just for failure)
Otherwise looks OK to me.
Attached are the original patch updated for current trunk (r5798) and another one containing changes according to your advice. In the second patch I also applied the same "technique" to show proper replace text in messages.
Hi.
Aren't these patches forgotten?
Hi,
Le 03/06/2011 09:47, Eugene Arshinov a écrit :
On Sat, 21 May 2011 12:16:59 +0400 Eugene Arshinov earshinov@gmail.com wrote: [...]
Hi.
Aren't these patches forgotten?
Not really forgotten, but since I don't find that much time these days to review patches & stuff (you might have noticed ^^), I didn't reviewed/applied them yet, sorry :/ (moreover most work was already done for this one, shame on me)
Thanks for pushing me up ^^ So it's committed now. I also pushed another small commit to use the original text in combo box history so it becomes really usable when there are escape sequences in the search or replace.
Thanks for the patch, and sorry again for the delay.
Cheers, Colomban
On Fri, 03 Jun 2011 15:44:46 +0200, Colomban wrote:
Hi,
Le 03/06/2011 09:47, Eugene Arshinov a écrit :
On Sat, 21 May 2011 12:16:59 +0400 Eugene Arshinov earshinov@gmail.com wrote: [...]
Hi.
Aren't these patches forgotten?
Not really forgotten, but since I don't find that much time these days to review patches & stuff (you might have noticed ^^), I didn't
Similar here. Spare time is rare an currently, I focus my work on the new wiki to get this done ASAP.
Sorry.
So it's committed now. I also pushed another small commit to use the
Yay, thanks Colomban.
Regards, Enrico
On Fri, 3 Jun 2011 15:48:48 +0200 Enrico Tröger enrico.troeger@uvena.de wrote:
On Fri, 03 Jun 2011 15:44:46 +0200, Colomban wrote:
Hi,
Le 03/06/2011 09:47, Eugene Arshinov a écrit :
On Sat, 21 May 2011 12:16:59 +0400 Eugene Arshinov earshinov@gmail.com wrote: [...]
Hi.
Aren't these patches forgotten?
Not really forgotten, but since I don't find that much time these days to review patches & stuff (you might have noticed ^^), I didn't
Similar here. Spare time is rare an currently, I focus my work on the new wiki to get this done ASAP.
Sorry.
So it's committed now. I also pushed another small commit to use the
Yay, thanks Colomban.
No problem, guys. I just wanted to remind that these patches exist. I don't really care much if they are committed in two weeks or two months. Thanks for committing and for the little additional patch.
-- Best regards, Eugene