Merge lp://qastaging/~evfool/scratch/lp1309291 into lp://qastaging/~elementary-apps/scratch/scratch

Thanks for your branch Robert!

I think maybe we should just use a text button here. I don't think any themes ship with a "replace all" icon. While we're at it, maybe we should use a text button for "replace" as well. I think we're probably not going to get the best level of clarity out of an icon for this action.

Updated branch on your request, using "Replace" and "Replace all" as texts.

space after "=" on diffline 23

Hm, it seems sometimes it takes a while for the operation to complete. Can we add a Gtk.Spinner while the replace_all operation is in progress?

Also, would it be possible to have the button insensitive whenever clicking it would have no results?

> space after "=" on diffline 23
>
Fixed.
> Hm, it seems sometimes it takes a while for the operation to complete. Can we
> add a Gtk.Spinner while the replace_all operation is in progress?

I will tackle this issue too, it seems like it might be useful to update the whole search (custom implementation) to be handled by the sourceview, which might give us a speed boost, and option for async handling. As this isn't a trivial change, I would like to ask the scratch developers first, if they agree.
>
> Also, would it be possible to have the button insensitive whenever clicking it
> would have no results?

Fixed.

@Daniel Fore: After migrating the replace all to GtkSourceView the UI is still blocked, so adding a spinner and running the replace all in background seems to be a must, however in this context I would also suggest:
* adding a button to cancel the replace all action
* set the search and replace ui insensitive, as you shouldn't be able to run another replace all while a replace all action is in progress
* set the textbuffer not editable, as the text shouldn't be editable

What do you think?

That all sounds spot on to me :)

@Daniel: after experimenting on how to make replace all async, and asking the gedit developers, it turned out that replace all is fast enough (less than 1 second on a 3 mb text doc) with some attention (textbuffer changed signal handling after each replace did slow it down to more than a minute), that is why the gtksourceview devs did not add an async replace all.

So I have tweaked the code a bit, and now replace all seems fast enough for me to avoid the need for spinner, cancel button, and stuff like these. Could you please try and comment if you're satisfied with the result?

Wow, you've put a lot of work into this! Thanks Robert :)

It's still possible to create replaces that take a few seconds, but I think for any regular use (as in, not intentionally trying to break it) it's probably good.

I added a bounty to the bug about switching to GtkSourceView search for your extra time and effort :)

I think it's good for me, just needs a proper code review!

Hey Robert, your hard work is super appreciated here! Search and replace is working great for me, but replace-all is segfaulting for me. I was editing SourceManager.vala and I tried doing a replace-all for "private" to "public" and I got a segfault. Here's a backtrace: https://gist.githubusercontent.com/aroman/22ef3fddc286c9fe211a/raw/92347518fb1ec01ff4ac77cf5466453995bb3ee1/segfault

I'll try to dig into this later tonight if I can, but i suspect you'll have an easier shot debugging it.

@Avi Romanoff: thanks for the review, however the crash is absolutely strange, seems to mention symbolic iconinfo load, which doesn't seem to have anything to do with replace all.
I am running Ubuntu 14.04, no scratch plugins enabled, and I can't reproduce the crash. I would appreciate if you could take a look at it, as I can't reproduce it, so I can't do anything about it.

@Avi Romanoff: I have just tried this on another computer, couldn't reproduce it either. One small note: You mentioned in your testcase editing SourceManager.vala, is that SearchManager.vala, as I can't find a SourceManager.vala file in scratch? SearchManager.vala was the file I have tried with, and successfully replaced all occurrences of private with public, without a crash.

you know, my Isis install is really messed up these days -- I'm running gtk/glib from daily git, so it's entirely likely the code is fine, and the issue is on my end. I trust that if you've tried to reproduce this on two different computers (and it Dan hasn't run into it), it's good enough to merge.

When I get a sane install back, I'll try to reproduce, and then I'll report a bug if I can. But for now, I think it's better to just get this in trunk and keep moving forward.

