After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 638134 - Redesign of Single and Multi-File Search & Replace
Redesign of Single and Multi-File Search & Replace
Status: RESOLVED FIXED
Product: anjuta
Classification: Applications
Component: plugins: document-manager
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Anjuta maintainers
Anjuta maintainers
Depends on:
Blocks:
 
 
Reported: 2010-12-27 20:43 UTC by Eugenia Gabrielova
Modified: 2011-03-07 18:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch 1: Advanced Search Menu Item and Action Callback (4.08 KB, patch)
2010-12-30 04:39 UTC, Eugenia Gabrielova
reviewed Details | Review
Patch 2: placeholders for docman replace functionality. (4.17 KB, patch)
2011-01-04 08:12 UTC, Eugenia Gabrielova
reviewed Details | Review
Patch 3: Unify / simply ui menuitems for search. (7.59 KB, patch)
2011-01-05 00:08 UTC, Eugenia Gabrielova
none Details | Review
Patch 4: Search/Replace Inline Widget, Part 1 (26.73 KB, patch)
2011-01-08 00:04 UTC, Eugenia Gabrielova
reviewed Details | Review
Patch 5: Replace Feature by Default in Search (Temporary) (46.49 KB, patch)
2011-01-10 03:06 UTC, Eugenia Gabrielova
none Details | Review
Patch 6: Inline Replace in UI, Replace button not quite ready though. (61.50 KB, patch)
2011-01-11 10:58 UTC, Eugenia Gabrielova
reviewed Details | Review
Patch 7: Replace Implementation + Labels Fix (68.26 KB, patch)
2011-01-13 02:34 UTC, Eugenia Gabrielova
none Details | Review
Patch 7.5: Signal fix for replace, moved replace setup to search_box_init (4.31 KB, patch)
2011-01-13 16:40 UTC, Eugenia Gabrielova
none Details | Review
Patch 7_5 Take 2: Signal fix for replace, moved replace setup to search_box_init (74.62 KB, patch)
2011-01-13 23:05 UTC, Eugenia Gabrielova
none Details | Review
Patch 7_6 Replace Help Text Handler (85.24 KB, patch)
2011-01-18 08:14 UTC, Eugenia Gabrielova
reviewed Details | Review
Patch 8 GTKGrid for Search Box GUI (101.63 KB, patch)
2011-01-19 23:30 UTC, Eugenia Gabrielova
none Details | Review
Patch to make it look as disired (8.38 KB, patch)
2011-01-20 17:45 UTC, Johannes Schmid
none Details | Review
9_1 Search Replace Refactoring (121.13 KB, patch)
2011-01-26 09:03 UTC, Eugenia Gabrielova
none Details | Review
9_2 Added Popup Menu for Check/Replace and Regex Entry (In Progress) (130.87 KB, patch)
2011-01-29 07:55 UTC, Eugenia Gabrielova
rejected Details | Review
Patch 9_3: Replace Key Bindings and Wrap (124.04 KB, patch)
2011-01-30 18:17 UTC, Eugenia Gabrielova
none Details | Review
Patch 9 4 Refactoring of Replace Keybind/Wrap Patch (130.93 KB, patch)
2011-01-31 03:12 UTC, Eugenia Gabrielova
none Details | Review
All of your patches merged into one to ease review. (20.30 KB, patch)
2011-01-31 16:20 UTC, Johannes Schmid
needs-work Details | Review
Initial foward/backward implementation (15.60 KB, patch)
2011-01-31 16:21 UTC, Johannes Schmid
needs-work Details | Review
Replace and Forward/Backward Navigation Fixes (14.61 KB, patch)
2011-02-02 04:22 UTC, Eugenia Gabrielova
reviewed Details | Review
Popup Menu With Case Check (40.81 KB, patch)
2011-02-11 00:07 UTC, Eugenia Gabrielova
none Details | Review
Popup Menu with Case Check and Highlight (54.30 KB, patch)
2011-02-13 10:48 UTC, Eugenia Gabrielova
reviewed Details | Review
Popup with highlight, case sensitivity, correctly implemented action groups (81.56 KB, patch)
2011-02-16 08:38 UTC, Eugenia Gabrielova
committed Details | Review
Regular Expression Search, has a quirk (28.43 KB, patch)
2011-02-23 08:44 UTC, Eugenia Gabrielova
none Details | Review
Regex Search Implementation Take 2 (Only missing backward) (46.17 KB, patch)
2011-02-25 16:17 UTC, Eugenia Gabrielova
none Details | Review
Regex Search Forward + Backward (16.24 KB, patch)
2011-02-28 09:36 UTC, Eugenia Gabrielova
none Details | Review
Completed Regex Searched [UTF8 Offset Solved] (16.81 KB, patch)
2011-03-01 19:36 UTC, Eugenia Gabrielova
none Details | Review
Completed Regex Search (14.93 KB, patch)
2011-03-02 20:08 UTC, Eugenia Gabrielova
needs-work Details | Review
[Ignore This One] (16.23 KB, patch)
2011-03-03 07:40 UTC, Eugenia Gabrielova
none Details | Review
Regex Search & Replace (18.69 KB, patch)
2011-03-05 19:33 UTC, Eugenia Gabrielova
none Details | Review
Regex Search & Replace (18.69 KB, patch)
2011-03-06 21:54 UTC, Eugenia Gabrielova
none Details | Review
Regular Expression Replace + Highlight Bug (11.19 KB, patch)
2011-03-07 08:22 UTC, Eugenia Gabrielova
none Details | Review

