Merge lp:~oliver-joos/zim/selection-fixes into lp:~jaap.karssenberg/zim/pyzim

[Oliver Joos]

After using this branch now for several weeks with command line option "--debug" and no strange log lines or other problems I propose it for merging.

I saw that one minor bugfix included in this branch has also been fixed in the main branch meanwhile. The rest should merge seamlessly.

[lp:~oliver-joos/zim/selection-fixes]

411. By Oliver Joos on 2011-06-19

Fix for broken search_backlinks due to selection on page.

[Oliver Joos (oliver-joos)]

While working on another branch to improve the SearchDialog (to solve bug 796215), I saw that this branch broke menu "Search Backlinks..." whenever a selection exists in the pageview. Rev 411 fixes this.

Sorry for that! Now this branch should be truly "Mature".

review: Needs Resubmitting

[Jaap Karssenberg (jaap.karssenberg)]

Had a look at the patch to see what it does. Agree with using text selection as default in the search dialog. Also defining constant for scroll margin is good cleanup.

However I fail to see the advantage of having a special tag for the find match. You still need to select the match with a normal selection. Maybe I miss something here ?

review: Needs Information

[Oliver Joos (oliver-joos)]

Yes, this branch does still select the match with a normal selection. But a selection only is quite volatile:

Select a word and press Ctrl+F => cursor and selection will jump to the InputEntry of the FindBar. You can't see where your current match is, and you have to remember/guess where you get by pressing "Next" or "Previous". You may jump a few words or scroll down dozens of lines. Activating "Highlight" improves it a bit, as you then see the potential matches. But the current match is still not visible, whenever something else gets selected.

My branch solves it like Firefox and Chromium do. Although Firefox (at least 3.x) has quite funny colors: green for the current match and purple to highlight other matches. Chromium looks better with yellow and orange. Zim has orange to highlight, so I chose a dark yellow for the current match, different enough from the bright yellow of Marked text. To me it feels more predictable/less volatile than with the selection only.

[Jaap Karssenberg (jaap.karssenberg)]

On Tue, Jun 21, 2011 at 3:32 PM, Oliver Joos <email address hidden> wrote:

> My branch solves it like Firefox and Chromium do. Although Firefox (at
> least 3.x) has quite funny colors: green for the current match and purple to
> highlight other matches. Chromium looks better with yellow and orange. Zim
> has orange to highlight, so I chose a dark yellow for the current match,
> different enough from the bright yellow of Marked text. To me it feels more
> predictable/less volatile than with the selection only.
>

I checked in firefox, however the behavior is different. The green color is
just the text selection, as soon as you move the cursor (e.g. hit an arrow
key) it is gone. However as implemented now in zim it stays even when the
cursor moves.

Note that when we hit "Next" it starts at the cursor position anyway, not
necessarily the match highlighted.

-- Jaap

[Oliver Joos (oliver-joos)]

I see what you mean. It seems that the Firefox page and its find-in-page field have separate selections, whereas Zim can only have one visible selection. Changing this could be a bigger task, and that's not worth the benefit.

I solved it simply be re-using Zims way of highlighting all matches. If "Highlight" is enabled then my fix makes not a big difference. If it is disabled then (as you wrote) my fix makes the current-match selection a bit more stubborn, but only like if "Highlight" was enabled. And if find-in-page is closed then any match selections are gone - with or without Highlight and with or without my fix.

Do you think I should change my branch? E.g. I could limit the "make the current match yellow" part to be only applied if Highlight is enabled.

[Oliver Joos (oliver-joos)]

> Note that when we hit "Next" it starts at the cursor position anyway, not
> necessarily the match highlighted.

After re-reading this I looked again at it. Now I see your point here: if cursor position changes then the highlighted match is not a "current match" anymore, which determines what hitting "Next" will do. Ok, I agree! Then let us call it the "most recent match", which is still nice to be highlighted, I think.

For me the bigger point is: when we hit Ctrl+F and then "Next" it *does not* start at the cursor position! And it is hard to understand where it will start, at least without my fix! It will start at the second match after the cursor, because hitting Ctrl+F already selects the first match after the cursor but in the same time moves the selection from this match to the InputEntry of find-in-page area. The first match is simply not visible!! This is what my fix tries to solve, simply by doing the same for the (!) "most recent match" as the "Highlight" button does for all matches.