There are some problems with the search function, which can be demonstrated as follows (All extensions turned off):

1) Create a new document consisting of a blank line followed by a line with one word on it followed by another blank line.
2) Type the word into the search box - it is found and selected, the arrows next to the search box are inactive (correct)
3) Place the cursor on the last blank line - the word is not highlighted (incorrect) but the up arrow is active (correct).
4) Press the up arrow - the word is not found and both arrows become inactive (incorrect)
5) Click in the search box - the word appears red indicating it does not occur in the document (incorrect).
6) Retype the word in the box - it is found and selected again (correct)
7) Place the cursor on the first blank line - the up arrow becomes active instead of the down arrow (incorrect)
8) CLick in the search field - the text does not become red (compare point 5).
9) Type something in the replace field - the buttons are active (correct)
10) Press "Replace" - nothing is replaced and the buttons stay active (incorrect)
11) Press "Replace all" - nothing is replaced and the bottoms become inactive (incorrect)

I would prefer the search to be always case sensitive by default. At the moment it is only case-sensitive if the search term is mixed case and I do not see the utility of this. Perhaps there could be a setting or tick-box to control case sensitivity.

There are some one line if statements - do these comply with the code style guidelines?

The program freezes when searching in certain machine generated text files (e.g. web pages, rtf files) but gdb indicated this to be a bug in Gtk (gedit had the same problem).

Thanks Jeremy for the detailed testing, I will look into the issues you have reported later in the afternoon.

Regarding the case sensitivity: the behaviour is the same as it was before (case sensitive only if the search text is mixed-case), we should ask Daniel Fore whether we want a checkbox for case sensitivity or not?

As for the incorrect behaviour: my hunch is that it has something to do with the wrap-around setting used as false by default (even before this merge [1]), and not changed anywhere.

[1] http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Widgets/SearchManager.vala#L58

I agree Daniel needs to decide the desired behaviour regarding case
sensitivity. There may have been a good design or technical reason why it
is like this. I now see that gedit behaves the same way. Its not an issue
that would prevent approval of this branch anyway as it was pre-existing.

On 26 May 2014 09:11, Robert Roth <email address hidden> wrote:

> Thanks Jeremy for the detailed testing, I will look into the issues you
> have reported later in the afternoon.
>
> Regarding the case sensitivity: the behaviour is the same as it was before
> (case sensitive only if the search text is mixed-case), we should ask
> Daniel Fore whether we want a checkbox for case sensitivity or not?
>
> As for the incorrect behaviour: my hunch is that it has something to do
> with the wrap-around setting used as false by default (even before this
> merge [1]), and not changed anywhere.
>
> [1]
> http://bazaar.launchpad.net/~elementary-apps/scratch/scratch/view/head:/src/Widgets/SearchManager.vala#L58
> --
> https://code.launchpad.net/~evfool/scratch/lp1309291/+merge/219621
> You are reviewing the proposed merge of lp:~evfool/scratch/lp1309291 into
> lp:scratch.
>

The case sensitivity is by design and as noted Gedit has the same behavior. The logic of this is that most of the time one will probably want to search without case sensitivity. However, if you'd like to search with case sensitivity you only need to include an uppercase letter in your search terms.

The only problem with this is that if you want to search for "word" and
exclude "Word" it cannot be done.

On 26 May 2014 19:35, Daniel Fore <email address hidden> wrote:

> The case sensitivity is by design and as noted Gedit has the same
> behavior. The logic of this is that most of the time one will probably want
> to search without case sensitivity. However, if you'd like to search with
> case sensitivity you only need to include an uppercase letter in your
> search terms.
> --
> https://code.launchpad.net/~evfool/scratch/lp1309291/+merge/219621
> You are reviewing the proposed merge of lp:~evfool/scratch/lp1309291 into
> lp:scratch.
>