Description Eugenia Gabrielova 2010-12-27 20:43:02 UTC
Progress on Search redesign in Search Plugin
Comment 1 Eugenia Gabrielova 2010-12-30 04:39:46 UTC
Created attachment 177241 [details] [review]
Patch 1: Advanced Search Menu Item and Action Callback

Added Advanced Search option in Document Manager UI, current functionality is a placeholder which just mirrors basic document manager search.
Comment 2 Johannes Schmid 2010-12-30 08:22:52 UTC
Review of attachment 177241 [details] [review]:

What's the rational with having an "Advanced search" menu item? Shouldn't we just have one search and a "Find in Files" later?
Comment 3 Eugenia Gabrielova 2011-01-04 08:12:54 UTC
Created attachment 177459 [details] [review]
Patch 2: placeholders for docman replace functionality. 

Small patch to include a replace option for the DocumentManager replace feature. More to follow.
Comment 4 Johannes Schmid 2011-01-04 11:47:36 UTC
Review of attachment 177459 [details] [review]:

::: plugins/document-manager/anjuta-document-manager.xml
@@ +47,3 @@
 							<menuitem name="QuickSearch" action="ActionEditSearchQuickSearch"/>
 							<menuitem name="QuickReSearch" action="ActionEditSearchQuickSearchAgain"/>
+							<menuitem name="DocmanReplace" action="ActionEditSearchDocmanReplace"/>

Can you please use a common name for all these items, by either using "QuickSearch" or (better) just using "Search" as a prefix.
Comment 5 Eugenia Gabrielova 2011-01-05 00:08:46 UTC
Created attachment 177532 [details] [review]
Patch 3: Unify / simply ui menuitems for search.

Simplify UI options for search in document-manager; minor edit.
Comment 6 Eugenia Gabrielova 2011-01-08 00:04:03 UTC
Created attachment 177798 [details] [review]
Patch 4: Search/Replace Inline Widget, Part 1

Modified action callback and source files. Next step: need to fix a minor bug with the SEARCH_REPLACE declarations.
Comment 7 Johannes Schmid 2011-01-08 22:05:46 UTC
Review of attachment 177798 [details] [review]:

Overall this looks ok but I really think all that code (apart from the callback) should to into search-box.[ch] without the need of a seperate search-
Comment 8 Johannes Schmid 2011-01-08 22:06:23 UTC
Review of attachment 177798 [details] [review]:

Overall this looks ok but I really think all that code (apart from the callback) should to into search-box.[ch] without the need of a separate search-replace.c

(sorry, the last comment was trucated somehow).
Comment 9 Eugenia Gabrielova 2011-01-10 03:06:00 UTC
Created attachment 177907 [details] [review]
Patch 5: Replace Feature by Default in Search (Temporary)

