Adds hidden preference 'always_set_from_selected' [...]

[etkaar]

Closes #2693.

Adds hidden preference 'always_set_from_selected' (editable in the various preferences) which always updates the search phrase in search dialogs with selected text after reopening the dialog (esp. using shortcuts), since default behaviour is to insert selected text as search phrase only if the dialog was not already opened before.

(Previous, closed PL: #2695)

---

I noticed the geany.html is automatically updated, so I only edited geany.txt. The change now applies to all search dialogs (Find, Find in Files and Replace) because I think it would be confusing if the preference would only apply to Find and Replace.

@elextr I played around wit git and created a dedicated branch for this Pull Request only, I hope I have done all correct.

[codebrainz]

I thought due to this comment the request for an option was dropped? I think all this started over not closing #2693 as a duplicate of #758 and not reading the discussion there.

Key points (duplicated across issues):

1. The goal of Find popup does not update with new highlighted text without closing and reopening it. #758 and Search for selected text when pressing [Ctrl]+[F] #2693 is to update the search text in the Find and Replace dialogs whenever Ctrl+f is pressed, rather than only the first time the dialogs are shown. This seems like a totally reasonable behaviour that most applications that have a Find feature do.
2. There was a misunderstanding in both issues where it was believed that the goal was to update the search text in the Find and Replace dialogs whenever the selection was changed (which would indeed be weird), thus prompting for a request about needing an option to turn this feature off.
3. It was clarified that the intention is 1 and not 2 so no option is needed.

I assume this issue is a duplicate of #2695 because a PR is bound the branch on which it was created, and it was suggested to not use master branch for the PR (a good suggestion for future PRs but not really essential/required).

If the above summary is accurate, then all that's needed is a patch like this:

diff --git a/src/search.c b/src/search.c
index 82682ae33..f45d7b477 100644
--- a/src/search.c
+++ b/src/search.c
@@ -575,14 +575,16 @@ void search_show_find_dialog(void)
 	}
 	else
 	{
-		/* only set selection if the dialog is not already visible */
-		if (! gtk_widget_get_visible(find_dlg.dialog) && sel)
+		if (sel != NULL)
+		{
+			/* update the search text from current selection */
 			gtk_entry_set_text(GTK_ENTRY(find_dlg.entry), sel);
+			/* reset the entry widget's background colour */
+			ui_set_search_entry_background(find_dlg.entry, TRUE);
+		}
 		gtk_widget_grab_focus(find_dlg.entry);
 		set_dialog_position(find_dlg.dialog, find_dlg.position);
 		gtk_widget_show(find_dlg.dialog);
-		if (sel != NULL) /* when we have a selection, reset the entry widget's background colour */
-			ui_set_search_entry_background(find_dlg.entry, TRUE);
 		/* bring the dialog back in the foreground in case it is already open but the focus is away */
 		gtk_window_present(GTK_WINDOW(find_dlg.dialog));
 	}