[Jaap Karssenberg (jaap.karssenberg)]

On Mon, Jul 4, 2011 at 2:21 AM, Oliver Joos <email address hidden> wrote:

> For me the bigger point is: when we hit Ctrl+F and then "Next" it *does
> not* start at the cursor position! And it is hard to understand where it
> will start, at least without my fix! It will start at the second match after
> the cursor, because hitting Ctrl+F already selects the first match after the
> cursor

To be precise:
- If the cursor is at the beginning of the a match "Find" will highlight it
while "Next" will jump to the next one
- If the cursor is some in between matches both "Find" and "Next" will go
to the first match

> but in the same time moves the selection from this match to the InputEntry
> of find-in-page area. The first match is simply not visible!! This is what
> my fix tries to solve, simply by doing the same for the (!) "most recent
> match" as the "Highlight" button does for all matches.
>

What do you mean by "the first match is not visible" ? When testing here the
text selection is visible even when the text does not have the input focus
-- be it in a less obvious color. Maybe you have a different Gtk theme that
makes the selection less obvious ?

Sounds to me we should add some code to your patch to unset the match
highlighting when the cursor moves. (There is a signal for mark position
changed and the cursor is a special mark.)

-- Jaap

[Oliver Joos (oliver-joos)]

> To be precise:
> - If the cursor is at the beginning of the a match "Find" will highlight it
> while "Next" will jump to the next one
> - If the cursor is some in between matches both "Find" and "Next" will go
> to the first match

Exactly, except Ctrl+F does not highlight the selection here!?

> What do you mean by "the first match is not visible" ? When testing here the
> text selection is visible even when the text does not have the input focus
> -- be it in a less obvious color. Maybe you have a different Gtk theme that
> makes the selection less obvious ?

I use Ubuntu 10.04.2 LTS with theme "Dust Sand" and also tested 10.10 with default theme "Ambiance": if page text has a selection but not the focus then this selection is light gray (less obvious, but expected). But if text in the find InputEntry gets selected (e.g by Ctrl+F) then the selection is taken away from the page text - irretrievably, and not only visually!

> Sounds to me we should add some code to your patch to unset the match
> highlighting when the cursor moves. (There is a signal for mark position
> changed and the cursor is a special mark.)

This would be ok for me too! But first I'll investigate it further. It really could be some problem with GTK.

[Jaap Karssenberg (jaap.karssenberg)]

On Wed, Jul 6, 2011 at 1:22 PM, Oliver Joos <email address hidden> wrote:

> But if text in the find InputEntry gets selected (e.g by Ctrl+F) then the
> selection is taken away from the page text - irretrievably, and not only
> visually!
>

Ah OK - now I see what you mean.

 > Sounds to me we should add some code to your patch to unset the match
> > highlighting when the cursor moves. (There is a signal for mark position
> > changed and the cursor is a special mark.)
>
> This would be ok for me too! But first I'll investigate it further. It
> really could be some problem with GTK.
>

I know in one of the dialogs I have a work around to set back the selection
after focus goes back to the pageview. So seems to be a Gtk feature that the
interface can have only one selection at the time. Probably has to do with
the select-paste behavior, having multiple selections would ruin the
usability of that.

-- Jaap

[Jaap Karssenberg (jaap.karssenberg)]

Now integrating this branch - with some cleanups but keeping the idea of the matching tag.

review: Approve

[Oliver Joos (oliver-joos)]

I'd like to thank you for merging my not completely finished selection_fixes! Unfortunately I had not much time for zim recently.

I checked the latest Zim revision: your cleanup does improve my branch, especially its source code. I see that you copied the colors of matching text from Firefox. While this is reasonable, magenta/green does not look very harmonic, but I can live with that - no problem!

But a more annoying inconvenience has made it into lp:zim. In zim/gui/__init__.py show_search() my branch creates a SearchDialog and then sets the current selection as query string by invoking dialog.set_from_buffer(), which starts the search process. A query string may also be passed as parameter of SearchDialog(), which is cleaner from a developers point of view. But now the SearchDialog won't show up until the search is finished, which might take several seconds.