For testing purposes, after merging inline-replace with search, I am committing my setup for having replace on by default with QuickSearch. The patch is to document for future reference,and to include the removal of search-replace.c/h.
Comment 10 Eugenia Gabrielova 2011-01-11 10:58:18 UTC
Created attachment 178022 [details] [review]
Patch 6: Inline Replace in UI, Replace button not quite ready though.

Replace field and buttons appear for search & replace command. 

Implemented replace_one feature partially; currently fixing entry not being replaced.
Comment 11 Johannes Schmid 2011-01-11 17:23:22 UTC
Review of attachment 178022 [details] [review]:

Hmm, inline comments seems to be broken today...

Anyway, please don't add search/replace_label to the struct. Labels are nothing you would want to refer to later so just leave them as temporary variables, gtk+ will take care of their life-cycle.
Comment 12 Eugenia Gabrielova 2011-01-11 17:37:36 UTC
I didn't catch the comments thing, thanks, I'll check on that. 

Removing labels as well.
Comment 13 Johannes Schmid 2011-01-12 09:09:16 UTC
Sorry, I didn't talk about any bug in anjuta. What I meant was that the inline patch review tool of bugzilla seems broken for me so I couldn't comment on individual lines of the patch.
Comment 14 Eugenia Gabrielova 2011-01-13 02:34:35 UTC
Created attachment 178196 [details] [review]
Patch 7: Replace Implementation + Labels Fix

Replace Entry now works on button press (Replace!) - next step is just to include replace all. Barrier in previous patch was an incorrect gsignal for the replace button.  Replaced GtkWidget labels in search_box with temporary variables.
Comment 15 Johannes Schmid 2011-01-13 09:24:53 UTC
I have to test this so just code review might not be sufficient for now. Anyway, for the set_replace() method I would prefer if it would just hide/show the appropriate widgets while they are created on startup.
Comment 16 Eugenia Gabrielova 2011-01-13 16:40:44 UTC
Created attachment 178245 [details] [review]
Patch 7.5: Signal fix for replace, moved replace setup to search_box_init

- Moved replace buttons/entry setup to search-box-init
- search_set_replace just shows the elements, probably need to add a couple of error checks on that. 
- Not sure how we want to manage closing replace - sufficient to just call search-box-hide? Left that alone for now.
- Next step: Fixing replace-all. should be ready by this afternoon. I think the signal bug in replace that I missed yesterday may have affected it, so will try again.
Comment 17 Johannes Schmid 2011-01-13 22:57:25 UTC
The last patch doesn't apply for me. Could you create a full patch that contains all the previous patches like done before?

Thanks!
Comment 18 Eugenia Gabrielova 2011-01-13 23:05:58 UTC
Created attachment 178274 [details] [review]
Patch 7_5 Take 2: Signal fix for replace, moved replace setup to search_box_init

Take 2: Moved replace widget set up to search_box init, fixed a couple of signals bugs.

This one should work, my mistake on the other one, formatted incorrectly.
Comment 19 Johannes Schmid 2011-01-14 17:47:58 UTC
OK, that's a really a step forward, seems the functionality is quite good. Thanks!

