Merge lp:~shokry/pantheon-terminal/fix-1426470 into lp:~elementary-apps/pantheon-terminal/trunk

[Shokry]

[xapantu (xapantu)]

Thanks for your patch! However, there are a few things that could be improved:

Code style (not the most important):
* you don't need to use private, it is private by default (this one is a subjective one, I agree)
* you don't need to use "this" unless it is absolutely necessary (i.e. if you use an object it will by default be the attribute of "this" object unless superseded)

Bugs that I found (that must be fixed before merging):
* if you search something that only appear once, the arrows are disabled once it has been found, I don't think this is what we want.
* if I type "x" and then backward so as the search entry is empty, I still jump between "x" because the terminal search is not updated in "search_changed_cb"

review: Needs Fixing

[Shokry (shokry)]

Thank you for the really helpful comment.

As for the first bug, I think it's a limitation in the VTE widget's search functions, if a search term appears only once, it'll be highlighted once, next call to the find next/previous function doesn't highlight it again and looks like it's treated as a not-found even if the search wrapping is enabled, I don't know if this behavior can be overridden.

I can try to mimic the desired (normal) search behavior but I'm not sure if that's the right thing to do

[xapantu (xapantu)]

The problem with the first bug is that I have that :
- Type "unique_word" in the search box
- Click on next -> that selects the word
- Click on next -> that unselects the word, and makes the arrow unsensitive.

If that's a vte bug, then it should be reported. Reading your code I tend to agree with you, but that may need more investigation.

Another problem is that the search should be more responsive : when something is typed, it should automatically go to the first occurence of the word (that is linked with the other behaviour problems).

So, to sum up, the expected behaviour would be (maybe not entirely doable with our current vte version) :
* Type something in the search entry : the terminals jump to the first occurrence, if available.
* If not available, everything go unsensitive
* No backward problem if the search entry == "" (and the buttons must be unsensitive too)
* If there is only one occurence, the next buttons should be unsensitive

[lp:~shokry/pantheon-terminal/fix-1426470]

748. By Shokry on 2015-10-01

highlight first occurence while typing in search entry

749. By Shokry on 2015-10-02

button sensitivity changes only in search functions

750. By Shokry on 2015-10-03

code style fix

[Shokry (shokry)]

The desired behavior should now be present except for the last point.

Unmerged revisions

750. By Shokry on 2015-10-03

code style fix

749. By Shokry on 2015-10-02

button sensitivity changes only in search functions

748. By Shokry on 2015-10-01

highlight first occurence while typing in search entry

747. By Shokry on 2015-08-14

sensitivity is set by a function

746. By Shokry on 2015-08-14

search buttons insinsitive when created

745. By Shokry on 2015-08-14

buttons insinsitive when no text entered or no match

=== modified file 'src/SearchToolbar.vala'
--- src/SearchToolbar.vala	2015-09-16 22:16:20 +0000
+++ src/SearchToolbar.vala	2015-10-03 10:27:06 +0000
@@ -28,6 +28,9 @@
 
         public Gtk.SearchEntry search_entry;
 
+        private Gtk.ToolButton previous_button;
+        private Gtk.ToolButton next_button;
+
         public signal void clear ();
 
         public SearchToolbar (PantheonTerminalWindow window) {
@@ -45,18 +48,20 @@
             var i = new Gtk.Image.from_icon_name ("go-up-symbolic",
                                                   Gtk.IconSize.SMALL_TOOLBAR);
             i.pixel_size = 16;
-            var previous_button = new Gtk.ToolButton (i, null);
-            previous_button.set_tooltip_text (_("Previous result"));
+            this.previous_button = new Gtk.ToolButton (i, null);
+            this.previous_button.set_tooltip_text (_("Previous result"));
 
             i = new Gtk.Image.from_icon_name ("go-down-symbolic",
                                               Gtk.IconSize.SMALL_TOOLBAR);
             i.pixel_size = 16;
-            var next_button = new Gtk.ToolButton (i, null);
-            next_button.set_tooltip_text (_("Next result"));
+            this.next_button = new Gtk.ToolButton (i, null);
+            this.next_button.set_tooltip_text (_("Next result"));
+
+            set_search_buttons_sensitivity (false);
 
             this.add (tool_search_entry);
-            this.add (previous_button);
-            this.add (next_button);
+            this.add (this.previous_button);
+            this.add (this.next_button);
 
             this.show_all ();
             this.set_style (Gtk.ToolbarStyle.ICONS);
@@ -66,8 +71,8 @@
             this.clear.connect (clear_cb);
             this.grab_focus.connect (grab_focus_cb);
             this.search_entry.search_changed.connect (search_changed_cb);
-            previous_button.clicked.connect (previous_search);
-            next_button.clicked.connect (next_search);
+            this.previous_button.clicked.connect (previous_search);
+            this.next_button.clicked.connect (next_search);
         }
 
         void clear_cb () {
@@ -80,22 +85,52 @@
 
         void search_changed_cb () {
             try {
+<<<<<<< TREE
                 // FIXME Have a configuration menu or something.
                 var regex = new Regex (Regex.escape_string (search_entry.text),
                                        RegexCompileFlags.CASELESS);
                 this.window.current_terminal.search_set_gregex (regex, 0);
                 this.window.current_terminal.search_set_wrap_around (true);
+=======
+                if (search_entry.text == "") {
+                    this.window.current_terminal.search_set_gregex (null);
+                    this.window.current_terminal.select_none ();
+                    set_search_buttons_sensitivity (false);
+                } else {
+                    // FIXME Have a configuration menu or something.
+                    var regex = new Regex (Regex.escape_string (search_entry.text),
+                                           RegexCompileFlags.CASELESS);
+                    this.window.current_terminal.search_set_gregex (regex);
+                    this.window.current_terminal.search_set_wrap_around (true);
+
+                    this.window.current_terminal.select_none ();
+                    next_search ();
+                }
+>>>>>>> MERGE-SOURCE
             } catch (RegexError er) {
                 warning ("There was an error to compile the regex: %s", er.message);
             }
         }
 
         public void previous_search () {
-            this.window.current_terminal.search_find_previous ();
+            if (this.window.current_terminal.search_find_previous () == false) {
+                set_search_buttons_sensitivity (false);
+            } else {
+                set_search_buttons_sensitivity (true);
+            }
         }
 
         public void next_search () {
-            this.window.current_terminal.search_find_next ();
+            if (this.window.current_terminal.search_find_next () == false) {
+                set_search_buttons_sensitivity (false);
+            } else {
+                set_search_buttons_sensitivity (true);
+            }
+        }
+
+        void set_search_buttons_sensitivity (bool sensitivity) {
+            this.previous_button.set_sensitive (sensitivity);
+            this.next_button.set_sensitive (sensitivity);
         }
     }
 }