For the moment we could change show_search() to the more ugly variant of my "selection-fixes" branch. I could address this problem in my branch named "enhanced_search". There I would move the query string assignment (and the related search process) from SearchDialog.__init__() to a seperate method, e.g. "search(query_string=None)". Or what do you think?

[Jaap Karssenberg (jaap.karssenberg)]

This can be easily fixed by setting an attribute in SearchDialog.__init__()
and then do the actual search from show_all(). That way this logic stays
within the search dialog code. Will have a look later this weekend.

[Oliver Joos (oliver-joos)]

Just setting an attribute in __init__ sounds good. And everything can stay within the search dialog! But I'd prefer a new method in class SearchDialog to run an actual search. IMHO it seems not so obvious that searching is just a side-effect of show_all().

Could you please have a look at my branch https://code.launchpad.net/~oliver-joos/zim/enhanced_search? It heavily depends on the current code of SearchDialog and would be affected from bigger changes.

I'd also welcome some comments from you about my plans for enhanced_search (see branch details). And if you like the current state of the branch, I could set it to mature and open a new branch for the enhanced search plugin. The first part is finished, whereas the plugin could take longer.

[Jaap Karssenberg (jaap.karssenberg)]

On Mon, Sep 5, 2011 at 10:04 AM, Oliver Joos <email address hidden> wrote:

> Just setting an attribute in __init__ sounds good. And everything can stay
> within the search dialog! But I'd prefer a new method in class SearchDialog
> to run an actual search. IMHO it seems not so obvious that searching is just
> a side-effect of show_all().
>

That's a valid argument. We can take the argument out of the constructor and
have a "search(query)" method.

Could you please have a look at my branch
> https://code.launchpad.net/~oliver-joos/zim/enhanced_search? It heavily
> depends on the current code of SearchDialog and would be affected from
> bigger changes.
>
> I'd also welcome some comments from you about my plans for enhanced_search
> (see branch details). And if you like the current state of the branch, I
> could set it to mature and open a new branch for the enhanced search plugin.
> The first part is finished, whereas the plugin could take longer.
>

Afraid I can not merge that branch as it. Changes that contain new
translation strings will have to wait till after the next release. And for
feedback in the dialog I would like to use a call back from the search
function so it can be cancelled interactively as well.

Plans sound useful, my main interest is how they will be implemented. But
can discuss that part once you have a prototype.

Regards,

Jaap

=== modified file 'zim/gui/__init__.py'
--- zim/gui/__init__.py	2011-06-06 21:00:17 +0000
+++ zim/gui/__init__.py	2011-06-19 20:42:37 +0000
@@ -1272,7 +1272,10 @@
 
 	def show_search(self, query=None):
 		from zim.gui.searchdialog import SearchDialog
-		SearchDialog(self, query).show_all()
+		dialog = SearchDialog(self, query)
+		if query is None:
+			dialog.set_from_buffer()
+		dialog.show_all()
 
 	def show_search_backlinks(self):
 		query = 'LinksTo: "%s"' % self.page.name

=== modified file 'zim/gui/pageview.py'
--- zim/gui/pageview.py	2011-06-09 11:35:46 +0000
+++ zim/gui/pageview.py	2011-06-19 20:42:37 +0000
@@ -216,6 +216,9 @@
 
 PIXBUF_CHR = u'\uFFFC'
 