However, the UI needs improvements:
* Replace entry should definitly be below search entry IMHO
* I don't like the "Replace with:" label, instead I would propose the following as done in some other GNOME applications:
 - There is neither a "search:" nor a "replace" label
 - when the entry boxes are empty, a grey text is displayed in the entries, with something like (maybe you have better wording): "<Type Search term here>", "<Replace with>". (http://git.gnome.org/browse/glade/tree/gladeui/glade-inspector.c#n369 has an example how to do this properly)
* Buttons for searching: Next, Prev, Highlight
* Buttons for replaceing: Replace All
* Pressing Enter ("activate" signal) in the replace entry should invoke search next/replace depending on the state.

Bad ASCII-Art ahead:
---------------------------------------------------------------------------
     | <Type search term here> |  <Next> <Prev> <Highlight>
---------------------------------------------------------------------------
     | <Replace with>          |  <Replace all>
---------------------------------------------------------------------------

* Case sensitivity and regular expressions should be available as popup-menu (see bug 545389)

Please also make sure to use stock items for all buttons, they exist for all these buttons in gtk+ (not entirely sure about "Highlight" and "Replace all" but the rest should be there).
Comment 20 Eugenia Gabrielova 2011-01-14 18:02:02 UTC
Thanks for the excellent feedback, I'm thrilled to hear it is going well.

My Next Steps, Moving Forward

1. Pack Replace Entry below Search Entry, Remove "Buttons", add Grey Text 
2. Improve Activate signal to account for Search Next/Replace
3. Replace All Implementation
4. Add pop up menu for Case / Regex

My goal is to finish #1-3 by Monday, starting today, and add a basic format for #4.
Comment 21 Johannes Schmid 2011-01-14 18:14:48 UTC
btw, don't forget to blog about your stuff.
Comment 22 Eugenia Gabrielova 2011-01-18 08:14:18 UTC
Created attachment 178588 [details] [review]
Patch 7_6 Replace Help Text Handler

Minor patch, just to show some progress. 

I'm focusing on the help text < Search here >, <Replace here>. Works fine for Replace because on the start of a research, replace is not in focus. For replace, if implemented similarly to glade example, help text does not show up. 

They use a "search_disabled" flag in their version of a private search_box structure, and I tried adding a function similar to this, but I'm not sure if that's the best idea in the long run. Anyway, I'll leave this as is for now and focus on the pop-up menu and rearrangement of search and replace entries.
Comment 23 Johannes Schmid 2011-01-18 17:40:39 UTC
Review of attachment 178588 [details] [review]:

Some comments (in addition to the previous):
* There are some critical warnings on the command-line, please address those
* The <Replace with> text should also be shown on focus-out when the entry is empty.
 * Actually, maybe this is overkill. It might look quite ok if we use GTK_STOCK_FIND and GTK_STOCK_FIND_AND_REPLACE as primary icon of the entries (http://library.gnome.org/devel/gtk/unstable/GtkEntry.html#GtkEntry--primary-icon-stock). Simply drop the grey text thing
 * Sorry for bothering you with it in the first place...

Well, you have the mockup otherwise ;)
Comment 24 Eugenia Gabrielova 2011-01-19 23:30:00 UTC
Created attachment 178800 [details] [review]
Patch 8  GTKGrid for Search Box GUI

- Reorganized Search Box into Grid - currently stored as GtkWidget within SearchBox, though not sure if that's the best setup. 
- Any advice on if I'm setting up GtkGrid correctly is appreciated - I think I am, but feedback would be great. 
- Created prev/next stock buttons, will find a good one for the Replace All button. 
- Cleaned up gtk_set_replace method
- Is it important that search_box ui takes up whole bottom of screen? Perhaps if it just takes up the bottom right corner and adapts its width as needed, it would be extra pretty. 

Next Patch will include pop-menu for Case_check, Replace-all. 

Just a note, the Search_box_init method is getting pretty crowded. It might be useful for code maintainability to move the layout to a glade-ui file and load from that, but not urgent.
Comment 25 Johannes Schmid 2011-01-20 17:45:24 UTC
Created attachment 178861 [details] [review]
Patch to make it look as disired

This is a patch on top of yours that makes the search-box look like I would have wanted it.

Small general comments on your coding style:
* Please also make an extra line for { so
if (test)
{

}
instead of
if (test {
}

* All strings need to be embedded into the gettext macro _("Hello World") so they can get translated

* We don't use C99 so please always declare your variables at the beginning of the block.

Otherwise it looks quite good. Thanks!
Comment 26 Eugenia Gabrielova 2011-01-22 19:44:20 UTC
Thanks, jhs, looks great. 

Currently working on replace all, and adjusting the replace-one handler, and find in multiple files.
Comment 27 Eugenia Gabrielova 2011-01-26 09:03:14 UTC
Created attachment 179358 [details] [review]
9_1 Search Replace Refactoring

Refactored Search / Replace / Replace All to reduce amount of complex code. 
- Still need to fix up handling of "Enter" event within replace_entry, and handling of previous/next buttons. 

Replace all ended up being just fine, especially with the improved logic. 

Hopefully will have utf-8 fixed up ASAP :]
Comment 28 Eugenia Gabrielova 2011-01-29 07:55:29 UTC
Created attachment 179577 [details] [review]
9_2 Added Popup Menu for Check/Replace and Regex Entry (In Progress)

In progress - currently adapting from Migration Checklist at 
http://library.gnome.org/devel/gtk/unstable/gtk-migrating-checklist.html#checklist-popup-menu

- Added "More..." option in search_box menu, and an associated button press handler
- Added regex entry to menu, may add that to search_box structure in the future, OK here for now. 
- Not sure of best place to attach it, need to create a positioning function perhaps: http://library.gnome.org/devel/gtk/unstable/GtkMenu.html#GtkMenuPositionFunc

Status: Loads popup but then causes segfault, not sure precisely why but I think it is either related to the signal to start the menu or the fact that the menu has nothing in it yet, or perhaps where I am attaching it. Will test more and correct.
Comment 29 Johannes Schmid 2011-01-30 14:37:55 UTC
Review of attachment 179577 [details] [review]:

Sorry, but I am not happy with the way you work and the recent changes. Lots of stuff just didn't work yet in the past patches and you don't fix it but instead add more half-finished stuff. Please, first fix all the stuff you had in the previous patch, that is

* Working forward/backward buttons
* Correct keybindings for replace
* Working replace all

As I said in the previous comment I want the popup menu connected to the image in the GtkEntry for the search string but please focus on making things working first. Probably best if you go back to your previous state and fix all the things I mentioned.

Thanks.
Comment 30 Eugenia Gabrielova 2011-01-30 18:17:31 UTC
Created attachment 179644 [details] [review]
Patch 9_3: Replace Key Bindings and Wrap

Replace now works fine on enter press. 

Added a condition to incremental_search to allow wrap-around, which wasn't functioning correctly on replace/replace all - now wraps correctly.
Comment 31 Eugenia Gabrielova 2011-01-31 03:12:04 UTC
Created attachment 179674 [details] [review]
Patch 9 4 Refactoring of Replace Keybind/Wrap Patch

Refactoring of Patch 9_3

- Created search_forward and search_backward methods to handle incremental search calls (forward is default for replace functionality)

- Merged on_search_box_search into search_box_incremental_search
Comment 32 Johannes Schmid 2011-01-31 16:20:56 UTC
Created attachment 179724 [details] [review]
All of your patches merged into one to ease review.
Comment 33 Johannes Schmid 2011-01-31 16:21:50 UTC
Created attachment 179725 [details] [review]
Initial foward/backward implementation

Needs more work.
Comment 34 Johannes Schmid 2011-01-31 16:26:52 UTC
Review of attachment 179724 [details] [review]:

::: plugins/document-manager/search-box.c
@@ +319,3 @@
+
+	else
+	// Otherwise, search was unsuccessful

Don't comment obvious things, if the first branch was successful search it is pretty clear what the else branch will be.

@@ +328,3 @@
+on_search_box_forward_search (GtkWidget * widget, SearchBox * search_box)
+{
+	gboolean search_forward, search_result;

These signal handlers don't return anything. Fixed in my next patch.

@@ +363,3 @@
+	}
+
+	// Otherwise, if search ends up unsuccessful, 

dito, see above

@@ +376,3 @@
+	const gchar* replace_text = gtk_entry_get_text (GTK_ENTRY (private->replace_entry));
+
+	// If editor is not active or replace text does not exist, end replace progress

Please use C-style comments (/* */) if posssible

@@ +400,3 @@
+	gboolean entry_found;
+
+	// If editor is not active or replace text does not exist, end replace progress

Another comments that doesn't say anything more than the code itself.
Comment 35 Johannes Schmid 2011-01-31 16:30:17 UTC
Review of attachment 179725 [details] [review]:

::: plugins/document-manager/search-box.c
@@ +353,3 @@
+		}
+	}
+	else