@@ -751,11 +753,13 @@ void search_show_replace_dialog(void)
 	}
 	else
 	{
-		/* only set selection if the dialog is not already visible */
-		if (! gtk_widget_get_visible(replace_dlg.dialog) && sel)
+		if (sel != NULL)
+		{
+			/* update the search text from current selection */
 			gtk_entry_set_text(GTK_ENTRY(replace_dlg.find_entry), sel);
-		if (sel != NULL) /* when we have a selection, reset the entry widget's background colour */
+			/* reset the entry widget's background colour */
 			ui_set_search_entry_background(replace_dlg.find_entry, TRUE);
+		}
 		gtk_widget_grab_focus(replace_dlg.find_entry);
 		set_dialog_position(replace_dlg.dialog, replace_dlg.position);
 		gtk_widget_show(replace_dlg.dialog);
@@ -1059,10 +1063,8 @@ void search_show_find_in_files_dialog_full(const gchar *text, const gchar *dir)
 
 	if (!text)
 	{
-		/* only set selection if the dialog is not already visible, or has just been created */
-		if (doc && ! sel && ! gtk_widget_get_visible(fif_dlg.dialog))
+		if (doc && ! sel)
 			sel = editor_get_default_selection(doc->editor, search_prefs.use_current_word, NULL);
-
 		text = sel;
 	}
 	entry = gtk_bin_get_child(GTK_BIN(fif_dlg.search_combo));

If that's the case, I propose to update the title and description of this PR and to force push a commit similar to above patch. I'm currently on vacation and will have time to merge this in the next days. @etkaar your work on this is appreciated!

[etkaar]

@codebrainz Correct, intention is 1. I closed the old PR because of the branch issue and because I changed the PL to also apply to Search > Find in Files.

If I understand your patch correct, it would change the default behaviour to do exactly that what my PL does, but per default and without a hidden preference. This is of course up to you, but I think a hidden preference may be better than changing the default behaviour without allowing users to fall back to the old default behaviour. My PL would resolve this issue. We could also just change the default value for search.always_set_from_selected from false to true.

[codebrainz]

If I understand your patch correct, it would change the default behaviour to do exactly that what my PL does, but per default and without a hidden preference.

Correct.

...but I think a hidden preference may be better than changing the default behaviour without allowing users to fall back to the old default behaviour.

IMO, a preference is not needed here and was only suggested as a result of a misunderstanding of the desired behaviour. Perhaps I'm missing the use case for not wanting the search text to be updated. If the reason to jump back to the Find dialogs without updating the text is to click the Find Next/Previous buttons, then one can simply use the keybindings for this (Ctrl+g and Ctrl+Shift+g by default). If the reason is to jump back and change some of the options without updating the search text, one can simply press Ctrl+f followed by Up to recall the previous search string before changing the other options.

Is there another use case I'm missing?

[etkaar]

Perhaps I'm missing the use case for not wanting the search text to be updated.

Well, I also personally don't see a use case for it, but on the other side, I don't see that many (or that many) users complained in five years. Also there must be a reason why the search phrase is (seems intentionally?) not updated in the current version. People have different ways of working. And since the users are used with that current behaviour - for which we don't see a reason yet -, I think it is not bad to let them fallback to the old behaviour, if we change the default. That is the only reason for the hidden preference.

[etkaar]

How should we proceed here?

1. Not changing default behaviour, but adding the hidden preference with default FALSE (= this PR).
2. Changing default behaviour by adding the hidden preference with default TRUE (= only a slight change on this PR).
3. Changing default behaviour without adding a hidden preference to revert back (= most of this PR would be useless).

I think the best option would be 2 because I think it is more safe for users which - for whatever reason - prefer the old behaviour.

[elextr]

Lets examine the use-case, currently all <ctrl>f does when the dialog is open, is it focuses the search pattern entry and selects its contents, it doesn't do any actions like next or prev. To get next or previous with the current pattern there are specific shortcuts (default bindings <ctrl>g and <ctrl><shift>g). So the only use-case for <ctrl>f now is to enter a new search pattern or to edit the current one.

The new behaviour will be to focus the search pattern entry and paste any current selection and select its contents, so it still provides the use-case of entering a new search pattern, just that the user will be typing over a different contents, so that use-case is still there. If there is no selection to paste the use-case to edit the current search pattern is still there.

So the only use-case to change is if there is a selection and the user wants to manually edit the current search pattern. But any movement key prior to <ctrl>f will remove the selection, and right doesn't even move the caret.

So AFAICT the new functionality is backward compatible with the exception of one sub-use-case which has a one keystroke workaround.

Therefore I agree with @codebrainz that it doesn't need a preference (and thats rare 😁), so we say 3.

Or you could wait and see if anyone else weighs in, might be a while though, given the time of year.

[kugel-]

I vote for 3 as well

[etkaar]

@elextr @kugel- Thanks a lot for your votes, then we will go with 3.

@elextr However, I feel it would make more sense for me to create a new branch (and so a new PL) for that, since it is more a fix than a feature and so the original changes would be kept. Would that be okay? EDIT: Or does overcomitting leaves the changes, so people can easily use the original approach?

[codebrainz]

@etkaar since this is already a duplicate of #2565, why not just force push to the branch of this PR? This topic already has a number of duplicate issues and PRs, no need to make more, IMO.

You can still keep this current work in a branch on your Github fork, Git lets you branch off of branches and/or rename branches. Something like:

# backup the current version
$ git branch -m always_set_from_selected always_set_from_selected_pref
# make a new branch off of master with the same name as this PR branch
$ git checkout -b always_set_from_selected master
# use my patch (or edit the files manually)
$ git apply <file_containing_my_patch_above>
# ...do other changes, commits, etc.
# force push to this branch to update this PR
$ git push <your remote/fork name> +always_set_from_selected
# if you want the backed up version on Github too
$ git push <your remote/fork name> always_set_from_selected_pref

[etkaar]

@codebrainz Perfect, then I will do that. I am not so familiar with all of that, but it makes fun! Thanks for your help.

[etkaar]

I was a bit struggling, sorry for any inconvinience, but I think now the correct commit is used for this PL now. @codebrainz I have used your patch, it seems to work as intended after compiling and testing.

[codebrainz]

@etkaar thanks, and sorry about all the confusion.

[etkaar]

@codebrainz You're welcome, thanks for the help :)