+# Minimal distance from mark to window border after scroll_to_mark()
+SCROLL_TO_MARK_MARGIN = 0.2
+
 # Regexes used for autoformatting
 heading_re = Re(r'^(={2,7})\s*(.*)\s*(\1)?$')
 page_re = Re(r'''(
@@ -386,6 +389,7 @@
 		'checked-checkbox': {},
 		'xchecked-checkbox': {},
 		'find-highlight': {'background': 'orange'},
+		'find-match': {'background': '#f2d957'}
 	}
 	static_style_tags = (
 		'h1', 'h2', 'h3', 'h4', 'h5', 'h6',
@@ -1023,7 +1027,7 @@
 			else:
 				return False
 
- 	def range_has_tags(self, func, start, end):
+	def range_has_tags(self, func, start, end):
 		'''Like range_has_tag() but uses a function to check for
 		multiple tags.
 		'''
@@ -1183,6 +1187,9 @@
 		bounds = self.get_selection_bounds()
 		if bounds:
 			start, end = bounds
+			end.backward_char() # exclude last line if selection ends_line
+			if not end.ends_line():
+				end.forward_char()
 			self.foreach_line(start.get_line(), end.get_line(), func, *args, **kwarg)
 			return True
 		else:
@@ -2044,6 +2051,9 @@
 		self.highlight = False
 		self.highlight_tag = self.buffer.create_tag(
 			None, **self.buffer.tag_styles['find-highlight'] )
+		self.match_tag = self.buffer.create_tag(
+			None, **self.buffer.tag_styles['find-match'] )
+		self.match_bounds = (None, None)
 
 	def get_state(self):
 		'''Returns the current search string, flags and highlight state'''
@@ -2099,9 +2109,9 @@
 		# Looking for a match starting at iter
 		if self.regex is None:
 			self.buffer.unset_selection()
+			self._apply_match(None, None)
 			return False
 
-
 		line = iter.get_line()
 		lastline = self.buffer.get_end_iter().get_line()
 		for start, end, _ in self._check_range(line, lastline, 1):
@@ -2109,19 +2119,22 @@
 				continue
 			else:
 				self.buffer.select_range(start, end)
+				self._apply_match(start, end)
 				return True
 		for start, end, _ in self._check_range(0, line, 1):
 			self.buffer.select_range(start, end)
+			self._apply_match(start, end)
 			return True
 
 		self.buffer.unset_selection()
+		self._apply_match(None, None)
 		return False
 
-
 	def find_previous(self):
 		'''Skip back to the previous match and select it'''
 		if self.regex is None:
 			self.buffer.unset_selection()
+			self._apply_match(None, None)
 			return False
 
 		iter = self.buffer.get_insert_iter()
@@ -2132,17 +2145,39 @@
 				continue
 			else:
 				self.buffer.select_range(start, end)
+				self._apply_match(start, end)
 				return True
 		for start, end, _ in self._check_range(lastline, line, -1):
 			self.buffer.select_range(start, end)
+			self._apply_match(start, end)
 			return True
 
 		self.buffer.unset_selection()
+		self._apply_match(None, None)
 		return False
 
-	def set_highlight(self, highlight):
+	def select_match(self):
+		# Select last match
+		bounds = self.match_bounds
+		if not None in bounds:
+			self.buffer.select_range(*bounds)
+
+	def _apply_match(self, start, end):
+		# Sets and displays new match bounds. (None, None) means "no match".
+		self.buffer.remove_tag(self.match_tag, *self.buffer.get_bounds())
+
+		self.match_bounds = (start, end)
+		if start and end:
+			self.buffer.apply_tag(self.match_tag, start, end)
+
+	def set_highlight(self, highlight, match = True):
+		# Sets or clears highlight and match tags
 		self.highlight = highlight
 		self._update_highlight()
+		if match:
+			self._apply_match(*self.match_bounds)
+		else:
+			self._apply_match(None, None)
 		# TODO we could connect to buffer signals to update highlighting
 		# when the buffer is modified.
 
@@ -2597,7 +2632,6 @@
 					return False
 			elif multi_line_indent(start, end):
 				level = []
-				buffer.strip_selection()
 				buffer.foreach_line_in_selection(
 					lambda l: level.append(buffer.get_indent(l)) )
 				if level and min(level) > 0:
@@ -3492,7 +3526,7 @@
 		if cursorpos != -1:
 			buffer.place_cursor(buffer.get_iter_at_offset(cursorpos))
 
-		self.view.scroll_to_mark(buffer.get_insert(), 0.3)
+		self.view.scroll_to_mark(buffer.get_insert(), SCROLL_TO_MARK_MARGIN)
 
 		self._buffer_signals = (
 			buffer.connect('textstyle-changed', self.do_textstyle_changed),
@@ -3584,7 +3618,7 @@
 	def set_cursor_pos(self, pos):
 		buffer = self.view.get_buffer()
 		buffer.place_cursor(buffer.get_iter_at_offset(pos))
-		self.view.scroll_to_mark(buffer.get_insert(), 0.2)
+		self.view.scroll_to_mark(buffer.get_insert(), SCROLL_TO_MARK_MARGIN)
 
 	def get_cursor_pos(self):
 		buffer = self.view.get_buffer()
@@ -4114,7 +4148,7 @@
 		self.hide_find() # remove previous highlighting etc.
 		buffer = self.view.get_buffer()
 		buffer.finder.find(string, flags)
-		self.view.scroll_to_mark(buffer.get_insert(), 0.3)
+		self.view.scroll_to_mark(buffer.get_insert(), SCROLL_TO_MARK_MARGIN)
 
 	def show_find(self, string=None, flags=0, highlight=False):
 		self.find_bar.show()
@@ -4675,7 +4709,7 @@
 			button.set_sensitive(ok)
 
 		if ok:
-			self.textview.scroll_to_mark(buffer.get_insert(), 0.3)
+			self.textview.scroll_to_mark(buffer.get_insert(), SCROLL_TO_MARK_MARGIN)
 
 	def on_find_entry_activate(self):
 		self.on_find_entry_changed()
@@ -4700,13 +4734,13 @@
 	def find_next(self):
 		buffer = self.textview.get_buffer()
 		buffer.finder.find_next()
-		self.textview.scroll_to_mark(buffer.get_insert(), 0.3)
+		self.textview.scroll_to_mark(buffer.get_insert(), SCROLL_TO_MARK_MARGIN)
 		self.textview.grab_focus()
 
 	def find_previous(self):
 		buffer = self.textview.get_buffer()
 		buffer.finder.find_previous()
-		self.textview.scroll_to_mark(buffer.get_insert(), 0.3)
+		self.textview.scroll_to_mark(buffer.get_insert(), SCROLL_TO_MARK_MARGIN)
 		self.textview.grab_focus()
 
 
@@ -4755,13 +4789,14 @@
 			self.pack_start(self.highlight_checkbox, False)
 
 		close_button = IconButton(gtk.STOCK_CLOSE, relief=False)
-		close_button.connect_object('clicked', self.__class__.hide, self)
+		close_button.connect_object('clicked', self.__class__.do_close_clicked, self)
 		self.pack_end(close_button, False)
 
 	def grab_focus(self):
 		self.find_entry.grab_focus()
 
 	def show(self):
+		self.on_highlight_toggled()
 		self.set_no_show_all(False)
 		self.show_all()
 
@@ -4769,7 +4804,11 @@
 		gtk.HBox.hide(self)
 		self.set_no_show_all(True)
 		buffer = self.textview.get_buffer()
-		buffer.finder.set_highlight(False)
+		buffer.finder.set_highlight(False, False)
+
+	def do_close_clicked(self):
+		self.hide()
+		self.textview.grab_focus()
 
 	def on_find_entry_activate(self):
 		self.on_find_entry_changed()
@@ -4778,6 +4817,7 @@
 	def do_key_press_event(self, event):
 		if event.keyval == KEYVAL_ESC:
 			self.hide()
+			self.textview.grab_focus()
 			return True
 		else:
 			return gtk.HBox.do_key_press_event(self, event)
@@ -4843,6 +4883,11 @@
 		buffer = self.textview.get_buffer()
 		buffer.finder.replace_all(string)
 
+	def do_response(self, id):
+		Dialog.do_response(self, id)
+		buffer = self.textview.get_buffer()
+		buffer.finder.set_highlight(False, False)
+
 
 class WordCountDialog(Dialog):
 

=== modified file 'zim/gui/searchdialog.py'
--- zim/gui/searchdialog.py	2011-05-06 14:54:51 +0000
+++ zim/gui/searchdialog.py	2011-06-19 20:42:37 +0000
@@ -68,6 +68,12 @@
 		button.connect('clicked', search)
 		self.query_entry.connect('activate', search)
 
+	def set_from_buffer(self):
+		'''Copies selected text of page to query entry, if there is any.'''
+		selection = self.ui.mainwindow.pageview.get_selection()
+		if selection:
+			self.query_entry.set_text(selection)
+
 
 class SearchResultsTreeView(BrowserTreeView):