I added basic handling for backward search here. It doesn't work correctly because the start/end is not adjusted correctly and there might be some other side-effects. Needs fixing but should be obvious.

@@ +414,3 @@
 	}
+
+	return found;

We already have a variable that tells us if we found a result so use this instead of the selection.

@@ -415,3 @@
-
-void
-on_search_activated (GtkWidget* widget, SearchBox* search_box)

I wondered why this is still there because it is basically already covered in search_box_incremental_search(). Anyway, used the code from this method as it worked as expected and moved it to search_box_incremental_search()
Comment 36 Johannes Schmid 2011-01-31 16:36:53 UTC
Completely unrelated, I think we need to bring the "Replace" button back in addition to replace_entry keybindings. When using the mouse it feel unnatural that there is no way to actually replace the selection.

Btw, before replacing you should check that the selection actually matches the text in search_entry.
Comment 37 Eugenia Gabrielova 2011-02-02 04:22:08 UTC
Created attachment 179860 [details] [review]
Replace and Forward/Backward Navigation Fixes

Focus of Patch: Implementing requested changes to search/replace and forward/backward.

- Added replace button and keybindings to search box.
- Added check to make sure that selection in editor matches search entry before replacing. 
- Rewrote a few comments for style + clarity
- Changed grid init in search_box_init to GTK_HBOX from GTK_BOX to SearchBox type. 
- Corrected backward functionality by sorting out search_start + search_end. 

