See issue #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. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2697
-- Commit Summary --
* 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.
-- File Changes --
M doc/geany.txt (10) M src/keyfile.c (4) M src/search.c (12) M src/search.h (1)
-- Patch Links --
https://github.com/geany/geany/pull/2697.patch https://github.com/geany/geany/pull/2697.diff
I thought due to [this comment](https://github.com/geany/geany/pull/2695#issuecomment-748722240) 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 #758 and #2693 is to update the search text in the Find and Replace dialogs __whenever <kbd>Ctrl</kbd>+<kbd>f</kbd> 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](https://github.com/geany/geany/pull/2695#issuecomment-748728322) 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 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!
@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_.
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 (<kbd>Ctrl</kbd>+<kbd>g</kbd> and <kbd>Ctrl</kbd>+<kbd>Shift</kbd>+<kbd>g</kbd> by default). If the reason is to jump back and change some of the options without updating the search text, one can simply press <kbd>Ctrl</kbd>+<kbd>f</kbd> followed by <kbd>Up</kbd> to recall the previous search string before changing the other options.
Is there another use case I'm missing?
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 users complained. 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 using the hidden preference. That is the only reason for the hidden preference.
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.
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 :grin:), 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.
I vote for 3 as well
@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?
@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:
```bash # 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 ```
@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 pushed 1 commit.
6185afa4241d9861394af82fca61c63fcd53c107 Update search phrase for search/replace dialogs always with selected text on dialog reopening (especially when using shortcuts).
@etkaar pushed 0 commits.
Closed #2697.
Reopened #2697.
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.
Merged #2697 into master.
@etkaar thanks, and sorry about all the confusion.
@codebrainz You're welcome, thanks for the help :)
github-comments@lists.geany.org