Known issue: When tabbing from search_entry -> back -> forward -> close -> replace_entry, the editor loses its selection when tabbing from close on the first row to replace_entry on the 2nd. Could be a focus thing, or could be a grid box thing, I'm not 100% sure.
Comment 38 Johannes Schmid 2011-02-03 18:07:22 UTC
Review of attachment 179860 [details] [review]:

Works fine here. Thanks!

Could you try to implement the popup-menu with "case sensitive" and "highlight occurences" next, please? After that I guess we can merge that into master.

::: plugins/document-manager/search-box.c
@@ +51,3 @@
 	GtkWidget* next_button;
 	GtkWidget* previous_button;
+

nit-picking, what are the *s for?
Comment 39 Eugenia Gabrielova 2011-02-11 00:07:29 UTC
Created attachment 180625 [details] [review]
Popup Menu With Case Check

This patch focuses on pop-up menu activity. 

Possible issues / questions:
- I created gtk_check_menu_items for highlight + case, seemed faster than creating a separate ui. The handlers are still in action callbacks in case you'd like to change that. 
- Highlight all isn't implemented here, but casecheck works fine.
- Popup menu pops down instead of up. Can create a GtkMenuPositionFunc to make it to otherwise.
Comment 40 Eugenia Gabrielova 2011-02-13 10:48:55 UTC
Created attachment 180765 [details] [review]
Popup Menu with Case Check and Highlight

Implementation of a popup menu with case check and also highlight. 

- Created popup menu from search_entry icon: currently contains case check, highlight all, clear highlight menu items.
- Case check works as expected of former case check button.
- Replace / Replace all work as expected with highlight and case check. 
- Highlight all allows full document highlight and navigation without
losing highlight.
- Clear highlight button wasn't in our spec discussion but I think it's a good idea in case user gets annoyed with the highlight. Thoughts? 

Known issues: Popup menu grows down rather than up - OK now, problem if there are more entries in the future. GtkMenuEntries created in popup menu setup method - might need a separate UI file to organize a bit better.
Comment 41 Johannes Schmid 2011-02-13 15:02:47 UTC
Review of attachment 180765 [details] [review]:

Looks pretty good overall.

I wonder if you really need to connect the signal and add the menu items everytime you use the menu. Shouldn't it be enough to do that once when the menu is created?

A seperator between the case check and the highlight menuitems would also be really nice.

::: plugins/document-manager/anjuta-document-manager.xml
@@ +154,3 @@
 		</menu>
 		<menuitem name="Close" action="ActionFileClose" />
 	</popup>

Please clean this if you don't need it.
Comment 42 Eugenia Gabrielova 2011-02-16 08:38:27 UTC
Created attachment 180965 [details] [review]
Popup with highlight, case sensitivity, correctly implemented action groups

- Removed popup check-menu items from search-box.c popup menu and setup. 
- Created toggle entry action group for Plugin to handle highlight / case sensitivity.
- Cleaned up a few warnings of incompatible pointer types. 
- Created toggles in searchbox struct to manage highlight and case sensitivity. Only toggle from popup menu action callbacks.
- All popup menu entries now created in anjuta-docman XML file
Comment 43 Eugenia Gabrielova 2011-02-16 15:19:05 UTC
A quick note: here are a few bugs in the Search plugin that I believe we have taken care of thus far with Docman search.

Replace All
https://bugzilla.gnome.org/show_bug.cgi?id=530060

Next / Quick ReSearch
https://bugzilla.gnome.org/show_bug.cgi?id=565695

Removing match lines option in the dialogue (our searchbox doesn't have it)
https://bugzilla.gnome.org/show_bug.cgi?id=565117

Double search results in one line
https://bugzilla.gnome.org/show_bug.cgi?id=601945

This one: Sort of? Find and replace all
https://bugzilla.gnome.org/show_bug.cgi?id=447814
Comment 44 Eugenia Gabrielova 2011-02-23 08:44:56 UTC
Created attachment 181676 [details] [review]
Regular Expression Search, has a quirk

I think the regular expression search I've implemented is good for basic regexes, not sure if I've accounted for every possible case. It took me a little while to sort out the logic, and I would appreciate pointers (harhar) on whether I implemented the incremental_regex_search method correctly.

Backward search is quirky right now, investigating why. Will fix for next patch.

Questions
1. Let's say we have the regex [A - Z]+, very simple. Let's say we have a word in our document, APPLE. My search will highlight "APPLE", then "PPLE", then "PLE", then "LE", then "E" before moving on. Is this the behavior we want? 
2. Not sure if my method will cause problems for certain classes of complex regexes. Advice appreciated on what sorts of things I should test.
Comment 45 Eugenia Gabrielova 2011-02-25 16:17:38 UTC
Created attachment 181927 [details] [review]
Regex Search Implementation Take 2 (Only missing backward)

Rewrote regex search correctly with positions. Forward search is fully implemented here, wanted to submit a patch to receive feedback on code style and logic and such. Will be doing some refactoring to add backward search, clean up repetitive pieces of code etc.
Comment 46 Eugenia Gabrielova 2011-02-28 09:36:05 UTC
Created attachment 182072 [details] [review]
Regex Search Forward + Backward

- Regex forward and backward are now fully functional. 
- Clicking highlight all toggles highlighting. 
- Still finishing up some UTF-8 encoding stuff, but this is at least a solid implementation of backward regex which was previously missing. 

Thanks for any feedback!
Comment 47 Eugenia Gabrielova 2011-03-01 19:36:38 UTC
Created attachment 182206 [details] [review]
Completed Regex Searched [UTF8 Offset Solved]

Regex search now functions correctly with forward, backward. Handles unusual characters by adjusting offset. Feedback / corrections to make as always are appreciated.

Un-Clicking "Highlight All" also removes all highlights, as requested.
Comment 48 Eugenia Gabrielova 2011-03-02 20:08:28 UTC
Created attachment 182301 [details] [review]
Completed Regex Search

Rewrite of previous completed regex search patch to successfully apply with new master. Seems to work correctly; hopefully the issue won't occur again.
Comment 49 Johannes Schmid 2011-03-02 22:37:38 UTC
Review of attachment 182301 [details] [review]:

The searching in the patch works fine. Thanks!

* Please make sure all the memory is free'd and all object are unref'd correctly. I marked some occurences in the patch but I am sure there is more. Basically you have to free/unref any pointer except GtkWidgets (usually those are memory counted). If you have questions/problems here please ask.
* Replacing does not work. Replacing should work with
1) replacing literally
2) replacing backreferences (http://www.regular-expressions.info/brackets.html has some information)
So I guess it would be best to use g_regex_replace when replacing. Instead of checking if the selection matches the search string (normal mode) it should be checked if the selection matches the regular expression in regex mode.
3) Please fix the string to look less nerdy ;)

::: plugins/document-manager/anjuta-document-manager.xml
@@ +184,3 @@
 		<menuitem name="ClearHighlight" action="ActionSearchboxPopupClearHighlight" />
+		<separator name="separator30"/>
+		<menuitem name="Regex Search" action="ActionSearchboxRegexSearch" />

I think it would be better to avoid that space here to comply with the rest of the code.

::: plugins/document-manager/plugin.c
@@ +292,3 @@
+	G_CALLBACK (on_search_popup_highlight_toggle)},
+  { "ActionSearchboxRegexSearch", GTK_STOCK_FIND,
+	N_("RegEx Search"), NULL,

Should be called "Regular Expression"

::: plugins/document-manager/search-box.c
@@ +342,3 @@
+					else
+					{
+						search_end = IANJUTA_EDITOR_CELL (ianjuta_editor_selection_get_end (selection, NULL));

search_end is never free'd

@@ +396,3 @@
+		if (search_forward)
+		{
+			text_to_search = ianjuta_editor_get_text (private->current_editor,

text_to_search is not free'd

@@ +407,3 @@
+		else
+		{
+			text_to_reverse = ianjuta_editor_get_text (private->current_editor,

dito for text_to_reverse, start_pos and end_pos

@@ +411,3 @@
+													IANJUTA_ITERABLE(search_start), NULL);
+
+			if (g_utf8_validate (text_to_reverse, strlen(text_to_reverse), NULL))

This is unnecessary, the editor may only contain utf-8 text, ianjuta_editor_get_text() may not return invalid utf8 characters.

@@ +418,3 @@
+
+				start_pos = ianjuta_iterable_get_position(IANJUTA_ITERABLE(search_end), NULL) - start_pos;
+				end_pos = ianjuta_iterable_get_position(IANJUTA_ITERABLE(search_end), NULL) - end_pos;

dito for freeing text_to_search, start_pos, end_pos

@@ +441,3 @@
+				result_start = NULL;
+				result_end = NULL;
+			}

result_start and result_end need to be free'd (before they are set to NULL).
Comment 50 Eugenia Gabrielova 2011-03-03 07:40:54 UTC
Created attachment 182325 [details] [review]
[Ignore This One]

Patch (includes content of previous patch) focusing on cleaning up Regex Search a bit before doing replace. 

Issues addressed: 
* Fixed a pretty significant bug that occurred with text selection (I had introduced it by accident). 
* XML, Plugin, ActionCallback entries for regex corrected (spaces, abbreviation, etc)
* I think I accounted for all memory issues. A number were already handled by the original search, and I added a few that you mentioned. Not sure what to do about start_pos, start_end since they have to maintain their info. 

Questions:
1. I'm confused as to which string is unusually nerdy. If it was the Star Wars reference in my commit message, no more I promise! :P. 

Next patch will include my Regex Replace implementation.
Comment 51 Johannes Schmid 2011-03-03 08:48:23 UTC
Hi!

> * I think I accounted for all memory issues. A number were already handled by
> the original search, and I added a few that you mentioned. Not sure what to do
> about start_pos, start_end since they have to maintain their info. 

They need to be free'd when they are not used anymore (e.g. when you don't have anymore pointer to them).

> Questions:
> 1. I'm confused as to which string is unusually nerdy. If it was the Star Wars
> reference in my commit message, no more I promise! :P. 

I was just referring to the RegEx abbreviation ;)
Comment 52 Johannes Schmid 2011-03-05 09:24:13 UTC
Would it be possible to have an updated patch (whether replace is working or not...) until Monday evening. We are entering string freeze then so we have no chance to merge this into 3.0 if it isn't ready by then.
Comment 53 Eugenia Gabrielova 2011-03-05 19:33:38 UTC
Created attachment 182576 [details] [review]
Regex Search & Replace

I believe that Regex Search & Replace work fine. Tested with a few backreferences for replace - works fine there. I think I sorted out most of the memory-deallocation issues, please let me know if you find any. The g_regex_replace method is designed to handle backreferences I believe, but also handles literal replace just fine.
Comment 54 Eugenia Gabrielova 2011-03-05 19:36:10 UTC
There's a bug in highlight-all mode that I am currently working on before moving forward with multi-file search. Will try to catch jhs on IRC to chat about it.
Comment 55 Eugenia Gabrielova 2011-03-06 21:54:59 UTC
Created attachment 182637 [details] [review]
Regex Search & Replace

Regex Search & Replace
Comment 56 Eugenia Gabrielova 2011-03-07 08:22:19 UTC
Created attachment 182678 [details] [review]
Regular Expression Replace + Highlight Bug

Regular Expression Replace

Fixed highlight-all bug.
Comment 57 Johannes Schmid 2011-03-07 18:21:39 UTC
OK, that works fine for me. As I think everything we planned is implemented now, I will close this bug.